[PATCH] t/t3515-cherry-pick-rebase.sh: new testcase demonstrating broken behavior

[PATCH] t/t3515-cherry-pick-rebase.sh: new testcase demonstrating broken behavior

[Vegard Nossum]

Running "git cherry-pick" as an x-command in the rebase plan loses the
original authorship information.

Write a known-broken test case for this:

    $ (cd t && ./t3515-cherry-pick-rebase.sh)
    ok 1 - setup
    ok 2 - cherry-pick preserves authorship information
    not ok 3 - cherry-pick inside rebase preserves authorship information # TODO known breakage
    # still have 1 known breakage(s)
    # passed all remaining 2 test(s)
    1..3

Running with --verbose we see the diff between expected and actual:

    --- expected    2024-02-02 08:54:48.954753285 +0000
    +++ actual      2024-02-02 08:54:48.966753294 +0000
    @@ -1 +1 @@
    -Original Author
    +A U Thor

As far as I can tell, this is due to the check in print_advice()
which deletes CHERRY_PICK_HEAD when GIT_CHERRY_PICK_HELP is set,
but I'm not sure what a good fix would be.

Cc: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 t/t3515-cherry-pick-rebase.sh | 37 +++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100755 t/t3515-cherry-pick-rebase.sh

diff --git a/t/t3515-cherry-pick-rebase.sh b/t/t3515-cherry-pick-rebase.sh
new file mode 100755
index 0000000000..ffe6f5fe2a
--- /dev/null
+++ b/t/t3515-cherry-pick-rebase.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='test cherry-pick during a rebase'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	test_commit --author "Original Author <original.author@example.com>" foo file contents1 &&
+	git checkout -b feature &&
+	test_commit --author "Another Author <another.author@example.com>" bar file contents2
+'
+
+test_expect_success 'cherry-pick preserves authorship information' '
+	git checkout -B tmp feature &&
+	test_must_fail git cherry-pick foo &&
+	git add file &&
+	git commit --no-edit &&
+	git log -1 --format='%an' foo >expected &&
+	git log -1 --format='%an' >actual &&
+	test_cmp expected actual
+'
+
+test_expect_failure 'cherry-pick inside rebase preserves authorship information' '
+	git checkout -B tmp feature &&
+	echo "x git cherry-pick -x foo" >rebase-plan &&
+	test_must_fail env GIT_SEQUENCE_EDITOR="cp rebase-plan" git rebase -i feature &&
+	git add file &&
+	git commit --no-edit &&
+	git log -1 --format='%an' foo >expected &&
+	git log -1 --format='%an' >actual &&
+	test_cmp expected actual
+'
+
+test_done
-- 
2.34.1

Re: [PATCH] t/t3515-cherry-pick-rebase.sh: new testcase demonstrating broken behavior

[Phillip Wood]

Hi Vegard

On 02/02/2024 09:18, Vegard Nossum wrote:
> Running "git cherry-pick" as an x-command in the rebase plan loses the
> original authorship information.
> 
> Write a known-broken test case for this:
> 
>      $ (cd t && ./t3515-cherry-pick-rebase.sh)
>      ok 1 - setup
>      ok 2 - cherry-pick preserves authorship information
>      not ok 3 - cherry-pick inside rebase preserves authorship information # TODO known breakage
>      # still have 1 known breakage(s)
>      # passed all remaining 2 test(s)
>      1..3
> 
> Running with --verbose we see the diff between expected and actual:
> 
>      --- expected    2024-02-02 08:54:48.954753285 +0000
>      +++ actual      2024-02-02 08:54:48.966753294 +0000
>      @@ -1 +1 @@
>      -Original Author
>      +A U Thor
> 
> As far as I can tell, this is due to the check in print_advice()
> which deletes CHERRY_PICK_HEAD when GIT_CHERRY_PICK_HELP is set,
> but I'm not sure what a good fix would be.

Thanks for reporting this and for the test case. I agree with your 
diagnosis. I think the simplest fix would be to unset 
GIT_CHERRY_PICK_HELP in the child environment in sequencer.c:do_exec(). 
Long term we should stop setting GIT_CHERRY_PICK_HELP when rebasing and 
hard code the rebase conflicts message in sequencer.c as the environment 
variable is a vestige of the scripted rebase implementation.

To work around the bug I think you can change the exec lines in the todo 
list to

     exec unset GIT_CHERRY_PICK_HELP; git cherry-pick ...

Best Wishes

Phillip

