Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 21, 2018, 15:54 UTC
- Message-ID
- <xmqqsh5gt9sm.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <87efh0pdln.fsf@javad.com>
Sergey Organov <sorganov@gmail.com> writes:
> When cherry-picking multiple commits, it's impossible to have both > merge- and non-merge commits on the same command-line. Not specifying > '-m 1' results in cherry-pick refusing to handle merge commits, while > specifying '-m 1' fails on non-merge commits.
Allowing "-m1" even when picking a single parent commit, because the 1-st parent is defined for such a commit, makes sense, espeially when running a cherry-pick on a range, exactly for the above reason. It is slightly less so when cherry-picking a single commit, but not by a large margin.
I think the original reasoning for requiring "-m $n" not present, especially because cherry-pick was originally for replaying only a single commit, was because it would lead somebody to propose that the command should behave as if -m1 is always given (and when trying to cherry-pick a merge relative to its second parent, give -m2 to override it), which in turn encourage the 'first-parent is special' world-view from the tool-side. IOW, "The worldview to treat the first-parent chain specially is correct, because Git has many features to work with that worldview conveniently" was something we wanted to avoid; rather "Such and such workflows benefit from treating the first-parent chain specially, so let's add features to do so" was we wanted to do, and of course, back then cherry-pick that allows mixture of merges and single-parent commits to be picked, which would have made the need to do something like this patch does felt greater, did not exist.
Now, it appears, at least to me, that the world pretty much accepted that the first-parent worldview is often very convenient and worth supporting by the tool, so the next logical step might be to set opts->mainline to 1 by default (and allow an explicit "-m $n" from the command line to override it). But that should happen after this patch lands---it is logically a separate step, I would think.
Show 8 quoted lines
> This patch allows '-m 1' for non-merge commits. Besides, as mainline is > always the only parent for a non-merge commit, it made little sense to > disable it in the first place. > > Signed-off-by: Sergey Organov <sorganov@gmail.com> > --- > sequencer.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-)
Show 15 quoted lines
>
> diff --git a/sequencer.c b/sequencer.c
> index 1ce6326..2393bdf 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1543,9 +1543,11 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
> return error(_("commit %s does not have parent %d"),
> oid_to_hex(&commit->object.oid), opts->mainline);
> parent = p->item;
> - } else if (0 < opts->mainline)
> - return error(_("mainline was specified but commit %s is not a merge."),
> - oid_to_hex(&commit->object.oid));
> + } else if (1 < opts->mainline)
> + /* Non-first parent explicitly specified as mainline for
> + * non-merge commit */Style.
/* * Our multi-line comments are to be * formatted like this. */
> + return error(_("commit %s does not have parent %d"),
> + oid_to_hex(&commit->object.oid), opts->mainline);
> else
> parent = commit->parents->item;