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

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.

Signed-off-by Sergey Organov <[email protected]>

---
 sequencer.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

sequencer.c +0 -0

index 1ce6326..2393bdf 100644

sequencer.c +2 -0

+		return error(_("commit %s does not have parent %d"),
+			     oid_to_hex(&commit->object.oid), opts->mainline);
 	else
 		parent = commit->parents->item;

Quoted 4 lines · Sergey Organov

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.

sequencer.c static int do_pick_commit(enum todo_command command, struct commit *commit, · line 1548

Quoted 3 lines

sequencer.c +3 -0

+	} 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.
	 */

Quoted 4 lines · Sergey Organov

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.

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.

Review

Quoted 3 lines

(quoted diff) +3 -0

+	} 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.
	 */

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.

Signed-off-by Sergey Organov <[email protected]>

---
 sequencer.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

sequencer.c +7 -3

index e1a4dd1..d0fd61b 100644
@@ -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;

Quoted 8 lines · Sergey Organov

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.

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.

Quoted 2 lines · Junio C Hamano

Sergey Organov <[email protected]> writes:

Quoted 6 lines

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.

I fixed most of the tests, but

"t3510/4: cherry-pick persists opts correctly"

is an offender for me. It looks like it [ab]uses current "-m 1" behavior just to stop in the middle of the sequence, and I'm not sure how to fix it most suitably.

I came up with the following as a preparatory change. Looks acceptable?

-- 8< --

t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks

We are going to allow 'git cherry-pick -m 1' for non-merge commits, so this method to force failure will stop to work.

Use '-m 4' instead as it's very unlikely we will ever have such an octopus in this test setup.

Modified t/t3510-cherry-pick-sequence.sh

t/t3510-cherry-pick-sequence.sh +3 -3

index c84eeef..a873cf4 100755
@@ -61,7 +61,8 @@ test_expect_success 'cherry-pick mid-cherry-pick-sequence' '
 
 test_expect_success 'cherry-pick persists opts correctly' '
 	pristine_detach initial &&
-	test_expect_code 128 git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours initial..anotherpick &&
+	m=4 &&
+	test_expect_code 128 git cherry-pick -s -m $m --strategy=recursive -X patience -X ours initial..anotherpick &&
 	test_path_is_dir .git/sequencer &&
 	test_path_is_file .git/sequencer/head &&
 	test_path_is_file .git/sequencer/todo &&

@@ -69,7 +70,7 @@ test_expect_success 'cherry-pick persists opts correctly' '
 	echo "true" >expect &&
 	git config --file=.git/sequencer/opts --get-all options.signoff >actual &&
 	test_cmp expect actual &&
-	echo "1" >expect &&
+	echo "$m" >expect &&
 	git config --file=.git/sequencer/opts --get-all options.mainline >actual &&
 	test_cmp expect actual &&
 	echo "recursive" >expect &&
 
-- 8< --

Signed-off-by Sergey Organov <[email protected]>

---
 t/t3502-cherry-pick-merge.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

t/t3502-cherry-pick-merge.sh +3 -3

index b160271..3259bd5 100755
@@ -40,12 +40,12 @@ test_expect_success 'cherry-pick -m complains of bogus numbers' '
 	test_expect_code 129 git cherry-pick -m 0 b
 '
 
-test_expect_success 'cherry-pick a non-merge with -m should fail' '
+test_expect_success 'cherry-pick explicit first parent of a non-merge' '
 
 	git reset --hard &&
 	git checkout a^0 &&
-	test_expect_code 128 git cherry-pick -m 1 b &&
-	git diff --exit-code a --
+	git cherry-pick -m 1 b &&
+	git diff --exit-code c --
 
 '
 

@@ -84,12 +84,12 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent should f
 
 '

t/t3502-cherry-pick-merge.sh +0 -0

 '
 

Signature · 1 line

2.10.0.1.g57b01a3

Quoted 1 line · Sergey Organov

Signed-off-by Sergey Organov <[email protected]>

Quoted 4 lines

t/t3502-cherry-pick-merge.sh +0 -0

@@ -84,12 +84,12 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent should f
 
 '

t/t3502-cherry-pick-merge.sh test_expect_success 'cherry pick a merge relative to nonexistent parent should f · line 92

