Merge branch 'so/cherry-pick-always-allow-m1'

gitster · gitster · commit 77fbd96aebf7 · 2019-01-18T13:49:54.000-08:00
"git cherry-pick -m1" was forbidden when picking a non-merge
commit, even though there _is_ parent number 1 for such a commit.
This was done to avoid mistakes back when "cherry-pick" was about
picking a single commit, but is no longer useful with "cherry-pick"
that can pick a range of commits.  Now the "-m$num" option is
allowed when picking any commit, as long as $num names an existing
parent of the commit.

Technically this is a backward incompatible change; hopefully
nobody is relying on the error-checking behaviour.

* so/cherry-pick-always-allow-m1:
  t3506: validate '-m 1 -ff' is now accepted for non-merge commits
  t3502: validate '-m 1' argument is now accepted for non-merge commits
  cherry-pick: do not error on non-merge commits when '-m 1' is specified
  t3510: stop using '-m 1' to force failure mid-sequence of cherry-picks
diff --git a/sequencer.c b/sequencer.c
@@ -1790,9 +1790,13 @@ static int do_pick_commit(struct repository *r,
 			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;
 
diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
@@ -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
 
 '
 
-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 --
 
 '
 
diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
@@ -64,10 +64,10 @@ test_expect_success 'merge setup' '
 	git checkout -b new A
 '
 
-test_expect_success 'cherry-pick a non-merge with --ff and -m should fail' '
+test_expect_success 'cherry-pick explicit first parent of a non-merge with --ff' '
 	git reset --hard A -- &&
-	test_must_fail git cherry-pick --ff -m 1 B &&
-	git diff --exit-code A --
+	git cherry-pick --ff -m 1 B &&
+	git diff --exit-code C --
 '
 
 test_expect_success 'cherry pick a merge with --ff but without -m should fail' '
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
@@ -61,15 +61,19 @@ 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 &&
+	# to make sure that the session to cherry-pick a sequence
+	# gets interrupted, use a high-enough number that is larger
+	# than the number of parents of any commit we have created
+	mainline=4 &&
+	test_expect_code 128 git cherry-pick -s -m $mainline --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 &&
 	test_path_is_file .git/sequencer/opts &&
 	echo "true" >expect &&
 	git config --file=.git/sequencer/opts --get-all options.signoff >actual &&
 	test_cmp expect actual &&
-	echo "1" >expect &&
+	echo "$mainline" >expect &&
 	git config --file=.git/sequencer/opts --get-all options.mainline >actual &&
 	test_cmp expect actual &&
 	echo "recursive" >expect &&

Original file line number	Diff line number	Diff line change
`@@ -40,12 +40,12 @@ test_expect_success 'cherry-pick -m complains of bogus numbers' '`
`40`	`40`	`test_expect_code 129 git cherry-pick -m 0 b`
`41`	`41`	`'`
`42`	`42`
`43`		`-test_expect_success 'cherry-pick a non-merge with -m should fail' '`
	`43`	`+test_expect_success 'cherry-pick explicit first parent of a non-merge' '`
`44`	`44`
`45`	`45`	`git reset --hard &&`
`46`	`46`	`git checkout a^0 &&`
`47`		`- test_expect_code 128 git cherry-pick -m 1 b &&`
`48`		`- git diff --exit-code a --`
	`47`	`+ git cherry-pick -m 1 b &&`
	`48`	`+ git diff --exit-code c --`
`49`	`49`
`50`	`50`	`'`
`51`	`51`
`@@ -84,12 +84,12 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent should f`
`84`	`84`
`85`	`85`	`'`
`86`	`86`
`87`		`-test_expect_success 'revert a non-merge with -m should fail' '`
	`87`	`+test_expect_success 'revert explicit first parent of a non-merge' '`
`88`	`88`
`89`	`89`	`git reset --hard &&`
`90`	`90`	`git checkout c^0 &&`
`91`		`- test_must_fail git revert -m 1 b &&`
`92`		`- git diff --exit-code c`
	`91`	`+ git revert -m 1 b &&`
	`92`	`+ git diff --exit-code a --`
`93`	`93`
`94`	`94`	`'`
`95`	`95`
Original file line number	Diff line number	Diff line change
`@@ -64,10 +64,10 @@ test_expect_success 'merge setup' '`
`64`	`64`	`git checkout -b new A`
`65`	`65`	`'`
`66`	`66`
`67`		`-test_expect_success 'cherry-pick a non-merge with --ff and -m should fail' '`
	`67`	`+test_expect_success 'cherry-pick explicit first parent of a non-merge with --ff' '`
`68`	`68`	`git reset --hard A -- &&`
`69`		`- test_must_fail git cherry-pick --ff -m 1 B &&`
`70`		`- git diff --exit-code A --`
	`69`	`+ git cherry-pick --ff -m 1 B &&`
	`70`	`+ git diff --exit-code C --`
`71`	`71`	`'`
`72`	`72`
`73`	`73`	`test_expect_success 'cherry pick a merge with --ff but without -m should fail' '`