| Message ID | cover.1732481200.git.code@khaugsbakk.name (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
Hi Kristoffer Thanks for re-rolling, I've left some comments on the range-diff On 24/11/2024 20:56, kristofferhaugsbakk@fastmail.com wrote: > From: Kristoffer Haugsbakk <code@khaugsbakk.name> > > Range-diff against v2: > 1: fc3b4438845 ! 1: a46767263f6 sequencer: comment checked-out branch properly > [...] > @@ t/t3400-rebase.sh: test_expect_success 'rebase when inside worktree subdirectory > + git checkout base && > + test_commit msg3 && > + git checkout topic2 && > -+ git -c core.commentChar=% rebase --update-refs base > ++ GIT_SEQUENCE_EDITOR="cat >actual" git -c core.commentChar=% \ > ++ rebase -i --update-refs base && > ++ grep "% Ref refs/heads/wt-topic checked out at" actual && > ++ grep "% Ref refs/heads/topic2 checked out at" actual It would be nicer to use test_grep here as it prints a helpful message when the pattern is not found which aids debugging test failures > 2: 710c5b1a3f6 ! 2: 7a452142666 sequencer: comment `--reference` subject line properly > [...] > @@ t/t3501-revert-cherry-pick.sh: test_expect_success 'identification of reverted c > +test_expect_success 'git revert --reference with core.commentChar' ' > + test_when_finished "git reset --hard to-ident" && > + git checkout --detach to-ident && > -+ git -c core.commentChar=% revert \ > ++ GIT_EDITOR="cat | head -4 >actual" git -c core.commentChar=% revert \ > + --edit --reference HEAD && "cat" is not doing anything here, GIT_EDITOR="head -n4 > actual" is all you need (I've added "-n" there as I'm not sure how portable a bare "-4" is). > -+ git log -1 --format=%B HEAD >actual && > -+ printf "This reverts commit $(git show -s \ > -+ --pretty=reference HEAD^).\n\n" \ > -+ >expect && > ++ cat <<-EOF >expect && > ++ % *** SAY WHY WE ARE REVERTING ON THE TITLE LINE *** > ++ > ++ This reverts commit $(git show -s --pretty=reference HEAD^). > ++ > ++ EOF > + test_cmp expect actual This looks good - we're now checking that the user sees the comment when they edit the message. > +' > + > 3: 86b4b485e0b ! 3: 4c342bc0422 sequencer: comment commit messages properly > @@ Metadata > ## Commit message ## > sequencer: comment commit messages properly > > + The rebase todo editor has commands like `fixup -c` which affects > + the commit messages of the rebased commits.[1] For example: > + > + pick hash1 <msg> > + fixup hash2 <msg> > + fixup -c hash3 <msg> > + > + This says that hash2` and hash3 should be squashed into hash1 and Stray "`" > + that hash3’s commit message should be used for the resulting commit. > + So the user is presented with an editor where the two first commit > + messages are commented out and the third is not. I'd perhaps say If there are conflicts when applying commit hash3 then the user is presented ... as we only show all the messages to the user when there are conflicts. > However this does > + not work if `core.commentChar`/`core.commentString` is in use since > + the comment char is hardcoded (#) in this `sequencer.c` function. > + As a result the first commit message will not be commented out. > + > + † 1: See 9e3cebd97cb (rebase -i: add fixup [-C | -c] command, > + 2021-01-29) > + > + Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk> > + Reported-by: Taylor Blau <me@ttaylorr.com> > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> Thanks for updating the trailers, they look good to me Best Wishes Phillip
On Mon, Nov 25, 2024, at 11:07, phillip.wood123@gmail.com wrote: > Hi Kristoffer > > Thanks for re-rolling, I've left some comments on the range-diff Hi Phillip, thanks for the review! I should be able to fix these and reroll today. > [...] > Stray "`" > >> + that hash3’s commit message should be used for the resulting commit. >> + So the user is presented with an editor where the two first commit >> + messages are commented out and the third is not. > > I'd perhaps say > > If there are conflicts when applying commit hash3 then the user is > presented ... > > as we only show all the messages to the user when there are conflicts. I use `fixup -c` for the third/last commit here. So I am prompted to edit the commit message. I tested this on this series: git checkout --detach kh/sequencer-comment-char git rebase -i fd3785337beb285ed7fd67ce6fc3d3bed2097b40 Which gave me [this] editor without these changes (with `core.commentChar` set to `%`). > >> However this does >> + not work if `core.commentChar`/`core.commentString` is in use since >> + the comment char is hardcoded (#) in this `sequencer.c` function. >> + As a result the first commit message will not be commented out. >> + >> + † 1: See 9e3cebd97cb (rebase -i: add fixup [-C | -c] command, >> + 2021-01-29) >> + >> + Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >> Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk> >> + Reported-by: Taylor Blau <me@ttaylorr.com> >> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> > > Thanks for updating the trailers, they look good to me > > Best Wishes > > Phillip † this: % This is a combination of 3 commits. % This is the 1st commit message: sequencer: comment checked-out branch properly `git rebase --update-ref` does not insert commands for dependent/sub- branches which are checked out.[1] Instead it leaves a comment about that fact. The comment char is hardcoded (#). In turn the comment line gets interpreted as an invalid command when `core.commentChar`/ `core.commentString` is in use. † 1: See 900b50c242 (rebase: add --update-refs option, 2022-07-19) Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> % The commit message #2 will be skipped: % sequencer: comment `--reference` subject line properly % % `git revert --reference <commit>` leaves behind a comment in the % first line:[1] % % # *** SAY WHY WE ARE REVERTING ON THE TITLE LINE *** % % Meaning that the commit will just consist of the next line if the user % exits the editor directly: % % This reverts commit <--format=reference commit> % % But the comment char here is hardcoded (#). Which means that the % comment line will inadvertently be included in the commit message if % `core.commentChar`/`core.commentString` is in use. % % † 1: See 43966ab3156 (revert: optionally refer to commit in the % "reference" format, 2022-05-26) % % Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> % This is the commit message #3: sequencer: comment commit messages properly The rebase todo editor has commands like `fixup -c` which affects the commit messages of the rebased commits.[1] For example: pick hash1 <msg> fixup hash2 <msg> fixup -c hash3 <msg> This says that hash2` and hash3 should be squashed into hash1 and that hash3’s commit message should be used for the resulting commit. So the user is presented with an editor where the two first commit messages are commented out and the third is not. However this does not work if `core.commentChar`/`core.commentString` is in use since the comment char is hardcoded (#) in this `sequencer.c` function. As a result the first commit message will not be commented out. † 1: See 9e3cebd97cb (rebase -i: add fixup [-C | -c] command, 2021-01-29) Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk> Reported-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> % Please enter the commit message for your changes. Lines starting
Hi Kristoffer On 25/11/2024 10:52, Kristoffer Haugsbakk wrote: > On Mon, Nov 25, 2024, at 11:07, phillip.wood123@gmail.com wrote: > > Hi Phillip, thanks for the review! You're welcome, thanks for fixing this >>> + that hash3’s commit message should be used for the resulting commit. >>> + So the user is presented with an editor where the two first commit >>> + messages are commented out and the third is not. >> >> I'd perhaps say >> >> If there are conflicts when applying commit hash3 then the user is >> presented ... >> >> as we only show all the messages to the user when there are conflicts. > > I use `fixup -c` for the third/last commit here. So I am prompted to > edit the commit message. I tested this on this series: > > git checkout --detach kh/sequencer-comment-char > git rebase -i fd3785337beb285ed7fd67ce6fc3d3bed2097b40 > > Which gave me [this] editor without these changes (with > `core.commentChar` set to `%`). Oh, I see the same thing, I was sure we only showed the final message unless there were conflicts. I wonder if I've misremembered or the behavior has changed in any case that's outside the scope of this series. Thanks Phillip >> >>> However this does >>> + not work if `core.commentChar`/`core.commentString` is in use since >>> + the comment char is hardcoded (#) in this `sequencer.c` function. >>> + As a result the first commit message will not be commented out. >>> + >>> + † 1: See 9e3cebd97cb (rebase -i: add fixup [-C | -c] command, >>> + 2021-01-29) >>> + >>> + Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> >>> Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk> >>> + Reported-by: Taylor Blau <me@ttaylorr.com> >>> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> >> >> Thanks for updating the trailers, they look good to me >> >> Best Wishes >> >> Phillip > > † this: > > % This is a combination of 3 commits. > % This is the 1st commit message: > > sequencer: comment checked-out branch properly > > `git rebase --update-ref` does not insert commands for dependent/sub- > branches which are checked out.[1] Instead it leaves a comment about > that fact. The comment char is hardcoded (#). In turn the comment > line gets interpreted as an invalid command when `core.commentChar`/ > `core.commentString` is in use. > > † 1: See 900b50c242 (rebase: add --update-refs option, 2022-07-19) > > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> > > % The commit message #2 will be skipped: > > % sequencer: comment `--reference` subject line properly > % > % `git revert --reference <commit>` leaves behind a comment in the > % first line:[1] > % > % # *** SAY WHY WE ARE REVERTING ON THE TITLE LINE *** > % > % Meaning that the commit will just consist of the next line if the user > % exits the editor directly: > % > % This reverts commit <--format=reference commit> > % > % But the comment char here is hardcoded (#). Which means that the > % comment line will inadvertently be included in the commit message if > % `core.commentChar`/`core.commentString` is in use. > % > % † 1: See 43966ab3156 (revert: optionally refer to commit in the > % "reference" format, 2022-05-26) > % > % Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> > > % This is the commit message #3: > > sequencer: comment commit messages properly > > The rebase todo editor has commands like `fixup -c` which affects > the commit messages of the rebased commits.[1] For example: > > pick hash1 <msg> > fixup hash2 <msg> > fixup -c hash3 <msg> > > This says that hash2` and hash3 should be squashed into hash1 and > that hash3’s commit message should be used for the resulting commit. > So the user is presented with an editor where the two first commit > messages are commented out and the third is not. However this does > not work if `core.commentChar`/`core.commentString` is in use since > the comment char is hardcoded (#) in this `sequencer.c` function. > As a result the first commit message will not be commented out. > > † 1: See 9e3cebd97cb (rebase -i: add fixup [-C | -c] command, > 2021-01-29) > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk> > Reported-by: Taylor Blau <me@ttaylorr.com> > Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name> > > % Please enter the commit message for your changes. Lines starting
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index f61a717b190..711bd230695 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -469,7 +469,10 @@ test_expect_success 'git rebase --update-ref with core.commentChar and branch on git checkout base && test_commit msg3 && git checkout topic2 && - git -c core.commentChar=% rebase --update-refs base + GIT_SEQUENCE_EDITOR="cat >actual" git -c core.commentChar=% \ + rebase -i --update-refs base && + grep "% Ref refs/heads/wt-topic checked out at" actual && + grep "% Ref refs/heads/topic2 checked out at" actual ' test_done diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh index 26d3cabb608..43476236131 100755 --- a/t/t3501-revert-cherry-pick.sh +++ b/t/t3501-revert-cherry-pick.sh @@ -231,12 +231,14 @@ test_expect_success 'identification of reverted commit (--reference)' ' test_expect_success 'git revert --reference with core.commentChar' ' test_when_finished "git reset --hard to-ident" && git checkout --detach to-ident && - git -c core.commentChar=% revert \ + GIT_EDITOR="cat | head -4 >actual" git -c core.commentChar=% revert \ --edit --reference HEAD && - git log -1 --format=%B HEAD >actual && - printf "This reverts commit $(git show -s \ - --pretty=reference HEAD^).\n\n" \ - >expect && + cat <<-EOF >expect && + % *** SAY WHY WE ARE REVERTING ON THE TITLE LINE *** + + This reverts commit $(git show -s --pretty=reference HEAD^). + + EOF test_cmp expect actual '