* [PATCH v2 1/3] sequencer: comment checked-out branch properly
2024-11-12 10:20 ` [PATCH v2 0/3] sequencer: comment out properly in todo list kristofferhaugsbakk
@ 2024-11-12 10:20 ` kristofferhaugsbakk
2024-11-13 1:07 ` Junio C Hamano
2024-11-12 10:20 ` [PATCH v2 2/3] sequencer: comment `--reference` subject line properly kristofferhaugsbakk
` (3 subsequent siblings)
4 siblings, 1 reply; 42+ messages in thread
From: kristofferhaugsbakk @ 2024-11-12 10:20 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, stolee, phillip.wood123, me
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
`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`
is in use.
† 1: 900b50c242 (rebase: add --update-refs option, 2022-07-19)
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v2:
• Message: “hardcoded” (more common according to `git grep`)
sequencer.c | 5 +++--
t/t3400-rebase.sh | 16 ++++++++++++++++
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 353d804999b..1b6fd86f70b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6382,8 +6382,9 @@ static int add_decorations_to_list(const struct commit *commit,
/* If the branch is checked out, then leave a comment instead. */
if ((path = branch_checked_out(decoration->name))) {
item->command = TODO_COMMENT;
- strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
- decoration->name, path);
+ strbuf_commented_addf(ctx->buf, comment_line_str,
+ "Ref %s checked out at '%s'\n",
+ decoration->name, path);
} else {
struct string_list_item *sti;
item->command = TODO_UPDATE_REF;
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 09f230eefb2..f61a717b190 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -456,4 +456,20 @@ test_expect_success 'rebase when inside worktree subdirectory' '
)
'
+test_expect_success 'git rebase --update-ref with core.commentChar and branch on worktree' '
+ test_when_finished git branch -D base topic2 &&
+ test_when_finished git checkout main &&
+ test_when_finished git branch -D wt-topic &&
+ test_when_finished git worktree remove wt-topic &&
+ git checkout main &&
+ git checkout -b base &&
+ git checkout -b topic2 &&
+ test_commit msg2 &&
+ git worktree add wt-topic &&
+ git checkout base &&
+ test_commit msg3 &&
+ git checkout topic2 &&
+ git -c core.commentChar=% rebase --update-refs base
+'
+
test_done
--
2.47.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v2 1/3] sequencer: comment checked-out branch properly
2024-11-12 10:20 ` [PATCH v2 1/3] sequencer: comment checked-out branch properly kristofferhaugsbakk
@ 2024-11-13 1:07 ` Junio C Hamano
2024-11-13 1:18 ` Junio C Hamano
2024-11-13 14:47 ` phillip.wood123
0 siblings, 2 replies; 42+ messages in thread
From: Junio C Hamano @ 2024-11-13 1:07 UTC (permalink / raw)
To: kristofferhaugsbakk
Cc: git, Kristoffer Haugsbakk, stolee, phillip.wood123, me
kristofferhaugsbakk@fastmail.com writes:
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 09f230eefb2..f61a717b190 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -456,4 +456,20 @@ test_expect_success 'rebase when inside worktree subdirectory' '
> )
> '
>
> +test_expect_success 'git rebase --update-ref with core.commentChar and branch on worktree' '
> + test_when_finished git branch -D base topic2 &&
> + test_when_finished git checkout main &&
> + test_when_finished git branch -D wt-topic &&
> + test_when_finished git worktree remove wt-topic &&
> + git checkout main &&
> + git checkout -b base &&
> + git checkout -b topic2 &&
> + test_commit msg2 &&
> + git worktree add wt-topic &&
> + git checkout base &&
> + test_commit msg3 &&
> + git checkout topic2 &&
> + git -c core.commentChar=% rebase --update-refs base
> +'
Can we improve this test a bit to give it more visibility into the
breakage?
I am sure that the internal machinery gets confused because it wants
to skip commented out lines assuming '%' is used for comments, and
fails to skip lines that are commented with '#', but it is a bit too
opaque how this would break without the fix. Perhaps add a line or
two of a comment to the test to describe what the expected mode of
failure is?
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/3] sequencer: comment checked-out branch properly
2024-11-13 1:07 ` Junio C Hamano
@ 2024-11-13 1:18 ` Junio C Hamano
2024-11-13 14:47 ` phillip.wood123
1 sibling, 0 replies; 42+ messages in thread
From: Junio C Hamano @ 2024-11-13 1:18 UTC (permalink / raw)
To: kristofferhaugsbakk
Cc: git, Kristoffer Haugsbakk, stolee, phillip.wood123, me
Junio C Hamano <gitster@pobox.com> writes:
> kristofferhaugsbakk@fastmail.com writes:
>
>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>> index 09f230eefb2..f61a717b190 100755
>> --- a/t/t3400-rebase.sh
>> +++ b/t/t3400-rebase.sh
>> @@ -456,4 +456,20 @@ test_expect_success 'rebase when inside worktree subdirectory' '
>> )
>> '
>>
>> +test_expect_success 'git rebase --update-ref with core.commentChar and branch on worktree' '
>> + test_when_finished git branch -D base topic2 &&
>> + test_when_finished git checkout main &&
>> + test_when_finished git branch -D wt-topic &&
>> + test_when_finished git worktree remove wt-topic &&
>> + git checkout main &&
>> + git checkout -b base &&
>> + git checkout -b topic2 &&
>> + test_commit msg2 &&
>> + git worktree add wt-topic &&
>> + git checkout base &&
>> + test_commit msg3 &&
>> + git checkout topic2 &&
>> + git -c core.commentChar=% rebase --update-refs base
>> +'
>
> Can we improve this test a bit to give it more visibility into the
> breakage?
>
> I am sure that the internal machinery gets confused because it wants
> to skip commented out lines assuming '%' is used for comments, and
> fails to skip lines that are commented with '#', but it is a bit too
> opaque how this would break without the fix. Perhaps add a line or
> two of a comment to the test to describe what the expected mode of
> failure is?
Something like "a line commented-out with '#' in the list of
instructions used internally by "rebase" is not recognised as a
comment and you'd get an error that says '#' is not a valid
instruction", perhaps.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/3] sequencer: comment checked-out branch properly
2024-11-13 1:07 ` Junio C Hamano
2024-11-13 1:18 ` Junio C Hamano
@ 2024-11-13 14:47 ` phillip.wood123
2024-11-13 22:57 ` Junio C Hamano
2024-11-24 20:02 ` Kristoffer Haugsbakk
1 sibling, 2 replies; 42+ messages in thread
From: phillip.wood123 @ 2024-11-13 14:47 UTC (permalink / raw)
To: Junio C Hamano, kristofferhaugsbakk; +Cc: git, Kristoffer Haugsbakk, stolee, me
On 13/11/2024 01:07, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
>
>> +test_expect_success 'git rebase --update-ref with core.commentChar and branch on worktree' '
>> + test_when_finished git branch -D base topic2 &&
>> + test_when_finished git checkout main &&
>> + test_when_finished git branch -D wt-topic &&
>> + test_when_finished git worktree remove wt-topic &&
>> + git checkout main &&
>> + git checkout -b base &&
>> + git checkout -b topic2 &&
>> + test_commit msg2 &&
>> + git worktree add wt-topic &&
>> + git checkout base &&
>> + test_commit msg3 &&
>> + git checkout topic2 &&
>> + git -c core.commentChar=% rebase --update-refs base
>> +'
>
> Can we improve this test a bit to give it more visibility into the
> breakage?
>
> I am sure that the internal machinery gets confused because it wants
> to skip commented out lines assuming '%' is used for comments, and
> fails to skip lines that are commented with '#', but it is a bit too
> opaque how this would break without the fix. Perhaps add a line or
> two of a comment to the test to describe what the expected mode of
> failure is?
Or check the todo list shown to the user with
GIT_SEQUENCE_EDITOR="cat >actual" git -c core.commentChar=% \
rebase -i --update-refs base &&
test_grep "% Ref refs/heads/wt-topic checked out at " actual
so that we are sure that line exists - I had a quick look and I can't
see any existing coverage checking that the todo list contains this comment.
Best Wishes
Phillip
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/3] sequencer: comment checked-out branch properly
2024-11-13 14:47 ` phillip.wood123
@ 2024-11-13 22:57 ` Junio C Hamano
2024-11-24 20:02 ` Kristoffer Haugsbakk
1 sibling, 0 replies; 42+ messages in thread
From: Junio C Hamano @ 2024-11-13 22:57 UTC (permalink / raw)
To: phillip.wood123
Cc: kristofferhaugsbakk, git, Kristoffer Haugsbakk, stolee, me
phillip.wood123@gmail.com writes:
>>> ...
>>> + git checkout topic2 &&
>>> + git -c core.commentChar=% rebase --update-refs base
>>> +'
>> Can we improve this test a bit to give it more visibility into the
>> breakage?
>> I am sure that the internal machinery gets confused because it wants
>> to skip commented out lines assuming '%' is used for comments, and
>> fails to skip lines that are commented with '#', but it is a bit too
>> opaque how this would break without the fix. Perhaps add a line or
>> two of a comment to the test to describe what the expected mode of
>> failure is?
>
> Or check the todo list shown to the user with
>
> GIT_SEQUENCE_EDITOR="cat >actual" git -c core.commentChar=% \
> rebase -i --update-refs base &&
> test_grep "% Ref refs/heads/wt-topic checked out at " actual
>
> so that we are sure that line exists - I had a quick look and I can't
> see any existing coverage checking that the todo list contains this
> comment.
Yeah, with "rebase -i", it is a part of end-user experience to see
these comments, and a check to make sure they are shown to guide the
user is certainly a good idea.
A test of "rebase" without "-i" is a different matter. Maybe in the
future, while we still reuse the machinery used for the interactive
mode to implement the non-interactive rebase, we may tweak it so
that we do not generate commented lines, only to be skipped by the
parser, in the todo list file that is only used internally.
Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 1/3] sequencer: comment checked-out branch properly
2024-11-13 14:47 ` phillip.wood123
2024-11-13 22:57 ` Junio C Hamano
@ 2024-11-24 20:02 ` Kristoffer Haugsbakk
1 sibling, 0 replies; 42+ messages in thread
From: Kristoffer Haugsbakk @ 2024-11-24 20:02 UTC (permalink / raw)
To: Phillip Wood, Junio C Hamano
Cc: git, Kristoffer Haugsbakk, Derrick Stolee, Taylor Blau
On Wed, Nov 13, 2024, at 15:47, phillip.wood123@gmail.com wrote:
> Or check the todo list shown to the user with
>
> GIT_SEQUENCE_EDITOR="cat >actual" git -c core.commentChar=% \
> rebase -i --update-refs base &&
> test_grep "% Ref refs/heads/wt-topic checked out at " actual
>
> so that we are sure that line exists - I had a quick look and I can't
> see any existing coverage checking that the todo list contains this comment.
Thanks!
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 2/3] sequencer: comment `--reference` subject line properly
2024-11-12 10:20 ` [PATCH v2 0/3] sequencer: comment out properly in todo list kristofferhaugsbakk
2024-11-12 10:20 ` [PATCH v2 1/3] sequencer: comment checked-out branch properly kristofferhaugsbakk
@ 2024-11-12 10:20 ` kristofferhaugsbakk
2024-11-13 1:07 ` Junio C Hamano
2024-11-12 10:20 ` [PATCH v2 3/3] sequencer: comment commit messages properly kristofferhaugsbakk
` (2 subsequent siblings)
4 siblings, 1 reply; 42+ messages in thread
From: kristofferhaugsbakk @ 2024-11-12 10:20 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, stolee, phillip.wood123, me
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
Comment the subject line used in `git cherry-pick --reference`
properly.
Follow the existing pattern and use the case described in the original
commit message[1] as the `core.commentChar` test case:
If the user exits the editor without touching this line by mistake,
what we prepare to become the first line of the body, i.e. "This
reverts commit 8fa7f667 (do this and that, 2022-04-25)", ends up to
be the title of the resulting commit.
† 1: 43966ab3156 (revert: optionally refer to commit in the "reference"
format, 2022-05-26)
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v2:
• `strbuf_commented_addf` adds a newline, unlike the previous function.
We need to remove a newline from the final `strbuf_addstr` with `This
reverts commits` and add a newline to each of the other
branches (`else if` and `else`).
sequencer.c | 9 +++++----
t/t3501-revert-cherry-pick.sh | 12 ++++++++++++
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 1b6fd86f70b..d26299cdea2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2341,8 +2341,8 @@ static int do_pick_commit(struct repository *r,
next = parent;
next_label = msg.parent_label;
if (opts->commit_use_reference) {
- strbuf_addstr(&ctx->message,
- "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
+ strbuf_commented_addf(&ctx->message, comment_line_str,
+ "*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
/*
* We don't touch pre-existing repeated reverts, because
@@ -2352,12 +2352,13 @@ static int do_pick_commit(struct repository *r,
!starts_with(orig_subject, "Revert \"")) {
strbuf_addstr(&ctx->message, "Reapply \"");
strbuf_addstr(&ctx->message, orig_subject);
+ strbuf_addstr(&ctx->message, "\n");
} else {
strbuf_addstr(&ctx->message, "Revert \"");
strbuf_addstr(&ctx->message, msg.subject);
- strbuf_addstr(&ctx->message, "\"");
+ strbuf_addstr(&ctx->message, "\"\n");
}
- strbuf_addstr(&ctx->message, "\n\nThis reverts commit ");
+ strbuf_addstr(&ctx->message, "\nThis reverts commit ");
refer_to_commit(opts, &ctx->message, commit);
if (commit->parents && commit->parents->next) {
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 411027fb58c..26d3cabb608 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -228,6 +228,18 @@ test_expect_success 'identification of reverted commit (--reference)' '
test_cmp expect actual
'
+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 \
+ --edit --reference HEAD &&
+ git log -1 --format=%B HEAD >actual &&
+ printf "This reverts commit $(git show -s \
+ --pretty=reference HEAD^).\n\n" \
+ >expect &&
+ test_cmp expect actual
+'
+
test_expect_success 'identification of reverted commit (revert.reference)' '
git checkout --detach to-ident &&
git -c revert.reference=true revert --no-edit HEAD &&
--
2.47.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v2 2/3] sequencer: comment `--reference` subject line properly
2024-11-12 10:20 ` [PATCH v2 2/3] sequencer: comment `--reference` subject line properly kristofferhaugsbakk
@ 2024-11-13 1:07 ` Junio C Hamano
2024-11-13 14:48 ` phillip.wood123
0 siblings, 1 reply; 42+ messages in thread
From: Junio C Hamano @ 2024-11-13 1:07 UTC (permalink / raw)
To: kristofferhaugsbakk
Cc: git, Kristoffer Haugsbakk, stolee, phillip.wood123, me
kristofferhaugsbakk@fastmail.com writes:
> @@ -2341,8 +2341,8 @@ static int do_pick_commit(struct repository *r,
> next = parent;
> next_label = msg.parent_label;
> if (opts->commit_use_reference) {
> - strbuf_addstr(&ctx->message,
> - "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
> + strbuf_commented_addf(&ctx->message, comment_line_str,
> + "*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
With the switch to "commented_addf", we'd terminate this line with
LF here, which means ...
> @@ -2352,12 +2352,13 @@ static int do_pick_commit(struct repository *r,
> !starts_with(orig_subject, "Revert \"")) {
> strbuf_addstr(&ctx->message, "Reapply \"");
> strbuf_addstr(&ctx->message, orig_subject);
> + strbuf_addstr(&ctx->message, "\n");
> } else {
> strbuf_addstr(&ctx->message, "Revert \"");
> strbuf_addstr(&ctx->message, msg.subject);
> - strbuf_addstr(&ctx->message, "\"");
> + strbuf_addstr(&ctx->message, "\"\n");
... we'd want to terminate the line in these two other if/else if/else
arms for symmetry, so that ...
> }
> - strbuf_addstr(&ctx->message, "\n\nThis reverts commit ");
> + strbuf_addstr(&ctx->message, "\nThis reverts commit ");
... we can lose the termination of the previous line from here.
Makes sense.
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 411027fb58c..26d3cabb608 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -228,6 +228,18 @@ test_expect_success 'identification of reverted commit (--reference)' '
> test_cmp expect actual
> '
>
> +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 \
> + --edit --reference HEAD &&
> + git log -1 --format=%B HEAD >actual &&
> + printf "This reverts commit $(git show -s \
> + --pretty=reference HEAD^).\n\n" \
> + >expect &&
> + test_cmp expect actual
> +'
I guess this fails by leaving the "# *** SAY WHY" in the resulting
message, because the stripspace wants to see '%' to start commented
out lines to be stripped? If we inspect with this test what the
temporary file we give to the editor looks like to make sure that
'%' is used for commenting, that would be a more direct test, but
without going that far, at least can we have a comment describing
how this is expected to fail without the fix?
Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 2/3] sequencer: comment `--reference` subject line properly
2024-11-13 1:07 ` Junio C Hamano
@ 2024-11-13 14:48 ` phillip.wood123
2024-11-13 23:00 ` Junio C Hamano
0 siblings, 1 reply; 42+ messages in thread
From: phillip.wood123 @ 2024-11-13 14:48 UTC (permalink / raw)
To: Junio C Hamano, kristofferhaugsbakk; +Cc: git, Kristoffer Haugsbakk, stolee, me
On 13/11/2024 01:07, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
>
>> +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 \
>> + --edit --reference HEAD &&
>> + git log -1 --format=%B HEAD >actual &&
>> + printf "This reverts commit $(git show -s \
>> + --pretty=reference HEAD^).\n\n" \
>> + >expect &&
>> + test_cmp expect actual
>> +'
>
> I guess this fails by leaving the "# *** SAY WHY" in the resulting
> message, because the stripspace wants to see '%' to start commented
> out lines to be stripped? If we inspect with this test what the
> temporary file we give to the editor looks like to make sure that
> '%' is used for commenting, that would be a more direct test, but
> without going that far, at least can we have a comment describing
> how this is expected to fail without the fix?
For me something like
GIT_EDITOR="cat >actual" git -c core.commentChar=% revert \
--edit --reference HEAD &&
test_grep "^% \*\*\* SAY WHY WE ARE REVERTING THE COMMIT \*\*\*" \
actual
Would be a more convincing test as it actually checks that the user sees
the line that we expect strbuf_stripspace() to remove from the final
message. If we want to check the commit message as well that's fine but
I'm not sure its necessary. (if we do we should use test_commit_message
like patch 3)
Best Wishes
Phillip
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 2/3] sequencer: comment `--reference` subject line properly
2024-11-13 14:48 ` phillip.wood123
@ 2024-11-13 23:00 ` Junio C Hamano
0 siblings, 0 replies; 42+ messages in thread
From: Junio C Hamano @ 2024-11-13 23:00 UTC (permalink / raw)
To: phillip.wood123
Cc: kristofferhaugsbakk, git, Kristoffer Haugsbakk, stolee, me
phillip.wood123@gmail.com writes:
>> I guess this fails by leaving the "# *** SAY WHY" in the resulting
>> message, because the stripspace wants to see '%' to start commented
>> out lines to be stripped? If we inspect with this test what the
>> temporary file we give to the editor looks like to make sure that
>> '%' is used for commenting, that would be a more direct test, but
>> without going that far, at least can we have a comment describing
>> how this is expected to fail without the fix?
>
> For me something like
>
> GIT_EDITOR="cat >actual" git -c core.commentChar=% revert \
> --edit --reference HEAD &&
> test_grep "^% \*\*\* SAY WHY WE ARE REVERTING THE COMMIT \*\*\*" \
> actual
>
> Would be a more convincing test as it actually checks that the user sees
> the line that we expect strbuf_stripspace() to remove from the final
> message.
Yes, it is a more direct test. If we did that, that would of course
be preferrable.
Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v2 3/3] sequencer: comment commit messages properly
2024-11-12 10:20 ` [PATCH v2 0/3] sequencer: comment out properly in todo list kristofferhaugsbakk
2024-11-12 10:20 ` [PATCH v2 1/3] sequencer: comment checked-out branch properly kristofferhaugsbakk
2024-11-12 10:20 ` [PATCH v2 2/3] sequencer: comment `--reference` subject line properly kristofferhaugsbakk
@ 2024-11-12 10:20 ` kristofferhaugsbakk
2024-11-13 1:03 ` Junio C Hamano
2024-11-13 0:26 ` [PATCH v2 0/3] sequencer: comment out properly in todo list Junio C Hamano
2024-11-24 20:56 ` [PATCH v3 " kristofferhaugsbakk
4 siblings, 1 reply; 42+ messages in thread
From: kristofferhaugsbakk @ 2024-11-12 10:20 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, stolee, phillip.wood123, me, Phillip Wood
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
Notes (series):
v2
• Phillip contributed the test and the `strbuf_setlen` changes
Notes (meta-trailers):
Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Link: https://lore.kernel.org/git/cfa466b8-a87d-4b5d-b330-6c660897de48@gmail.com/#t
sequencer.c | 12 ++++++++----
t/t3437-rebase-fixup-options.sh | 15 +++++++++++++++
2 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index d26299cdea2..42a6f257cbb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1941,10 +1941,10 @@ static int seen_squash(struct replay_ctx *ctx)
static void update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n)
{
- strbuf_setlen(buf1, 2);
+ strbuf_setlen(buf1, strlen(comment_line_str) + 1);
strbuf_addf(buf1, _(nth_commit_msg_fmt), n);
strbuf_addch(buf1, '\n');
- strbuf_setlen(buf2, 2);
+ strbuf_setlen(buf2, strlen(comment_line_str) + 1);
strbuf_addf(buf2, _(skip_nth_commit_msg_fmt), n);
strbuf_addch(buf2, '\n');
}
@@ -1963,8 +1963,12 @@ static void update_squash_message_for_fixup(struct strbuf *msg)
size_t orig_msg_len;
int i = 1;
- strbuf_addf(&buf1, "# %s\n", _(first_commit_msg_str));
- strbuf_addf(&buf2, "# %s\n", _(skip_first_commit_msg_str));
+ strbuf_add_commented_lines(&buf1, _(first_commit_msg_str),
+ strlen(_(first_commit_msg_str)),
+ comment_line_str);
+ strbuf_add_commented_lines(&buf2, _(skip_first_commit_msg_str),
+ strlen(_(skip_first_commit_msg_str)),
+ comment_line_str);
s = start = orig_msg = strbuf_detach(msg, &orig_msg_len);
while (s) {
const char *next;
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 7929e2e2e3a..a4b90e881e3 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -127,6 +127,21 @@ test_expect_success 'fixup -C with conflicts gives correct message' '
test_cmp expected-author actual-author
'
+test_expect_success 'conflicting fixup -C after fixup with custom comment string' '
+ test_config core.commentString COMMENT &&
+ test_when_finished "test_might_fail git rebase --abort" &&
+ git checkout --detach A3 &&
+ test_must_fail env FAKE_LINES="1 fixup 2 fixup_-C 4" git rebase -i A &&
+ echo resolved >A &&
+ git add A &&
+ FAKE_COMMIT_AMEND=edited git rebase --continue &&
+ test_commit_message HEAD <<-\EOF
+ A3
+
+ edited
+ EOF
+'
+
test_expect_success 'skipping fixup -C after fixup gives correct message' '
test_when_finished "test_might_fail git rebase --abort" &&
git checkout --detach A3 &&
--
2.47.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v2 3/3] sequencer: comment commit messages properly
2024-11-12 10:20 ` [PATCH v2 3/3] sequencer: comment commit messages properly kristofferhaugsbakk
@ 2024-11-13 1:03 ` Junio C Hamano
2024-11-13 14:49 ` phillip.wood123
0 siblings, 1 reply; 42+ messages in thread
From: Junio C Hamano @ 2024-11-13 1:03 UTC (permalink / raw)
To: kristofferhaugsbakk
Cc: git, Kristoffer Haugsbakk, stolee, phillip.wood123, me,
Phillip Wood
kristofferhaugsbakk@fastmail.com writes:
> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
Describe what happens when a custom comment string is used without
the fixed code in this space.
> Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
> +test_expect_success 'conflicting fixup -C after fixup with custom comment string' '
> + test_config core.commentString COMMENT &&
> + test_when_finished "test_might_fail git rebase --abort" &&
> + git checkout --detach A3 &&
> + test_must_fail env FAKE_LINES="1 fixup 2 fixup_-C 4" git rebase -i A &&
> + echo resolved >A &&
> + git add A &&
> + FAKE_COMMIT_AMEND=edited git rebase --continue &&
> + test_commit_message HEAD <<-\EOF
> + A3
> +
> + edited
> + EOF
> +'
Doing so would allow readers to imagine more easily how this test
would catch breakages when the code is not fixed (or broken again).
Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 3/3] sequencer: comment commit messages properly
2024-11-13 1:03 ` Junio C Hamano
@ 2024-11-13 14:49 ` phillip.wood123
2024-11-24 19:58 ` Kristoffer Haugsbakk
0 siblings, 1 reply; 42+ messages in thread
From: phillip.wood123 @ 2024-11-13 14:49 UTC (permalink / raw)
To: Junio C Hamano, kristofferhaugsbakk
Cc: git, Kristoffer Haugsbakk, stolee, me, Phillip Wood
On 13/11/2024 01:03, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
>
>> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>>
>
> Describe what happens when a custom comment string is used without
> the fixed code in this space.
It would also be helpful to explain how to trigger the bug [1]
If I remember correctly it was Taylor who first noticed this [2]. If so
we should credit him with a "Reported-by:" trailer.
>> Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
It seems odd to me to have a "Co-authored-by:" trailer without a
corresponding "Signed-off-by:" If someone has contributed enough to
deserve "Co-authored-by:" then they should be signing off the code they
have contributed. In this case I'd be happy with "Helped-by:" instead
but feel free to add my "Signed-off-by:" if you want to keep
"Co-authored-by:".
Best Wishes
Phillip
[1]
https://lore.kernel.org/git/cfa466b8-a87d-4b5d-b330-6c660897de48@gmail.com/
[2]https://lore.kernel.org/git/ZxlEJ+44M8z03VOj@nand.local/
>> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
>> ---
>
>> +test_expect_success 'conflicting fixup -C after fixup with custom comment string' '
>> + test_config core.commentString COMMENT &&
>> + test_when_finished "test_might_fail git rebase --abort" &&
>> + git checkout --detach A3 &&
>> + test_must_fail env FAKE_LINES="1 fixup 2 fixup_-C 4" git rebase -i A &&
>> + echo resolved >A &&
>> + git add A &&
>> + FAKE_COMMIT_AMEND=edited git rebase --continue &&
>> + test_commit_message HEAD <<-\EOF
>> + A3
>> +
>> + edited
>> + EOF
>> +'
>
> Doing so would allow readers to imagine more easily how this test
> would catch breakages when the code is not fixed (or broken again).
>
> Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 3/3] sequencer: comment commit messages properly
2024-11-13 14:49 ` phillip.wood123
@ 2024-11-24 19:58 ` Kristoffer Haugsbakk
0 siblings, 0 replies; 42+ messages in thread
From: Kristoffer Haugsbakk @ 2024-11-24 19:58 UTC (permalink / raw)
To: Phillip Wood, Junio C Hamano, Kristoffer Haugsbakk
Cc: git, Derrick Stolee, Taylor Blau
On Wed, Nov 13, 2024, at 15:49, phillip.wood123@gmail.com wrote:
> On 13/11/2024 01:03, Junio C Hamano wrote:
>> kristofferhaugsbakk@fastmail.com writes:
>>
>>> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>>>
>>
>> Describe what happens when a custom comment string is used without
>> the fixed code in this space.
>
> It would also be helpful to explain how to trigger the bug [1]
>
> If I remember correctly it was Taylor who first noticed this [2]. If so
> we should credit him with a "Reported-by:" trailer.
>
>>> Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> It seems odd to me to have a "Co-authored-by:" trailer without a
> corresponding "Signed-off-by:" If someone has contributed enough to
> deserve "Co-authored-by:" then they should be signing off the code they
> have contributed. In this case I'd be happy with "Helped-by:" instead
> but feel free to add my "Signed-off-by:" if you want to keep
> "Co-authored-by:".
I’ll fix that in the next version. Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v2 0/3] sequencer: comment out properly in todo list
2024-11-12 10:20 ` [PATCH v2 0/3] sequencer: comment out properly in todo list kristofferhaugsbakk
` (2 preceding siblings ...)
2024-11-12 10:20 ` [PATCH v2 3/3] sequencer: comment commit messages properly kristofferhaugsbakk
@ 2024-11-13 0:26 ` Junio C Hamano
2024-11-24 20:01 ` Kristoffer Haugsbakk
2024-11-24 20:56 ` [PATCH v3 " kristofferhaugsbakk
4 siblings, 1 reply; 42+ messages in thread
From: Junio C Hamano @ 2024-11-13 0:26 UTC (permalink / raw)
To: kristofferhaugsbakk
Cc: git, Kristoffer Haugsbakk, stolee, phillip.wood123, me
kristofferhaugsbakk@fastmail.com writes:
> The first version just had patch 1 but this one fixes two other places.
> The two other places where unearthered during the v1 discussion.
OK. I guess they could be handled in a single patch, but the three
patches address different things to be commented properly, so having
them as three separate patches is good.
> Rebased on `master` (b31fb630c0 (Merge https://github.com/j6t/git-gui,
> 2024-11-11)).
Was there any reason, other than "newer must be better" (which is
not always true)? I thought there isn't any in-flight topics that
touched the sequencer machinery.
> Some failures that didn’t look relevant.
Judging from https://github.com/git/git/actions/runs/11774751134
(which you rebased the topic on) passing all these CI tests,
if your topic saw CI breakages, there is nothing else we can put the
blame on.
But let's see what happens. I won't see CI failures on individual
topic, but we will see what the topic, together with everybody else,
in the context of 'seen', produces when I push today's integration
result out.
Thanks.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v2 0/3] sequencer: comment out properly in todo list
2024-11-13 0:26 ` [PATCH v2 0/3] sequencer: comment out properly in todo list Junio C Hamano
@ 2024-11-24 20:01 ` Kristoffer Haugsbakk
0 siblings, 0 replies; 42+ messages in thread
From: Kristoffer Haugsbakk @ 2024-11-24 20:01 UTC (permalink / raw)
To: Junio C Hamano, Kristoffer Haugsbakk
Cc: git, Derrick Stolee, Phillip Wood, Taylor Blau
On Wed, Nov 13, 2024, at 01:26, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
>
>> The first version just had patch 1 but this one fixes two other places.
>> The two other places where unearthered during the v1 discussion.
>
> OK. I guess they could be handled in a single patch, but the three
> patches address different things to be commented properly, so having
> them as three separate patches is good.
>
>> Rebased on `master` (b31fb630c0 (Merge https://github.com/j6t/git-gui,
>> 2024-11-11)).
>
> Was there any reason, other than "newer must be better" (which is
> not always true)? I thought there isn't any in-flight topics that
> touched the sequencer machinery.
I’m sorry. I worked on the v2 by rebasing on `jc/strbuf-commented-something`
which was in `seen` at the time. Then when I had to `rebase --onto` back
someplace I picked `master` since I didn’t need the interdiff for the cover
letter (two new patches, no changes to the first one).
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v3 0/3] sequencer: comment out properly in todo list
2024-11-12 10:20 ` [PATCH v2 0/3] sequencer: comment out properly in todo list kristofferhaugsbakk
` (3 preceding siblings ...)
2024-11-13 0:26 ` [PATCH v2 0/3] sequencer: comment out properly in todo list Junio C Hamano
@ 2024-11-24 20:56 ` kristofferhaugsbakk
2024-11-24 20:56 ` [PATCH v3 1/3] sequencer: comment checked-out branch properly kristofferhaugsbakk
` (4 more replies)
4 siblings, 5 replies; 42+ messages in thread
From: kristofferhaugsbakk @ 2024-11-24 20:56 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, stolee, phillip.wood123, me
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
Fix three places where the comment char/string is hardcoded (#) in the
todo list.
§ Changes in v3
I expanded on patch/commit message 3/3. I also checked the state (such as
the proposed commit message for the revert) in the tests. Both from
reviewer feedback.
See the rest of the changes as notes on the patches.
§ CC
• Stolee for the first patch
• Reviewers on the previous rounds
Kristoffer Haugsbakk (3):
sequencer: comment checked-out branch properly
sequencer: comment `--reference` subject line properly
sequencer: comment commit messages properly
sequencer.c | 26 ++++++++++++++++----------
t/t3400-rebase.sh | 19 +++++++++++++++++++
t/t3437-rebase-fixup-options.sh | 15 +++++++++++++++
t/t3501-revert-cherry-pick.sh | 14 ++++++++++++++
4 files changed, 64 insertions(+), 10 deletions(-)
Interdiff against v2:
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
'
Range-diff against v2:
1: fc3b4438845 ! 1: a46767263f6 sequencer: comment checked-out branch properly
@@ Commit message
`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`
- is in use.
+ line gets interpreted as an invalid command when `core.commentChar`/
+ `core.commentString` is in use.
- † 1: 900b50c242 (rebase: add --update-refs option, 2022-07-19)
+ † 1: See 900b50c242 (rebase: add --update-refs option, 2022-07-19)
Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
## Notes (series) ##
+ v3:
+ • Review feedback: check more in the test by inspecting the
+ sequence editor
+
+ Link: https://lore.kernel.org/git/5ed77fab-678d-4a06-bbd0-ea25462a7562@gmail.com/
+ • Message: consistency with the other two messages:
+ • Mention both commentChar and commentString
+ • Commit footnote style: See <commit>
v2:
• Message: “hardcoded” (more common according to `git grep`)
@@ 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
+'
+
test_done
2: 710c5b1a3f6 ! 2: 7a452142666 sequencer: comment `--reference` subject line properly
@@ Metadata
## Commit message ##
sequencer: comment `--reference` subject line properly
- Comment the subject line used in `git cherry-pick --reference`
- properly.
+ `git revert --reference <commit>` leaves behind a comment in the
+ first line:[1]
- Follow the existing pattern and use the case described in the original
- commit message[1] as the `core.commentChar` test case:
+ # *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***
- If the user exits the editor without touching this line by mistake,
- what we prepare to become the first line of the body, i.e. "This
- reverts commit 8fa7f667 (do this and that, 2022-04-25)", ends up to
- be the title of the resulting commit.
+ Meaning that the commit will just consist of the next line if the user
+ exits the editor directly:
- † 1: 43966ab3156 (revert: optionally refer to commit in the "reference"
- format, 2022-05-26)
+ 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>
## Notes (series) ##
+ v3:
+ • Review feedback: check more in the test by inspecting the
+ proposed commit message.
+
+ Link: https://lore.kernel.org/git/4c623fcf-01dd-4056-80c1-b3c860ab7f87@gmail.com/
+ • Message:
+ • Rewrite message now that we are testing something different
+ • consistency with the other two messages (see previous)
v2:
• `strbuf_commented_addf` adds a newline, unlike the previous function.
We need to remove a newline from the final `strbuf_addstr` with `This
@@ 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 &&
-+ 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
+'
+
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
+ 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>
## Notes (series) ##
- v2
+ v3:
+ • Message: Explain to the best of my knowledge what is going on here in
+ the message body
+
+ Link: https://lore.kernel.org/git/711b59d7-e649-4031-8924-a16fb632b4d4@gmail.com/
+ • Fixed wrong/subpar use of trailers
+
+ Link: https://lore.kernel.org/git/711b59d7-e649-4031-8924-a16fb632b4d4@gmail.com/
+ v2:
• Phillip contributed the test and the `strbuf_setlen` changes
base-commit: b31fb630c0fc6869a33ed717163e8a1210460d94
--
2.47.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v3 1/3] sequencer: comment checked-out branch properly
2024-11-24 20:56 ` [PATCH v3 " kristofferhaugsbakk
@ 2024-11-24 20:56 ` kristofferhaugsbakk
2024-11-24 20:56 ` [PATCH v3 2/3] sequencer: comment `--reference` subject line properly kristofferhaugsbakk
` (3 subsequent siblings)
4 siblings, 0 replies; 42+ messages in thread
From: kristofferhaugsbakk @ 2024-11-24 20:56 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, stolee, phillip.wood123, me
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
`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>
---
Notes (series):
v3:
• Review feedback: check more in the test by inspecting the
sequence editor
Link: https://lore.kernel.org/git/5ed77fab-678d-4a06-bbd0-ea25462a7562@gmail.com/
• Message: consistency with the other two messages:
• Mention both commentChar and commentString
• Commit footnote style: See <commit>
v2:
• Message: “hardcoded” (more common according to `git grep`)
sequencer.c | 5 +++--
t/t3400-rebase.sh | 19 +++++++++++++++++++
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 353d804999b..1b6fd86f70b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6382,8 +6382,9 @@ static int add_decorations_to_list(const struct commit *commit,
/* If the branch is checked out, then leave a comment instead. */
if ((path = branch_checked_out(decoration->name))) {
item->command = TODO_COMMENT;
- strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
- decoration->name, path);
+ strbuf_commented_addf(ctx->buf, comment_line_str,
+ "Ref %s checked out at '%s'\n",
+ decoration->name, path);
} else {
struct string_list_item *sti;
item->command = TODO_UPDATE_REF;
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 09f230eefb2..711bd230695 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -456,4 +456,23 @@ test_expect_success 'rebase when inside worktree subdirectory' '
)
'
+test_expect_success 'git rebase --update-ref with core.commentChar and branch on worktree' '
+ test_when_finished git branch -D base topic2 &&
+ test_when_finished git checkout main &&
+ test_when_finished git branch -D wt-topic &&
+ test_when_finished git worktree remove wt-topic &&
+ git checkout main &&
+ git checkout -b base &&
+ git checkout -b topic2 &&
+ test_commit msg2 &&
+ git worktree add wt-topic &&
+ git checkout base &&
+ test_commit msg3 &&
+ git checkout topic2 &&
+ 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
--
2.47.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v3 2/3] sequencer: comment `--reference` subject line properly
2024-11-24 20:56 ` [PATCH v3 " kristofferhaugsbakk
2024-11-24 20:56 ` [PATCH v3 1/3] sequencer: comment checked-out branch properly kristofferhaugsbakk
@ 2024-11-24 20:56 ` kristofferhaugsbakk
2024-11-24 20:56 ` [PATCH v3 3/3] sequencer: comment commit messages properly kristofferhaugsbakk
` (2 subsequent siblings)
4 siblings, 0 replies; 42+ messages in thread
From: kristofferhaugsbakk @ 2024-11-24 20:56 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, stolee, phillip.wood123, me
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
`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>
---
Notes (series):
v3:
• Review feedback: check more in the test by inspecting the
proposed commit message.
Link: https://lore.kernel.org/git/4c623fcf-01dd-4056-80c1-b3c860ab7f87@gmail.com/
• Message:
• Rewrite message now that we are testing something different
• consistency with the other two messages (see previous)
v2:
• `strbuf_commented_addf` adds a newline, unlike the previous function.
We need to remove a newline from the final `strbuf_addstr` with `This
reverts commits` and add a newline to each of the other
branches (`else if` and `else`).
sequencer.c | 9 +++++----
t/t3501-revert-cherry-pick.sh | 14 ++++++++++++++
2 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 1b6fd86f70b..d26299cdea2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2341,8 +2341,8 @@ static int do_pick_commit(struct repository *r,
next = parent;
next_label = msg.parent_label;
if (opts->commit_use_reference) {
- strbuf_addstr(&ctx->message,
- "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
+ strbuf_commented_addf(&ctx->message, comment_line_str,
+ "*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
/*
* We don't touch pre-existing repeated reverts, because
@@ -2352,12 +2352,13 @@ static int do_pick_commit(struct repository *r,
!starts_with(orig_subject, "Revert \"")) {
strbuf_addstr(&ctx->message, "Reapply \"");
strbuf_addstr(&ctx->message, orig_subject);
+ strbuf_addstr(&ctx->message, "\n");
} else {
strbuf_addstr(&ctx->message, "Revert \"");
strbuf_addstr(&ctx->message, msg.subject);
- strbuf_addstr(&ctx->message, "\"");
+ strbuf_addstr(&ctx->message, "\"\n");
}
- strbuf_addstr(&ctx->message, "\n\nThis reverts commit ");
+ strbuf_addstr(&ctx->message, "\nThis reverts commit ");
refer_to_commit(opts, &ctx->message, commit);
if (commit->parents && commit->parents->next) {
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 411027fb58c..43476236131 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -228,6 +228,20 @@ test_expect_success 'identification of reverted commit (--reference)' '
test_cmp expect actual
'
+test_expect_success 'git revert --reference with core.commentChar' '
+ test_when_finished "git reset --hard to-ident" &&
+ git checkout --detach to-ident &&
+ GIT_EDITOR="cat | head -4 >actual" git -c core.commentChar=% revert \
+ --edit --reference HEAD &&
+ 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
+'
+
test_expect_success 'identification of reverted commit (revert.reference)' '
git checkout --detach to-ident &&
git -c revert.reference=true revert --no-edit HEAD &&
--
2.47.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v3 3/3] sequencer: comment commit messages properly
2024-11-24 20:56 ` [PATCH v3 " kristofferhaugsbakk
2024-11-24 20:56 ` [PATCH v3 1/3] sequencer: comment checked-out branch properly kristofferhaugsbakk
2024-11-24 20:56 ` [PATCH v3 2/3] sequencer: comment `--reference` subject line properly kristofferhaugsbakk
@ 2024-11-24 20:56 ` kristofferhaugsbakk
2024-11-25 10:07 ` [PATCH v3 0/3] sequencer: comment out properly in todo list phillip.wood123
2024-11-25 20:13 ` [PATCH v4 " kristofferhaugsbakk
4 siblings, 0 replies; 42+ messages in thread
From: kristofferhaugsbakk @ 2024-11-24 20:56 UTC (permalink / raw)
To: git; +Cc: Kristoffer Haugsbakk, stolee, phillip.wood123, me, Phillip Wood
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
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>
---
Notes (series):
v3:
• Message: Explain to the best of my knowledge what is going on here in
the message body
Link: https://lore.kernel.org/git/711b59d7-e649-4031-8924-a16fb632b4d4@gmail.com/
• Fixed wrong/subpar use of trailers
Link: https://lore.kernel.org/git/711b59d7-e649-4031-8924-a16fb632b4d4@gmail.com/
v2:
• Phillip contributed the test and the `strbuf_setlen` changes
Notes (meta-trailers):
Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Link: https://lore.kernel.org/git/cfa466b8-a87d-4b5d-b330-6c660897de48@gmail.com/#t
sequencer.c | 12 ++++++++----
t/t3437-rebase-fixup-options.sh | 15 +++++++++++++++
2 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index d26299cdea2..42a6f257cbb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1941,10 +1941,10 @@ static int seen_squash(struct replay_ctx *ctx)
static void update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n)
{
- strbuf_setlen(buf1, 2);
+ strbuf_setlen(buf1, strlen(comment_line_str) + 1);
strbuf_addf(buf1, _(nth_commit_msg_fmt), n);
strbuf_addch(buf1, '\n');
- strbuf_setlen(buf2, 2);
+ strbuf_setlen(buf2, strlen(comment_line_str) + 1);
strbuf_addf(buf2, _(skip_nth_commit_msg_fmt), n);
strbuf_addch(buf2, '\n');
}
@@ -1963,8 +1963,12 @@ static void update_squash_message_for_fixup(struct strbuf *msg)
size_t orig_msg_len;
int i = 1;
- strbuf_addf(&buf1, "# %s\n", _(first_commit_msg_str));
- strbuf_addf(&buf2, "# %s\n", _(skip_first_commit_msg_str));
+ strbuf_add_commented_lines(&buf1, _(first_commit_msg_str),
+ strlen(_(first_commit_msg_str)),
+ comment_line_str);
+ strbuf_add_commented_lines(&buf2, _(skip_first_commit_msg_str),
+ strlen(_(skip_first_commit_msg_str)),
+ comment_line_str);
s = start = orig_msg = strbuf_detach(msg, &orig_msg_len);
while (s) {
const char *next;
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 7929e2e2e3a..a4b90e881e3 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -127,6 +127,21 @@ test_expect_success 'fixup -C with conflicts gives correct message' '
test_cmp expected-author actual-author
'
+test_expect_success 'conflicting fixup -C after fixup with custom comment string' '
+ test_config core.commentString COMMENT &&
+ test_when_finished "test_might_fail git rebase --abort" &&
+ git checkout --detach A3 &&
+ test_must_fail env FAKE_LINES="1 fixup 2 fixup_-C 4" git rebase -i A &&
+ echo resolved >A &&
+ git add A &&
+ FAKE_COMMIT_AMEND=edited git rebase --continue &&
+ test_commit_message HEAD <<-\EOF
+ A3
+
+ edited
+ EOF
+'
+
test_expect_success 'skipping fixup -C after fixup gives correct message' '
test_when_finished "test_might_fail git rebase --abort" &&
git checkout --detach A3 &&
--
2.47.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v3 0/3] sequencer: comment out properly in todo list
2024-11-24 20:56 ` [PATCH v3 " kristofferhaugsbakk
` (2 preceding siblings ...)
2024-11-24 20:56 ` [PATCH v3 3/3] sequencer: comment commit messages properly kristofferhaugsbakk
@ 2024-11-25 10:07 ` phillip.wood123
2024-11-25 10:52 ` Kristoffer Haugsbakk
2024-11-25 20:13 ` [PATCH v4 " kristofferhaugsbakk
4 siblings, 1 reply; 42+ messages in thread
From: phillip.wood123 @ 2024-11-25 10:07 UTC (permalink / raw)
To: kristofferhaugsbakk, git; +Cc: Kristoffer Haugsbakk, stolee, me
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
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 0/3] sequencer: comment out properly in todo list
2024-11-25 10:07 ` [PATCH v3 0/3] sequencer: comment out properly in todo list phillip.wood123
@ 2024-11-25 10:52 ` Kristoffer Haugsbakk
2024-11-25 14:36 ` phillip.wood123
0 siblings, 1 reply; 42+ messages in thread
From: Kristoffer Haugsbakk @ 2024-11-25 10:52 UTC (permalink / raw)
To: Phillip Wood, git; +Cc: Kristoffer Haugsbakk, Derrick Stolee, Taylor Blau
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
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v3 0/3] sequencer: comment out properly in todo list
2024-11-25 10:52 ` Kristoffer Haugsbakk
@ 2024-11-25 14:36 ` phillip.wood123
0 siblings, 0 replies; 42+ messages in thread
From: phillip.wood123 @ 2024-11-25 14:36 UTC (permalink / raw)
To: Kristoffer Haugsbakk, Phillip Wood, git
Cc: Kristoffer Haugsbakk, Derrick Stolee, Taylor Blau
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
^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH v4 0/3] sequencer: comment out properly in todo list
2024-11-24 20:56 ` [PATCH v3 " kristofferhaugsbakk
` (3 preceding siblings ...)
2024-11-25 10:07 ` [PATCH v3 0/3] sequencer: comment out properly in todo list phillip.wood123
@ 2024-11-25 20:13 ` kristofferhaugsbakk
2024-11-25 20:13 ` [PATCH v4 1/3] sequencer: comment checked-out branch properly kristofferhaugsbakk
` (3 more replies)
4 siblings, 4 replies; 42+ messages in thread
From: kristofferhaugsbakk @ 2024-11-25 20:13 UTC (permalink / raw)
To: gitster; +Cc: Kristoffer Haugsbakk, git, stolee, phillip.wood123, me
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
Fix three places where the comment char/string is hardcoded (#) in the
todo list.
§ Changes in v4
• Use `test_grep`
• Fix commit message (`)
• Don’t need to cat(1)
• Also use `-n4` in case `-4` is not widely supported
§ CC
• Stolee for the first patch
• Reviewers on the previous rounds
Kristoffer Haugsbakk (3):
sequencer: comment checked-out branch properly
sequencer: comment `--reference` subject line properly
sequencer: comment commit messages properly
sequencer.c | 26 ++++++++++++++++----------
t/t3400-rebase.sh | 19 +++++++++++++++++++
t/t3437-rebase-fixup-options.sh | 15 +++++++++++++++
t/t3501-revert-cherry-pick.sh | 14 ++++++++++++++
4 files changed, 64 insertions(+), 10 deletions(-)
Interdiff against v3:
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 711bd230695..7c47af6dcd9 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -471,8 +471,8 @@ test_expect_success 'git rebase --update-ref with core.commentChar and branch on
git checkout topic2 &&
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_grep "% Ref refs/heads/wt-topic checked out at" actual &&
+ test_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 43476236131..b84fdfe8a32 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -231,7 +231,7 @@ 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_EDITOR="cat | head -4 >actual" git -c core.commentChar=% revert \
+ GIT_EDITOR="head -n4 >actual" git -c core.commentChar=% revert \
--edit --reference HEAD &&
cat <<-EOF >expect &&
% *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***
Range-diff against v3:
1: a46767263f6 ! 1: a8813b5f14c sequencer: comment checked-out branch properly
@@ Commit message
## Notes (series) ##
+ v4
+ • Use `test_grep`
+
+ Link: https://lore.kernel.org/git/5267b9a9c8cc5cc66979117dc4c1e4d7329e2a03.1729704370.git.code@khaugsbakk.name/T/#me80519debcd013aa8c8a5e5003c58cff7281fac9
v3:
• Review feedback: check more in the test by inspecting the
sequence editor
@@ t/t3400-rebase.sh: test_expect_success 'rebase when inside worktree subdirectory
+ git checkout topic2 &&
+ 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_grep "% Ref refs/heads/wt-topic checked out at" actual &&
++ test_grep "% Ref refs/heads/topic2 checked out at" actual
+'
+
test_done
2: 7a452142666 ! 2: 4d10ad4ab55 sequencer: comment `--reference` subject line properly
@@ Commit message
## Notes (series) ##
+ v4:
+ • Don’t need to cat(1)
+ • Also use `-n4` in case `-4` is not widely supported
+
+ Link: https://lore.kernel.org/git/7739a6e2-8758-4d0f-b1d6-f0879a89590f@gmail.com/
v3:
• Review feedback: check more in the test by inspecting the
proposed commit message.
@@ 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_EDITOR="cat | head -4 >actual" git -c core.commentChar=% revert \
++ GIT_EDITOR="head -n4 >actual" git -c core.commentChar=% revert \
+ --edit --reference HEAD &&
+ cat <<-EOF >expect &&
+ % *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***
3: 4c342bc0422 ! 3: 42b9fbd12d6 sequencer: comment commit messages properly
@@ Commit message
fixup hash2 <msg>
fixup -c hash3 <msg>
- This says that hash2` and hash3 should be squashed into hash1 and
+ 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
@@ Commit message
## Notes (series) ##
+ v4:
+ • Fix commit message (`)
+
+ Link: https://lore.kernel.org/git/5267b9a9c8cc5cc66979117dc4c1e4d7329e2a03.1729704370.git.code@khaugsbakk.name/T/#me80519debcd013aa8c8a5e5003c58cff7281fac9
v3:
• Message: Explain to the best of my knowledge what is going on here in
the message body
base-commit: b31fb630c0fc6869a33ed717163e8a1210460d94
--
2.47.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v4 1/3] sequencer: comment checked-out branch properly
2024-11-25 20:13 ` [PATCH v4 " kristofferhaugsbakk
@ 2024-11-25 20:13 ` kristofferhaugsbakk
2024-11-25 20:13 ` [PATCH v4 2/3] sequencer: comment `--reference` subject line properly kristofferhaugsbakk
` (2 subsequent siblings)
3 siblings, 0 replies; 42+ messages in thread
From: kristofferhaugsbakk @ 2024-11-25 20:13 UTC (permalink / raw)
To: gitster; +Cc: Kristoffer Haugsbakk, git, stolee, phillip.wood123, me
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
`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>
---
Notes (series):
v4
• Use `test_grep`
Link: https://lore.kernel.org/git/5267b9a9c8cc5cc66979117dc4c1e4d7329e2a03.1729704370.git.code@khaugsbakk.name/T/#me80519debcd013aa8c8a5e5003c58cff7281fac9
v3:
• Review feedback: check more in the test by inspecting the
sequence editor
Link: https://lore.kernel.org/git/5ed77fab-678d-4a06-bbd0-ea25462a7562@gmail.com/
• Message: consistency with the other two messages:
• Mention both commentChar and commentString
• Commit footnote style: See <commit>
v2:
• Message: “hardcoded” (more common according to `git grep`)
sequencer.c | 5 +++--
t/t3400-rebase.sh | 19 +++++++++++++++++++
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 353d804999b..1b6fd86f70b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -6382,8 +6382,9 @@ static int add_decorations_to_list(const struct commit *commit,
/* If the branch is checked out, then leave a comment instead. */
if ((path = branch_checked_out(decoration->name))) {
item->command = TODO_COMMENT;
- strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
- decoration->name, path);
+ strbuf_commented_addf(ctx->buf, comment_line_str,
+ "Ref %s checked out at '%s'\n",
+ decoration->name, path);
} else {
struct string_list_item *sti;
item->command = TODO_UPDATE_REF;
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 09f230eefb2..7c47af6dcd9 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -456,4 +456,23 @@ test_expect_success 'rebase when inside worktree subdirectory' '
)
'
+test_expect_success 'git rebase --update-ref with core.commentChar and branch on worktree' '
+ test_when_finished git branch -D base topic2 &&
+ test_when_finished git checkout main &&
+ test_when_finished git branch -D wt-topic &&
+ test_when_finished git worktree remove wt-topic &&
+ git checkout main &&
+ git checkout -b base &&
+ git checkout -b topic2 &&
+ test_commit msg2 &&
+ git worktree add wt-topic &&
+ git checkout base &&
+ test_commit msg3 &&
+ git checkout topic2 &&
+ GIT_SEQUENCE_EDITOR="cat >actual" git -c core.commentChar=% \
+ rebase -i --update-refs base &&
+ test_grep "% Ref refs/heads/wt-topic checked out at" actual &&
+ test_grep "% Ref refs/heads/topic2 checked out at" actual
+'
+
test_done
--
2.47.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v4 2/3] sequencer: comment `--reference` subject line properly
2024-11-25 20:13 ` [PATCH v4 " kristofferhaugsbakk
2024-11-25 20:13 ` [PATCH v4 1/3] sequencer: comment checked-out branch properly kristofferhaugsbakk
@ 2024-11-25 20:13 ` kristofferhaugsbakk
2024-11-25 20:13 ` [PATCH v4 3/3] sequencer: comment commit messages properly kristofferhaugsbakk
2024-11-26 1:11 ` [PATCH v4 0/3] sequencer: comment out properly in todo list Junio C Hamano
3 siblings, 0 replies; 42+ messages in thread
From: kristofferhaugsbakk @ 2024-11-25 20:13 UTC (permalink / raw)
To: gitster; +Cc: Kristoffer Haugsbakk, git, stolee, phillip.wood123, me
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
`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>
---
Notes (series):
v4:
• Don’t need to cat(1)
• Also use `-n4` in case `-4` is not widely supported
Link: https://lore.kernel.org/git/7739a6e2-8758-4d0f-b1d6-f0879a89590f@gmail.com/
v3:
• Review feedback: check more in the test by inspecting the
proposed commit message.
Link: https://lore.kernel.org/git/4c623fcf-01dd-4056-80c1-b3c860ab7f87@gmail.com/
• Message:
• Rewrite message now that we are testing something different
• consistency with the other two messages (see previous)
v2:
• `strbuf_commented_addf` adds a newline, unlike the previous function.
We need to remove a newline from the final `strbuf_addstr` with `This
reverts commits` and add a newline to each of the other
branches (`else if` and `else`).
sequencer.c | 9 +++++----
t/t3501-revert-cherry-pick.sh | 14 ++++++++++++++
2 files changed, 19 insertions(+), 4 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 1b6fd86f70b..d26299cdea2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2341,8 +2341,8 @@ static int do_pick_commit(struct repository *r,
next = parent;
next_label = msg.parent_label;
if (opts->commit_use_reference) {
- strbuf_addstr(&ctx->message,
- "# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
+ strbuf_commented_addf(&ctx->message, comment_line_str,
+ "*** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
} else if (skip_prefix(msg.subject, "Revert \"", &orig_subject) &&
/*
* We don't touch pre-existing repeated reverts, because
@@ -2352,12 +2352,13 @@ static int do_pick_commit(struct repository *r,
!starts_with(orig_subject, "Revert \"")) {
strbuf_addstr(&ctx->message, "Reapply \"");
strbuf_addstr(&ctx->message, orig_subject);
+ strbuf_addstr(&ctx->message, "\n");
} else {
strbuf_addstr(&ctx->message, "Revert \"");
strbuf_addstr(&ctx->message, msg.subject);
- strbuf_addstr(&ctx->message, "\"");
+ strbuf_addstr(&ctx->message, "\"\n");
}
- strbuf_addstr(&ctx->message, "\n\nThis reverts commit ");
+ strbuf_addstr(&ctx->message, "\nThis reverts commit ");
refer_to_commit(opts, &ctx->message, commit);
if (commit->parents && commit->parents->next) {
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 411027fb58c..b84fdfe8a32 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -228,6 +228,20 @@ test_expect_success 'identification of reverted commit (--reference)' '
test_cmp expect actual
'
+test_expect_success 'git revert --reference with core.commentChar' '
+ test_when_finished "git reset --hard to-ident" &&
+ git checkout --detach to-ident &&
+ GIT_EDITOR="head -n4 >actual" git -c core.commentChar=% revert \
+ --edit --reference HEAD &&
+ 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
+'
+
test_expect_success 'identification of reverted commit (revert.reference)' '
git checkout --detach to-ident &&
git -c revert.reference=true revert --no-edit HEAD &&
--
2.47.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* [PATCH v4 3/3] sequencer: comment commit messages properly
2024-11-25 20:13 ` [PATCH v4 " kristofferhaugsbakk
2024-11-25 20:13 ` [PATCH v4 1/3] sequencer: comment checked-out branch properly kristofferhaugsbakk
2024-11-25 20:13 ` [PATCH v4 2/3] sequencer: comment `--reference` subject line properly kristofferhaugsbakk
@ 2024-11-25 20:13 ` kristofferhaugsbakk
2024-11-26 1:11 ` [PATCH v4 0/3] sequencer: comment out properly in todo list Junio C Hamano
3 siblings, 0 replies; 42+ messages in thread
From: kristofferhaugsbakk @ 2024-11-25 20:13 UTC (permalink / raw)
To: gitster
Cc: Kristoffer Haugsbakk, git, stolee, phillip.wood123, me,
Phillip Wood
From: Kristoffer Haugsbakk <code@khaugsbakk.name>
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>
---
Notes (series):
v4:
• Fix commit message (`)
Link: https://lore.kernel.org/git/5267b9a9c8cc5cc66979117dc4c1e4d7329e2a03.1729704370.git.code@khaugsbakk.name/T/#me80519debcd013aa8c8a5e5003c58cff7281fac9
v3:
• Message: Explain to the best of my knowledge what is going on here in
the message body
Link: https://lore.kernel.org/git/711b59d7-e649-4031-8924-a16fb632b4d4@gmail.com/
• Fixed wrong/subpar use of trailers
Link: https://lore.kernel.org/git/711b59d7-e649-4031-8924-a16fb632b4d4@gmail.com/
v2:
• Phillip contributed the test and the `strbuf_setlen` changes
Notes (meta-trailers):
Co-authored-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Link: https://lore.kernel.org/git/cfa466b8-a87d-4b5d-b330-6c660897de48@gmail.com/#t
sequencer.c | 12 ++++++++----
t/t3437-rebase-fixup-options.sh | 15 +++++++++++++++
2 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index d26299cdea2..42a6f257cbb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1941,10 +1941,10 @@ static int seen_squash(struct replay_ctx *ctx)
static void update_comment_bufs(struct strbuf *buf1, struct strbuf *buf2, int n)
{
- strbuf_setlen(buf1, 2);
+ strbuf_setlen(buf1, strlen(comment_line_str) + 1);
strbuf_addf(buf1, _(nth_commit_msg_fmt), n);
strbuf_addch(buf1, '\n');
- strbuf_setlen(buf2, 2);
+ strbuf_setlen(buf2, strlen(comment_line_str) + 1);
strbuf_addf(buf2, _(skip_nth_commit_msg_fmt), n);
strbuf_addch(buf2, '\n');
}
@@ -1963,8 +1963,12 @@ static void update_squash_message_for_fixup(struct strbuf *msg)
size_t orig_msg_len;
int i = 1;
- strbuf_addf(&buf1, "# %s\n", _(first_commit_msg_str));
- strbuf_addf(&buf2, "# %s\n", _(skip_first_commit_msg_str));
+ strbuf_add_commented_lines(&buf1, _(first_commit_msg_str),
+ strlen(_(first_commit_msg_str)),
+ comment_line_str);
+ strbuf_add_commented_lines(&buf2, _(skip_first_commit_msg_str),
+ strlen(_(skip_first_commit_msg_str)),
+ comment_line_str);
s = start = orig_msg = strbuf_detach(msg, &orig_msg_len);
while (s) {
const char *next;
diff --git a/t/t3437-rebase-fixup-options.sh b/t/t3437-rebase-fixup-options.sh
index 7929e2e2e3a..a4b90e881e3 100755
--- a/t/t3437-rebase-fixup-options.sh
+++ b/t/t3437-rebase-fixup-options.sh
@@ -127,6 +127,21 @@ test_expect_success 'fixup -C with conflicts gives correct message' '
test_cmp expected-author actual-author
'
+test_expect_success 'conflicting fixup -C after fixup with custom comment string' '
+ test_config core.commentString COMMENT &&
+ test_when_finished "test_might_fail git rebase --abort" &&
+ git checkout --detach A3 &&
+ test_must_fail env FAKE_LINES="1 fixup 2 fixup_-C 4" git rebase -i A &&
+ echo resolved >A &&
+ git add A &&
+ FAKE_COMMIT_AMEND=edited git rebase --continue &&
+ test_commit_message HEAD <<-\EOF
+ A3
+
+ edited
+ EOF
+'
+
test_expect_success 'skipping fixup -C after fixup gives correct message' '
test_when_finished "test_might_fail git rebase --abort" &&
git checkout --detach A3 &&
--
2.47.0
^ permalink raw reply related [flat|nested] 42+ messages in thread* Re: [PATCH v4 0/3] sequencer: comment out properly in todo list
2024-11-25 20:13 ` [PATCH v4 " kristofferhaugsbakk
` (2 preceding siblings ...)
2024-11-25 20:13 ` [PATCH v4 3/3] sequencer: comment commit messages properly kristofferhaugsbakk
@ 2024-11-26 1:11 ` Junio C Hamano
2024-11-26 11:24 ` Phillip Wood
3 siblings, 1 reply; 42+ messages in thread
From: Junio C Hamano @ 2024-11-26 1:11 UTC (permalink / raw)
To: kristofferhaugsbakk
Cc: Kristoffer Haugsbakk, git, stolee, phillip.wood123, me
kristofferhaugsbakk@fastmail.com writes:
> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> Fix three places where the comment char/string is hardcoded (#) in the
> todo list.
>
> § Changes in v4
>
> • Use `test_grep`
> • Fix commit message (`)
> • Don’t need to cat(1)
> • Also use `-n4` in case `-4` is not widely supported
All changes make sense. Will queue.
^ permalink raw reply [flat|nested] 42+ messages in thread* Re: [PATCH v4 0/3] sequencer: comment out properly in todo list
2024-11-26 1:11 ` [PATCH v4 0/3] sequencer: comment out properly in todo list Junio C Hamano
@ 2024-11-26 11:24 ` Phillip Wood
2024-11-27 12:39 ` Kristoffer Haugsbakk
0 siblings, 1 reply; 42+ messages in thread
From: Phillip Wood @ 2024-11-26 11:24 UTC (permalink / raw)
To: Junio C Hamano, kristofferhaugsbakk; +Cc: Kristoffer Haugsbakk, git, stolee, me
On 26/11/2024 01:11, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
>
>> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>>
>> Fix three places where the comment char/string is hardcoded (#) in the
>> todo list.
>>
>> § Changes in v4
>>
>> • Use `test_grep`
>> • Fix commit message (`)
>> • Don’t need to cat(1)
>> • Also use `-n4` in case `-4` is not widely supported
>
> All changes make sense. Will queue.
This version looks good to me too, thanks for working on it Kristoffer
Best Wishes
Phillip
^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH v4 0/3] sequencer: comment out properly in todo list
2024-11-26 11:24 ` Phillip Wood
@ 2024-11-27 12:39 ` Kristoffer Haugsbakk
0 siblings, 0 replies; 42+ messages in thread
From: Kristoffer Haugsbakk @ 2024-11-27 12:39 UTC (permalink / raw)
To: Phillip Wood, Junio C Hamano, Kristoffer Haugsbakk
Cc: git, Derrick Stolee, Taylor Blau
On Tue, Nov 26, 2024, at 12:24, Phillip Wood wrote:
> On 26/11/2024 01:11, Junio C Hamano wrote:
>> kristofferhaugsbakk@fastmail.com writes:
>>
>>> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>>>
>>> Fix three places where the comment char/string is hardcoded (#) in the
>>> todo list.
>>>
>>> § Changes in v4
>>>
>>> • Use `test_grep`
>>> • Fix commit message (`)
>>> • Don’t need to cat(1)
>>> • Also use `-n4` in case `-4` is not widely supported
>>
>> All changes make sense. Will queue.
>
> This version looks good to me too, thanks for working on it Kristoffer
Thanks for the reviews and for finishing the last test coverage in this
series (fixup -c/-C). I was not familiar with that code as a user so I
didn’t know what was expected.
Cheers
--
Kristoffer
^ permalink raw reply [flat|nested] 42+ messages in thread