Michael Haggerty <mhagger@xxxxxxxxxxxx> writes:
>> pick_one () {
>> ff=--ff
>> + extra_args=
>> + while test $# -gt 0
>> + do
>> + case "$1" in
>> + -n)
>> + ff=
>> + extra_args="$extra_args -n"
>> + ;;
>> + -*)
>> + warn "pick_one: ignored option -- $1"
>> + ;;
>
> This is an internal interface, right? I.e., user input isn't being
> processed here? If so, then the presence of an unrecognized option is a
> bug and it is preferable to "die" here rather than "warn".
>
> The same below and in at least one later commit.
And if this is purely an internal interface, then I really do not
see the point of allowing -n to be anywhere other than the front.
If we are planning to accept other random options to cherry-pick in
later steps, but we are not yet doing so at this step, then I do not
thin we want to have any loop like this before we actually start
accepting and passing them to the underlying cherry-pick.
Furthermore, if the "-n" is currently used as an internal signal
from the caller to pick_one() that it is executing the end-user
supplied "squash" in the insn sheet, it may be a good idea to change
that "-n" to something that is *NOT* a valid option to cherry-pick
at this step, before we start accepting user-supplied options and
relaying them to underlying cherry-pick.
One way to do so cleanly may be to _always_ add the type of pick as
the first parameter to pick_one, i.e. either "pick" or "squash", and
do:
pick_one () {
...
n_arg=
case "$1" in
pick) ;;
squash) n_arg=-n ;;
*) die "BUG: pick_one $1???" ;;
esac
shift
sha1=$1
...
output eval git cherry-pick $n_arg \
...
}
Also I suspect that you would need to be careful *not* to allow "-n"
to be given as part of the "random user-specified options" and pass
that to cherry-pick in the later steps of your series [*1*], and for
that you may need a loop that inspects the arguments like you had in
this patch.
[Footnote]
*1* The existing callers of "pick_one -n" very well know and expect
that the step will only update the working tree and the index
and it is the callers' responsibility to create a commit out of
that state (either by amending or committing); similarly the
existing callers of "pick_one" without "-n" very well know and
expect that the step will make a commit unless there is a
problem. I do not think you would consider it such a "problem
to replay the change in the named commit" for the end user's
insn sheet to pass a "-n".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html