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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Feb 4, 2024, 11:14 UTC
Message-ID: <0adb1068-ef10-44ed-ad1d-e0927a09245d@gmail.com>
In-Reply-To: <20240202091850.160203-1-vegard.nossum@oracle.com>

Hi Vegard

On 02/02/2024 09:18, Vegard Nossum wrote:

Show 24 quoted lines

> 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

Show 50 quoted lines

> 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