diff mbox series

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

Message ID 20240205141335.762947-1-vegard.nossum@oracle.com (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Vegard Nossum Feb. 5, 2024, 2:13 p.m. UTC
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(-)

Comments

Kristoffer Haugsbakk Feb. 5, 2024, 2:38 p.m. UTC | #1
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).
Junio C Hamano Feb. 5, 2024, 11:09 p.m. UTC | #2
"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?
Vegard Nossum Feb. 5, 2024, 11:14 p.m. UTC | #3
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
Junio C Hamano Feb. 6, 2024, 3:54 a.m. UTC | #4
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.
Phillip Wood Feb. 7, 2024, 2:03 p.m. UTC | #5
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 &&
Junio C Hamano Feb. 7, 2024, 4:39 p.m. UTC | #6
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 &&
Vegard Nossum Feb. 8, 2024, 8:48 a.m. UTC | #7
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
Phillip Wood Feb. 8, 2024, 2:26 p.m. UTC | #8
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
Junio C Hamano Feb. 8, 2024, 5:20 p.m. UTC | #9
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 &&
Phillip Wood Feb. 11, 2024, 11:11 a.m. UTC | #10
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 &&
Junio C Hamano Feb. 11, 2024, 5:05 p.m. UTC | #11
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.
Vegard Nossum Feb. 15, 2024, 2:24 p.m. UTC | #12
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
Junio C Hamano Feb. 15, 2024, 5:36 p.m. UTC | #13
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.
diff mbox series

Patch

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