Quoted 10 lines

t/t3502-cherry-pick-merge.sh +3 -3

 
-test_expect_success 'revert a non-merge with -m should fail' '
+test_expect_success 'revert explicit first parent of a non-merge' '
 
 	git reset --hard &&
 	git checkout c^0 &&
-	test_must_fail git revert -m 1 b &&
-	git diff --exit-code c
+	git revert -m 1 b &&
+	git diff --exit-code a

You need disambiguaion here, otherwise this test fails on case-insensitive file systems:

++git diff --exit-code a
fatal: ambiguous argument 'a': both revision and filename Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
error: last command exited with $?=128
not ok 8 - revert explicit first parent of a non-merge

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 series allow '-m 1' for non-merge commits as well as fixes relevant tests in accordance.

It also opens the way to making '-m 1' the default, that would be inline with the trends to assume first parent to be the mainline in most workflows.

Sergey Organov (4):
t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks
cherry-pick: do not error on non-merge commits when '-m 1' is
specified
t3502: validate '-m 1' argument is now accepted for non-merge commits
t3506: validate '-m 1 -ff' is now accepted for non-merge commits

 sequencer.c                     | 10 +++++++---
 t/t3502-cherry-pick-merge.sh    | 12 ++++++------
 t/t3506-cherry-pick-ff.sh       |  6 +++---
 t/t3510-cherry-pick-sequence.sh |  8 ++++++--
 4 files changed, 22 insertions(+), 14 deletions(-)

Hi Junio,

What's the status of these patches?

Sergey Organov <[email protected]>, Sergey Organov

Quoted 3 lines · <[email protected]> writes:

Hi Junio,

What's the status of these patches?

The status of these patches is "Just updated on the list", as far as I am concerned, and its cover letter would have described what's improved relative to the previous round better than whatever answer I could give here ;-)

Quoted 4 lines · Sergey Organov

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.

I think that "first-parent is special" is the way to go indeed for porcelain, as it does make many thing easier and more convenient[*].

[*] One example that immediately comes to mind is "git log -p" for a merge commit. I doesn't currently (as of v2.10) show the first-parent diff, for whatever reason. "git log -p -m --first-parent" is needed to get the answer to most "obvious" question: what (merge) commit did to my mainline? "git show" has its own issues.

"git log -p --first-parent" that requires "-m" to show the single ball of wax diff for a merge is a separate matter. When the user explicitly says "log --first-parent", it is a clear indication that the user does not want to see individual steps of how side branches built what each merge brings into the mainline. From that point of view, ever sice I introduced "--first-parent" traversal, I've been wondering what the true downside would be if we turned "-m" on automatically when these two options are used without "-m".

Quoted 2 lines · Sergey Organov

I think that "first-parent is special" is the way to go indeed for porcelain, as it does make many thing easier and more convenient[*].

Perhaps. However ...

Quoted 5 lines

[*] One example that immediately comes to mind is "git log -p" for a merge commit. I doesn't currently (as of v2.10) show the first-parent diff, for whatever reason. "git log -p -m --first-parent" is needed to get the answer to most "obvious" question: what (merge) commit did to my mainline? "git show" has its own issues.

... this is very much deliberate and will remain so.

A single ball of wax "diff M^ M" for a merge commit is not always what you would want, especially while viewing "git log -p" (without "--first-parent").

How about changing "git show -p M" to output "diff -p M^ M" rather than "diff-tree --cc M" for merge commits? It's really surprising specifying -p has no visible effect.

Also, is current output of "git log -m", being extremely confusing, suitable for anything? Maybe consider to change it to output diff with respect to the first parent only? Though it's then a pity "-m" lacks argument here, similar to what it has in cherry-pick.

Quoted 3 lines · Sergey Organov

How about changing "git show -p M" to output "diff -p M^ M" rather than "diff-tree --cc M" for merge commits? It's really surprising specifying -p has no visible effect.

That's because "-p" is already the default, and the format selection is orthogonal to the handling of merge commits. Providing "-m" would actually override the "--cc" default (though "--first-parent -m" is likely to be less noisy, per this discussion).

