patcht/t3515-cherry-pick-rebase.sh: new testcase demonstrating broken behavior

> 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

> 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" <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?

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

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

> 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".

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

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

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

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

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