Re: [RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 20, 2014, 19:53 UTC
Message-ID: <xmqqvbrvcq8p.fsf@gitster.dls.corp.google.com>
In-Reply-To: <53A439B2.7000106@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 19 quoted lines

>>  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".