As far as defaults go, I dunno. The idea is that "--cc" would give you a nice summary of what the merge _itself_ had to touch. I think that's valuable, too. If we were starting from scratch, I think there could be a discussion about whether one default is better than the other. But at this point I have a hard time finding one so much obviously better than the other to merit changing the behavior.

Quoted 12 lines · Jeff King

Quoted 3 lines · Sergey Organov

How about changing "git show -p M" to output "diff -p M^ M" rather than "diff-tree --cc M" for merge commits? It's really surprising specifying -p has no visible effect.

That's because "-p" is already the default, and the format selection is orthogonal to the handling of merge commits. Providing "-m" would actually override the "--cc" default (though "--first-parent -m" is likely to be less noisy, per this discussion).

Quoted 6 lines

As far as defaults go, I dunno. The idea is that "--cc" would give you a nice summary of what the merge _itself_ had to touch. I think that's valuable, too. If we were starting from scratch, I think there could be a discussion about whether one default is better than the other. But at this point I have a hard time finding one so much obviously better than the other to merit changing the behavior.

Indeed, some of us would view a first parent diff default for merges as problematic. However, I'd like to point out (or remind) that these two options aren't the only ways you could view a merge. Thomas Rast's --remerge-diff[1] is another (even if not yet part of git.git). Gerrit uses something similar-ish for its default way of showing a merge.

Quoted 20 lines · Elijah Newren

Quoted 12 lines · Jeff King

Quoted 3 lines · Sergey Organov

How about changing "git show -p M" to output "diff -p M^ M" rather than "diff-tree --cc M" for merge commits? It's really surprising specifying -p has no visible effect.

That's because "-p" is already the default, and the format selection is orthogonal to the handling of merge commits. Providing "-m" would actually override the "--cc" default (though "--first-parent -m" is likely to be less noisy, per this discussion).

Quoted 6 lines

As far as defaults go, I dunno. The idea is that "--cc" would give you a nice summary of what the merge _itself_ had to touch. I think that's valuable, too. If we were starting from scratch, I think there could be a discussion about whether one default is better than the other. But at this point I have a hard time finding one so much obviously better than the other to merit changing the behavior.

Quoted 6 lines

Indeed, some of us would view a first parent diff default for merges as problematic. However, I'd like to point out (or remind) that these two options aren't the only ways you could view a merge. Thomas Rast's --remerge-diff[1] is another (even if not yet part of git.git). Gerrit uses something similar-ish for its default way of showing a merge.

Heh, I almost mentioned remerge-diff, but since it's not actually part of Git, I didn't want to get into a tangent. But since you mention it, yes, I actually find it quite a useful way of looking at the diff, especially when I want to see what the person resolving the conflicts actually _did_. The --cc combined diff is too eager to throw away hunks that resolved purely to one side (which _most_ of the time is what you want, but when you're hunting a possible error in the merge, it's quite confusing).

How close is merge-recursive.c to actually doing a pure in-memory merge?

I seem to recall that was a (the?) sticking point for the original remerge-diff.

Quoted 3 lines · Sergey Organov

How about changing "git show -p M" to output "diff -p M^ M" rather than "diff-tree --cc M" for merge commits? It's really surprising specifying -p has no visible effect.

That's because "-p" is already the default, and the format selection is orthogonal to the handling of merge commits.

Providing "-m" would actually override the "--cc" default (though "--first-parent -m" is likely to be less noisy, per this discussion).

As far as defaults go, I dunno.

The idea is that "--cc" would give you a nice summary of what the merge _itself_ had to touch. I think that's valuable, too.

If we were starting from scratch, I think there could be
a discussion about whether one default is better than the other. But at this point I have a hard time finding one so much obviously better than the other to merit changing the behavior.

Quoted 4 lines

Also, is current output of "git log -m", being extremely confusing, suitable for anything? Maybe consider to change it to output diff with respect to the first parent only? Though it's then a pity "-m" lacks argument here, similar to what it has in cherry-pick.

I've used "-m" without "--first-parent" sometimes in order to track down mis-merges manually.

It's not usually a big deal to say "--first-parent" if that's what you want. But one thing I don't think is currently possible is to ask only for the first-parent diff, but _not_ restrict the actual traversal.

If that's what you mean by giving an argument to "-m", then yeah, I think that would be a useful addition.