Michael Haggerty <mhag...@alum.mit.edu> 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

        pick_one () {
                case "$1" in
                pick) ;;
                squash) n_arg=-n ;;
                *)      die "BUG: pick_one $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.


*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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to