git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Sergey Organov <sorganov@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified
Date: Thu, 21 Jun 2018 08:54:17 -0700	[thread overview]
Message-ID: <xmqqsh5gt9sm.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <87efh0pdln.fsf@javad.com> (Sergey Organov's message of "Fri, 25 May 2018 15:42:03 +0300")

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.

> 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(-)

>
> 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;

  reply	other threads:[~2018-06-21 15:54 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-25 12:42 Sergey Organov
2018-06-21 15:54 ` Junio C Hamano [this message]
2018-06-22  9:16   ` Sergey Organov
2018-12-12  5:35   ` Sergey Organov
2018-12-13  4:20     ` Junio C Hamano
2018-12-13  6:35       ` Sergey Organov
2018-12-13 15:35         ` Sergey Organov
2018-12-14  2:36           ` Junio C Hamano
2018-12-14  4:39             ` [PATCH v3 0/4] Allow 'cherry-pick -m 1' for non-merge commits Sergey Organov
2018-12-14  4:53               ` [PATCH v3 4/4] t3506: validate '-m 1 -ff' is now accepted " Sergey Organov
2018-12-14  4:53               ` [PATCH v3 1/4] t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks Sergey Organov
2018-12-14  4:53               ` [PATCH v3 3/4] t3502: validate '-m 1' argument is now accepted for non-merge commits Sergey Organov
2018-12-14  4:53               ` [PATCH v3 2/4] cherry-pick: do not error on non-merge commits when '-m 1' is specified Sergey Organov
2018-12-14  4:39             ` [PATCH v2 0/4] Allow 'cherry-pick -m 1' for non-merge commits Sergey Organov
2018-12-14  4:53               ` [PATCH v2 2/4] cherry-pick: do not error on non-merge commits when '-m 1' is specified Sergey Organov
2018-12-14  4:53               ` [PATCH v2 1/4] t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks Sergey Organov
2018-12-14  4:53               ` [PATCH v2 3/4] t3502: validate '-m 1' argument is now accepted for non-merge commits Sergey Organov
2019-01-03 17:22                 ` SZEDER Gábor
2019-01-06 14:41                   ` Sergey Organov
2018-12-14  4:53               ` [PATCH v2 4/4] t3506: validate '-m 1 -ff' " Sergey Organov
2018-12-25 12:39               ` [PATCH v2 0/4] Allow 'cherry-pick -m 1' " Sergey Organov, Sergey Organov
2018-12-26 22:52                 ` Junio C Hamano
2018-12-29  9:10                   ` Sergey Organov
2019-03-19 11:29   ` [PATCH] cherry-pick: do not error on non-merge commits when '-m 1' is specified Sergey Organov
2019-03-20  0:38     ` Junio C Hamano
2019-03-20  5:09       ` Jeff King
2019-03-25  6:43       ` Sergey Organov
2019-03-26 16:32         ` Jeff King
2019-03-26 22:07           ` Elijah Newren
2019-03-26 22:20             ` Jeff King
2019-03-27  0:33               ` Elijah Newren
2019-03-27 13:54           ` Sergey Organov
  -- strict thread matches above, loose matches on Subject: below --
2018-05-25 12:42 Sergey Organov

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqsh5gt9sm.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=sorganov@gmail.com \
    --subject='Re: [PATCH] cherry-pick: do not error on non-merge commits when '\''-m 1'\'' is specified' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git