| Message ID | 20240205141335.762947-1-vegard.nossum@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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
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.
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). 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 &&
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
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
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 &&
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 &&
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.
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
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 --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 &&
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(-)