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