> Cc: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> ---
>   t/t3515-cherry-pick-rebase.sh | 37 +++++++++++++++++++++++++++++++++++
>   1 file changed, 37 insertions(+)
>   create mode 100755 t/t3515-cherry-pick-rebase.sh
> 
> diff --git a/t/t3515-cherry-pick-rebase.sh b/t/t3515-cherry-pick-rebase.sh
> new file mode 100755
> index 0000000000..ffe6f5fe2a
> --- /dev/null
> +++ b/t/t3515-cherry-pick-rebase.sh
> @@ -0,0 +1,37 @@
> +#!/bin/sh
> +
> +test_description='test cherry-pick during a rebase'
> +
> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +	test_commit --author "Original Author <original.author@example.com>" foo file contents1 &&
> +	git checkout -b feature &&
> +	test_commit --author "Another Author <another.author@example.com>" bar file contents2
> +'
> +
> +test_expect_success 'cherry-pick preserves authorship information' '
> +	git checkout -B tmp feature &&
> +	test_must_fail git cherry-pick foo &&
> +	git add file &&
> +	git commit --no-edit &&
> +	git log -1 --format='%an' foo >expected &&
> +	git log -1 --format='%an' >actual &&
> +	test_cmp expected actual
> +'
> +
> +test_expect_failure 'cherry-pick inside rebase preserves authorship information' '
> +	git checkout -B tmp feature &&
> +	echo "x git cherry-pick -x foo" >rebase-plan &&
> +	test_must_fail env GIT_SEQUENCE_EDITOR="cp rebase-plan" git rebase -i feature &&
> +	git add file &&
> +	git commit --no-edit &&
> +	git log -1 --format='%an' foo >expected &&
> +	git log -1 --format='%an' >actual &&
> +	test_cmp expected actual
> +'
> +
> +test_done

[PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands

[Vegard Nossum]

Running "git cherry-pick" as an x-command in the rebase plan loses the
original authorship information.

To fix this, unset GIT_CHERRY_PICK_HELP for 'exec' commands.

Link: https://lore.kernel.org/git/0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com/
Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
---
 sequencer.c                   | 1 +
 t/t3515-cherry-pick-rebase.sh | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 91de546b32..f49a871ac0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3641,6 +3641,7 @@ static int do_exec(struct repository *r, const char *command_line)
 	fprintf(stderr, _("Executing: %s\n"), command_line);
 	cmd.use_shell = 1;
 	strvec_push(&cmd.args, command_line);
+	strvec_push(&cmd.env, "GIT_CHERRY_PICK_HELP");
 	status = run_command(&cmd);
 
 	/* force re-reading of the cache */
diff --git a/t/t3515-cherry-pick-rebase.sh b/t/t3515-cherry-pick-rebase.sh
index ffe6f5fe2a..5cb2b96f66 100755
--- a/t/t3515-cherry-pick-rebase.sh
+++ b/t/t3515-cherry-pick-rebase.sh
@@ -23,7 +23,7 @@ test_expect_success 'cherry-pick preserves authorship information' '
 	test_cmp expected actual
 '
 
-test_expect_failure 'cherry-pick inside rebase preserves authorship information' '
+test_expect_success 'cherry-pick inside rebase preserves authorship information' '
 	git checkout -B tmp feature &&
 	echo "x git cherry-pick -x foo" >rebase-plan &&
 	test_must_fail env GIT_SEQUENCE_EDITOR="cp rebase-plan" git rebase -i feature &&
-- 
2.34.1

Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands

[Kristoffer Haugsbakk]

On Mon, Feb 5, 2024, at 15:13, Vegard Nossum wrote:
> Link: https://lore.kernel.org/git/0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com/
> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>

`Link` is not really used a lot. Junio’s `refs/notes/amlog` will point
back to the patch (which is often close to the “suggested by” and so
on).

-- 
Kristoffer Haugsbakk

Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands

[Junio C Hamano]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

> On Mon, Feb 5, 2024, at 15:13, Vegard Nossum wrote:
>> Link: https://lore.kernel.org/git/0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com/
>> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
>> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
>
> `Link` is not really used a lot. Junio’s `refs/notes/amlog` will point
> back to the patch (which is often close to the “suggested by” and so
> on).

Good.  Also, is there [PATCH 1/2] that comes before this patch?

Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands

[Vegard Nossum]

On 06/02/2024 00:09, Junio C Hamano wrote:
> "Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> 
>> On Mon, Feb 5, 2024, at 15:13, Vegard Nossum wrote:
>>> Link: https://lore.kernel.org/git/0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com/
>>> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
>>> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
>>
>> `Link` is not really used a lot. Junio’s `refs/notes/amlog` will point
>> back to the patch (which is often close to the “suggested by” and so
>> on).
> 
> Good.  Also, is there [PATCH 1/2] that comes before this patch?

Yes, kind of -- that's the testcase at the root of the thread:

https://lore.kernel.org/git/20240202091850.160203-1-vegard.nossum@oracle.com/

("t/t3515-cherry-pick-rebase.sh: new testcase demonstrating broken 
behavior")


Vegard

Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands

[Junio C Hamano]

Vegard Nossum <vegard.nossum@oracle.com> writes:

> On 06/02/2024 00:09, Junio C Hamano wrote:
>> "Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
>> 
>>> On Mon, Feb 5, 2024, at 15:13, Vegard Nossum wrote:
>>>> Link: https://lore.kernel.org/git/0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com/
>>>> Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
>>>> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
>>>
>>> `Link` is not really used a lot. Junio’s `refs/notes/amlog` will point
>>> back to the patch (which is often close to the “suggested by” and so
>>> on).
>> Good.  Also, is there [PATCH 1/2] that comes before this patch?
>
> Yes, kind of -- that's the testcase at the root of the thread:
>
> https://lore.kernel.org/git/20240202091850.160203-1-vegard.nossum@oracle.com/
>
> ("t/t3515-cherry-pick-rebase.sh: new testcase demonstrating broken
> behavior")

If the first one was NOT marked as [1/2], it is customary to call
such an "we thought just one patch was sufficient, but here is
another" step [2/1] instead, and that was why I was confused.

Perhaps it is a good idea to squash them together as a single bugfix
patch?

Thanks.

Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands

[Phillip Wood]

On 06/02/2024 03:54, Junio C Hamano wrote:
> Vegard Nossum <vegard.nossum@oracle.com> writes:
> 
>> On 06/02/2024 00:09, Junio C Hamano wrote:
> Perhaps it is a good idea to squash them together as a single bugfix
> patch?

I think so, I'm not sure we want to add a new test file just for this
either. Having the test in a separate file was handy for debugging but
I think something like the diff below would suffice though I wouldn't
object to checking the author of the cherry-picked commit

Best Wishes

Phillip

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index c5f30554c6..84a92d6da0 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -153,6 +153,18 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '
  	git rebase --continue
  '
  
+test_expect_success 'cherry-pick works with rebase --exec' '
+	test_when_finished "git cherry-pick --abort; \
+			    git rebase --abort; \
+			    git checkout primary" &&
+	echo "exec git cherry-pick G" >todo &&
+	(
+		set_replace_editor todo &&
+		test_must_fail git rebase -i D D
+	) &&
+	test_cmp_rev G CHERRY_PICK_HEAD
+'
+
  test_expect_success 'rebase -x with empty command fails' '
  	test_when_finished "git rebase --abort ||:" &&
  	test_must_fail env git rebase -x "" @ 2>actual &&

Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> On 06/02/2024 03:54, Junio C Hamano wrote:
>> Vegard Nossum <vegard.nossum@oracle.com> writes:
>> 
>>> On 06/02/2024 00:09, Junio C Hamano wrote:
>> Perhaps it is a good idea to squash them together as a single bugfix
>> patch?
>
> I think so, I'm not sure we want to add a new test file just for this
> either. Having the test in a separate file was handy for debugging but
> I think something like the diff below would suffice though I wouldn't
> object to checking the author of the cherry-picked commit

Very true (I didn't even notice that the original "bug report
disguised as a test addition" was inventing a totally new file).

Thanks.

>
> Best Wishes
>
> Phillip
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index c5f30554c6..84a92d6da0 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -153,6 +153,18 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '
>  	git rebase --continue
>  '
>  +test_expect_success 'cherry-pick works with rebase --exec' '
> +	test_when_finished "git cherry-pick --abort; \
> +			    git rebase --abort; \
> +			    git checkout primary" &&
> +	echo "exec git cherry-pick G" >todo &&
> +	(
> +		set_replace_editor todo &&
> +		test_must_fail git rebase -i D D
> +	) &&
> +	test_cmp_rev G CHERRY_PICK_HEAD
> +'
> +
>  test_expect_success 'rebase -x with empty command fails' '
>  	test_when_finished "git rebase --abort ||:" &&
>  	test_must_fail env git rebase -x "" @ 2>actual &&

Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands

[Vegard Nossum]

On 07/02/2024 17:39, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> On 06/02/2024 03:54, Junio C Hamano wrote:
>>> Vegard Nossum <vegard.nossum@oracle.com> writes:
>>>
>>>> On 06/02/2024 00:09, Junio C Hamano wrote:
>>> Perhaps it is a good idea to squash them together as a single bugfix
>>> patch?
>>
>> I think so, I'm not sure we want to add a new test file just for this
>> either. Having the test in a separate file was handy for debugging but
>> I think something like the diff below would suffice though I wouldn't
>> object to checking the author of the cherry-picked commit
> 
> Very true (I didn't even notice that the original "bug report
> disguised as a test addition" was inventing a totally new file).

I'm sorry, but I'm confused about what I'm supposed to do now.

There is now another test case and it sounds like you would prefer that
one over mine, but I didn't write it and there is no SOB, so I cannot
submit that with the fix if I were to "squash them together".

I am not a regular contributor so I don't have a good grasp on things
like why you don't want a new test file for this, or why you (as the
maintainer) can't just squash the patches yourself if that's what you
prefer.

Thanks,


Vegard

Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands

[Phillip Wood]

Hi Vegard

On 08/02/2024 08:48, Vegard Nossum wrote:
> I'm sorry, but I'm confused about what I'm supposed to do now.
> 
> There is now another test case and it sounds like you would prefer that
> one over mine, but I didn't write it and there is no SOB, so I cannot
> submit that with the fix if I were to "squash them together".

Here's my SOB for the diff in 
https://lore.kernel.org/git/4e6d503a-8564-4536-82a7-29c489f5fec3@gmail.com/

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>

I think that typically for small suggestions like that we just add a 
Helped-by: trailer but feel free to add my SOB if you want.

> I am not a regular contributor so I don't have a good grasp on things
> like why you don't want a new test file for this,

I think keeping related tests together helps contributors see which test 
files to run when they're changing code (running the whole suite each 
time is too slow). There is also a (small) setup overhead for each new 
file. For tests like this it is a bit ambiguous whether it belongs with 
the other "rebase --exec" tests or the other "cherry-pick" tests. I 
opted to put it with the other "rebase --exec" tests as I think it is 
really fixing a bug with rebase rather than cherry-pick.

Best Wishes

Phillip

Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> I think that typically for small suggestions like that we just add a
> Helped-by: trailer but feel free to add my SOB if you want.

Thanks, both.  Here is what I assembled from the pieces.

----- >8 --------- >8 --------- >8 --------- >8 -----
From: Vegard Nossum <vegard.nossum@oracle.com>
Date: Fri, 2 Feb 2024 10:18:50 +0100
Subject: [PATCH] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands

Running "git cherry-pick" as an x-command in the rebase plan loses
the original authorship information.

To fix this, unset GIT_CHERRY_PICK_HELP for 'exec' commands.

Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 sequencer.c                   |  1 +
 t/t3404-rebase-interactive.sh | 12 ++++++++++++
 2 files changed, 13 insertions(+)

diff --git a/sequencer.c b/sequencer.c
index d584cac8ed..ed30ceaf8b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3647,6 +3647,7 @@ static int do_exec(struct repository *r, const char *command_line)
 	fprintf(stderr, _("Executing: %s\n"), command_line);
 	cmd.use_shell = 1;
 	strvec_push(&cmd.args, command_line);
+	strvec_push(&cmd.env, "GIT_CHERRY_PICK_HELP");
 	status = run_command(&cmd);
 
 	/* force re-reading of the cache */
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index c5f30554c6..84a92d6da0 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -153,6 +153,18 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '
 	git rebase --continue
 '
 
+test_expect_success 'cherry-pick works with rebase --exec' '
+	test_when_finished "git cherry-pick --abort; \
+			    git rebase --abort; \
+			    git checkout primary" &&
+	echo "exec git cherry-pick G" >todo &&
+	(
+		set_replace_editor todo &&
+		test_must_fail git rebase -i D D
+	) &&
+	test_cmp_rev G CHERRY_PICK_HEAD
+'
+
 test_expect_success 'rebase -x with empty command fails' '
 	test_when_finished "git rebase --abort ||:" &&
 	test_must_fail env git rebase -x "" @ 2>actual &&
-- 
2.43.0-561-g235986be82

Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands

[Phillip Wood]

Hi Junio

On 08/02/2024 17:20, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> I think that typically for small suggestions like that we just add a
>> Helped-by: trailer but feel free to add my SOB if you want.
> 
> Thanks, both.  Here is what I assembled from the pieces.
> 
> ----- >8 --------- >8 --------- >8 --------- >8 -----
> From: Vegard Nossum <vegard.nossum@oracle.com>
> Date: Fri, 2 Feb 2024 10:18:50 +0100
> Subject: [PATCH] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands
> 
> Running "git cherry-pick" as an x-command in the rebase plan loses
> the original authorship information.

It might be worth explaining why this happens

This is because rebase sets the GIT_CHERRY_PICK_HELP environment 
variable to customize the advice given to users when there are conflicts 
which causes the sequencer to remove CHERRY_PICK_HEAD.

> To fix this, unset GIT_CHERRY_PICK_HELP for 'exec' commands.

The patch itself looks fine

Best Wishes

Phillip

> Helped-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   sequencer.c                   |  1 +
>   t/t3404-rebase-interactive.sh | 12 ++++++++++++
>   2 files changed, 13 insertions(+)
> 
> diff --git a/sequencer.c b/sequencer.c
> index d584cac8ed..ed30ceaf8b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3647,6 +3647,7 @@ static int do_exec(struct repository *r, const char *command_line)
>   	fprintf(stderr, _("Executing: %s\n"), command_line);
>   	cmd.use_shell = 1;
>   	strvec_push(&cmd.args, command_line);
> +	strvec_push(&cmd.env, "GIT_CHERRY_PICK_HELP");
>   	status = run_command(&cmd);
>   
>   	/* force re-reading of the cache */
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index c5f30554c6..84a92d6da0 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -153,6 +153,18 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '
>   	git rebase --continue
>   '
>   
> +test_expect_success 'cherry-pick works with rebase --exec' '
> +	test_when_finished "git cherry-pick --abort; \
> +			    git rebase --abort; \
> +			    git checkout primary" &&
> +	echo "exec git cherry-pick G" >todo &&
> +	(
> +		set_replace_editor todo &&
> +		test_must_fail git rebase -i D D
> +	) &&
> +	test_cmp_rev G CHERRY_PICK_HEAD
> +'
> +
>   test_expect_success 'rebase -x with empty command fails' '
>   	test_when_finished "git rebase --abort ||:" &&
>   	test_must_fail env git rebase -x "" @ 2>actual &&

Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> Hi Junio
>
> On 08/02/2024 17:20, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>> 
>>> I think that typically for small suggestions like that we just add a
>>> Helped-by: trailer but feel free to add my SOB if you want.
>> Thanks, both.  Here is what I assembled from the pieces.
>> ----- >8 --------- >8 --------- >8 --------- >8 -----
>> From: Vegard Nossum <vegard.nossum@oracle.com>
>> Date: Fri, 2 Feb 2024 10:18:50 +0100
>> Subject: [PATCH] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands
>> Running "git cherry-pick" as an x-command in the rebase plan loses
>> the original authorship information.
>
> It might be worth explaining why this happens
>
> This is because rebase sets the GIT_CHERRY_PICK_HELP environment
> variable to customize the advice given to users when there are
> conflicts which causes the sequencer to remove CHERRY_PICK_HEAD.

True.  I'd prefer to see the original submitter assemble the pieces
and come up with the final version, rather than me doing so.

Thanks.

Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands

[Vegard Nossum]

On 11/02/2024 18:05, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>> On 08/02/2024 17:20, Junio C Hamano wrote:
>>> Phillip Wood <phillip.wood123@gmail.com> writes:
>> It might be worth explaining why this happens
>>
>> This is because rebase sets the GIT_CHERRY_PICK_HELP environment
>> variable to customize the advice given to users when there are
>> conflicts which causes the sequencer to remove CHERRY_PICK_HEAD.
> 
> True.  I'd prefer to see the original submitter assemble the pieces
> and come up with the final version, rather than me doing so.

Thanks for explaining and sorry for the delay. I saw the patch was
merged to main now, but I will keep this in mind for next time.


Vegard

Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands

[Junio C Hamano]

Vegard Nossum <vegard.nossum@oracle.com> writes:

> On 11/02/2024 18:05, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>> On 08/02/2024 17:20, Junio C Hamano wrote:
>>>> Phillip Wood <phillip.wood123@gmail.com> writes:
>>> It might be worth explaining why this happens
>>>
>>> This is because rebase sets the GIT_CHERRY_PICK_HELP environment
>>> variable to customize the advice given to users when there are
>>> conflicts which causes the sequencer to remove CHERRY_PICK_HEAD.
>> True.  I'd prefer to see the original submitter assemble the pieces
>> and come up with the final version, rather than me doing so.
>
> Thanks for explaining and sorry for the delay. I saw the patch was
> merged to main now, but I will keep this in mind for next time.

Thanks for finding and fixing.  Hope we'll see more of your
contributions in the future.