threads / patch / 50036

v2, 4 partst3506: validate '-m 1 -ff' is now accepted for non-merge commits

Subject: [PATCH v2 4/4] t3506: validate '-m 1 -ff' is now accepted for non-merge commits

## tl;dr

10 messages between Dec 14, 2018 and Jan 6, 2019. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Sergey Organov· Dec 14, 2018, 04:39 UTC · lore

[PATCH v2 0/4] Allow 'cherry-pick -m 1' for non-merge commits

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

-- 
2.10.0.1.g57b01a3

Sergey Organov· Dec 14, 2018, 04:53 UTC · re: Sergey Organov · lore

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t3506-cherry-pick-ff.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Show changes to t/t3506-cherry-pick-ff.sh +3 −3

diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh
index fb889ac..127dd00 100755
--- 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' '

-- 
2.10.0.1.g57b01a3

Sergey Organov· Dec 14, 2018, 04:53 UTC · re: Sergey Organov · lore

[PATCH v2 1/4] 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.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t3510-cherry-pick-sequence.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Show changes to t/t3510-cherry-pick-sequence.sh +6 −2

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index c84eeef..941d502 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -61,7 +61,11 @@ 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 &&
@@ -69,7 +73,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 "$mainline" >expect &&
 	git config --file=.git/sequencer/opts --get-all options.mainline >actual &&
 	test_cmp expect actual &&
 	echo "recursive" >expect &&

-- 
2.10.0.1.g57b01a3

Sergey Organov· Dec 14, 2018, 04:53 UTC · re: Sergey Organov · lore

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

This patch allows '-m 1' for non-merge commits. As mainline is always the only parent for a non-merge commit, it makes little sense to disable it.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 sequencer.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Show changes to sequencer.c +7 −3

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;

-- 
2.10.0.1.g57b01a3

Sergey Organov· Dec 14, 2018, 04:53 UTC · re: Sergey Organov · lore

[PATCH v2 3/4] t3502: validate '-m 1' argument is now accepted for non-merge commits

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 t/t3502-cherry-pick-merge.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Show changes to t/t3502-cherry-pick-merge.sh +6 −6

diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index b160271..3259bd5 100755
--- 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
 
 '

-- 
2.10.0.1.g57b01a3

SZEDER Gábor· Jan 3, 2019, 17:22 UTC · re: Sergey Organov · lore

Re: [PATCH v2 3/4] t3502: validate '-m 1' argument is now accepted for non-merge commits

On Fri, Dec 14, 2018 at 07:53:51AM +0300, Sergey Organov wrote:

Show 38 quoted lines

> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  t/t3502-cherry-pick-merge.sh | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
> index b160271..3259bd5 100755
> --- 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

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

Show 6 quoted lines

>  
>  '
>  
> -- 
> 2.10.0.1.g57b01a3
>

Sergey Organov· Jan 6, 2019, 14:41 UTC · re: SZEDER Gábor · lore

Re: [PATCH v2 3/4] t3502: validate '-m 1' argument is now accepted for non-merge commits

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Fri, Dec 14, 2018 at 07:53:51AM +0300, Sergey Organov wrote:
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>

[...]

Show 24 quoted lines

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

Good catch, -- thanks a lot!

-- Sergey

Sergey Organov· Dec 25, 2018, 12:39 UTC · re: Sergey Organov · lore

Re: [PATCH v2 0/4] Allow 'cherry-pick -m 1' for non-merge commits

Hi Junio,

What's the status of these patches?

Thanks,

-- Sergey

Sergey Organov <sorganov@gmail.com> writes:

Show 24 quoted lines

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

Junio C Hamano· Dec 26, 2018, 22:52 UTC · re: Sergey Organov · lore

Re: [PATCH v2 0/4] Allow 'cherry-pick -m 1' for non-merge commits

Sergey Organov <sorganov@gmail.com>, Sergey Organov <sorganov@gmail.com> 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 ;-)

I checked the overall diff between the previous round and the result of applying all four patches to find out that the updates are only to the tests. The changes in the code (still) looked correct.

Show changes to 3 files +15 −11

t/t3502-cherry-pick-merge.sh, t/t3506-cherry-pick-ff.sh, t/t3510-cherry-pick-sequence.sh

diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
index b1602718f8..3259bd59eb 100755
--- 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
index fb889ac6f0..127dd0082f 100755
--- 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
index c84eeefdc9..941d5026da 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -61,7 +61,11 @@ 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 &&
@@ -69,7 +73,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 "$mainline" >expect &&
 	git config --file=.git/sequencer/opts --get-all options.mainline >actual &&
 	test_cmp expect actual &&
 	echo "recursive" >expect &&

Sergey Organov· Dec 29, 2018, 09:10 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 0/4] Allow 'cherry-pick -m 1' for non-merge commits

Junio C Hamano <gitster@pobox.com> writes:

Show 11 quoted lines

> Sergey Organov <sorganov@gmail.com>, Sergey Organov
> <sorganov@gmail.com> 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 ;-)

Yeah, I need to be more thorough, thanks!

-- Sergey