Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified

[Junio C Hamano <gitster@pobox.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.
>
> 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.


The feature to give a range to cherry-pick came much much later in
7e2bfd3f ("revert: allow cherry-picking more than one commit",
2010-06-02) that first appeared in v1.7.2.  The feature to allow
picking a merge commit came in 7791ecbc ("revert/cherry-pick: work
on merge commits as well", 2007-10-23), first appeared in v1.5.4.

In the original context to pick a single commit, it made perfect
sense to avoid mistakes by blindly passing '-m 1' to non-merge
commit.  It may be fair to say that we failed to reconsider what to
do with '-m 1' when we did 7e2bfd3f, but it is utterly an unfair 
history revisionism to say that it made little sense to disable it
in the first place.

The change to the code itself looks sane, but applying this patch
alone will break existing tests whose expectations must be updated,
and this new behaviour must be protected by a new test (or two) so
that we won't accidentally stop accepting "-m 1" for a single-parent
commit.

Thanks.

> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  sequencer.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index e1a4dd1..d0fd61b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1766,9 +1766,13 @@ 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
> +		 */
> +		return error(_("commit %s does not have parent %d"),
> +			     oid_to_hex(&commit->object.oid), opts->mainline);
>  	else
>  		parent = commit->parents->item;