Re: [RFC PATCH] cherry-pick: set default `--mainline` parent to 1

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 20, 2019, 04:45 UTC
Message-ID: <xmqq1s32w3vu.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CALm+SVJR5BCHi_r7B279gKDukD4mYDQuv=K5guje73YDVmOxug@mail.gmail.com>

"C.J. Jameson" <cjcjameson@gmail.com> writes:

Show 5 quoted lines

> When cherry-picking from a standard merge commit, the parent should
> always be 1, as it has the meaningful new changes which were added to
> the mainline. If the source commit is unique in some way and a user
> wants to specify a value other than 1, they still can, but it's up to
> them to be aware of which parent is which.

I do not have a firm opinion for or against the "if the commit being cherry-picked happens to be a merge, cherry-pick it relative to the first parent by default, without requiring the '-m 1' option". It may not be such a bad idea after all.

But I do have a very strong opinion against adding yet another option that takes an optional argument. If we want to allow cherry-picking a merge commit just as easy as cherrry-picking a single-parent commit, "git cherry-pick -m merge" (assuming 'merge' is the tip of a branch that is a merge commit) that still requires the user to say "-m" is not a good improvement. We should just accept "git cherry-pick merge" without any "-m" if we want to move in this direction, I would think.

Show 10 quoted lines

> --m parent-number::
> ---mainline parent-number::
> +-m [parent-number]::
> +--mainline [parent-number]::
>   Usually you cannot cherry-pick a merge because you do not know which
>   side of the merge should be considered the mainline.  This
>   option specifies the parent number (starting from 1) of
>   the mainline and allows cherry-pick to replay the change
> - relative to the specified parent.
> + relative to the specified parent. The default parent-number is 1.

I somehow sense a total whitespace breakage. Usually these lines are HT indented, but I see only a single whitespace indent around here.

So, the first part of the paragraph needs to get revamped. It won't be "because", but "unless", and with your change, you no longer know which side is mainline---unless you tell Git otherwise, the first parent is assumed to be the mainline in the new world order.

	-m <parent-number>::
	--mainline <parent-number>::
                When cherry-picking a merge, by default, the first
                parent is taken as the mainline, and the command
                replays the change relative to the first parent of
                the given commit.  If the merge commit you are
                cherry-picking records the mainline as second or
                later parent, you can use this option to replay
                the change relative to the specified parent (parents
                count from 1, which is the first parent).
	+
	The default parent-number is 1.  Giving a parent number
	larger than the number of parent the cherry-picked commit has
	is an error.

or something like that.

Show 6 quoted lines

> diff --git a/builtin/revert.c b/builtin/revert.c
> index a47b53ceaf..280d1f00a9 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -105,8 +105,8 @@ static int run_sequencer(int argc, const char
> **argv, struct replay_opts *opts)

Line-wrapped patch cannot be applied.

Show 7 quoted lines

>   OPT_BOOL('e', "edit", &opts->edit, N_("edit the commit message")),
>   OPT_NOOP_NOARG('r', NULL),
>   OPT_BOOL('s', "signoff", &opts->signoff, N_("add Signed-off-by:")),
> - OPT_CALLBACK('m', "mainline", opts, N_("parent-number"),
> -      N_("select mainline parent"), option_parse_m),
> + { OPTION_INTEGER, 'm', "mainline", &opts->mainline, N_("parent-number"),
> + N_("select mainline parent"), PARSE_OPT_OPTARG, NULL, 1 },

Let's not do optarg. Use of OPT_INTEGER is fine, if you really want to, but then you are losing the last caller of option_parse_m() and the callback function itself must also be removed.

You'd also need to do a few more things:

#1 make it NONEG, so that we reject "--no-mainline"; in the new
   world order, cherry-picking a non-merge commit is replaying
   against the first parent; there is no situation where use of
   --no-mainline makes sense (perhaps other than cherry-picking the
   initial commit, for which there is no mainline parent).

#2 initialize opts->mainline to 1, so that with no --mainline option
   from the command line, we will still get opt->mainline == 1
   (alternatively, you could initialize opt->mainline == -1 and
   treat opt->mainline <= 0 as if it were set to 1 when the code
   must choose against which parent to replay the changes in a much
   deeper place in the codepath).

#3 think about how you'd loosen option compatibility check around
   --mainline (if you do #2, as opt->mainline would never be 0 now)
   and adjust the verify_opt_compatible() call(s).

#4 as OPT_INTEGER won't ensure that you get a positive integer,
   you'd need to somehow reject --mainline=0 (or --mainline=-1 for
   that matter) given from the command line.