From: Junio C Hamano <gitster@pobox•com>
To: Michael Haggerty <mhagger@alum•mit.edu>
Cc: Fabian Ruch <bafain@gmail•com>, git@vger•kernel.org
Subject: Re: [RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible
Date: Fri, 20 Jun 2014 12:53:10 -0700 [thread overview]
Message-ID: <xmqqvbrvcq8p.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <53A439B2.7000106@alum.mit.edu> (Michael Haggerty's message of "Fri, 20 Jun 2014 15:40:02 +0200")
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.
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".
next prev parent reply other threads:[~2014-06-20 19:53 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <cover.1403146774.git.bafain@gmail.com>
2014-06-19 3:28 ` [RFC PATCH 1/7] rebase -i: Make option handling in pick_one more flexible Fabian Ruch
2014-06-20 13:40 ` Michael Haggerty
2014-06-20 19:53 ` Junio C Hamano [this message]
2014-06-23 0:04 ` Fabian Ruch
2014-06-21 23:21 ` Fabian Ruch
2014-06-23 16:09 ` Johannes Schindelin
2014-06-19 3:28 ` [RFC PATCH 2/7] rebase -i: Teach do_pick the option --edit Fabian Ruch
2014-06-20 13:41 ` Michael Haggerty
2014-06-22 0:09 ` Fabian Ruch
2014-06-19 3:28 ` [RFC PATCH 3/7] rebase -i: Stop on root commits with empty log messages Fabian Ruch
2014-06-21 0:33 ` Eric Sunshine
2014-06-22 0:32 ` Fabian Ruch
2014-06-19 3:28 ` [RFC PATCH 4/7] rebase -i: Commit only once when rewriting picks Fabian Ruch
2014-06-19 3:28 ` [RFC PATCH 5/7] rebase -i: Do not die in do_pick Fabian Ruch
2014-06-19 3:28 ` [RFC PATCH 6/7] rebase -i: Prepare for squash in terms of do_pick --amend Fabian Ruch
2014-06-19 3:28 ` [RFC PATCH 7/7] rebase -i: Teach do_pick the options --amend and --file Fabian Ruch
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqvbrvcq8p.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=bafain@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=mhagger@alum$(echo .)mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox