Hi Junio,

On 06/20/2014 09:53 PM, Junio C Hamano wrote:
> 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.

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.

> 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


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

Reply via email to