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

From: Fabian Ruch <bafain@gmail.com>
Date: Jun 23, 2014, 00:04 UTC
Message-ID: <53A76EFE.3080909@gmail.com>
In-Reply-To: <xmqqvbrvcq8p.fsf@gitster.dls.corp.google.com>

Hi Junio,

On 06/20/2014 09:53 PM, Junio C Hamano wrote:

Show 27 quoted lines

> Michael Haggerty <mhagger@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.

Ok, until we require pick_one to accept options apart from -n, this patch is postponed, for the presence of a single option is checked easiest without the loop. It might be the case that rewriting replayed commits in do_pick is the better alternative anyway and that it will never be required to relay user-specified options beyond do_pick.

Show 31 quoted lines

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

I really like the idea of being explicit about how pick_one shall replay the named commit and not using the cherry-pick option name for the squash case. However, pick_one will never receive random user-specified options. do_pick is the interface function which handles the pick arguments. If any user-specified options are relayed to pick_one and cherry-pick, they will be validated by do_pick first (using a loop like above).

   Fabian

Show 11 quoted lines

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