From: Phillip Wood <phillip.wood123@gmail•com>
To: Ayush Chandekar <ayu.chandekar@gmail•com>
Cc: christian.couder@gmail•com, git@vger•kernel.org,
shyamthakkar001@gmail•com, kristofferhaugsbakk@fastmail•com,
gitster@pobox•com
Subject: Re: [GSOC PATCH v2] commit: avoid scanning trailing comments when 'core.commentChar' is "auto"
Date: Fri, 27 Jun 2025 09:34:30 +0100 [thread overview]
Message-ID: <91982162-b138-4bb1-81fd-6f9185801c99@gmail.com> (raw)
In-Reply-To: <20250626221631.457725-1-ayu.chandekar@gmail.com>
Hi Ayush
On 26/06/2025 23:16, Ayush Chandekar wrote:
> When core.commentChar is set to "auto", Git selects a comment character
> by scanning the commit message contents and avoiding any character
> already present in the message.
>
> If the message still contains old conflict comments (starting with a
> comment character), Git assumes that character is in use and chooses a
> different one. As a result, those existing comment lines are no longer
> recognized as comments and end up being included in the final commit
> message.
>
> To avoid this, skip scanning the trailing comment block when selecting
> the comment character. This allows Git to safely reuse the original
> character when appropriate, keeping the commit message clean and free of
> leftover conflict information.
This is a good explanation of the problem. Maybe this is another reason
to consider removing support for commentChar=auto [1]
[1] https://lore.kernel.org/git/xmqqa59i45wc.fsf@gitster.g/
> Background:
>
> The "auto" value for core.commentchar was introduced in the commit
> 84c9dc2c5a (commit: allow core.commentChar=auto for character auto
> selection, 2014-05-17) but did not exhibt this issue at that time.
>
> The bug was introduced in commit a6c2654f83 (rebase -m: fix --signoff
> with conflicts, 2024-04-18) where Git started writing conflict comments
> to the file at 'rebase_path_message()'.
>
> Mentored-by: Christian Couder <christian.couder@gmail•com>
> Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail•com>
> Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail•com>
> ---
>
> Thanks to Christian for mentoring, and to Kristopher and Junio for their reviews!
>
> builtin/commit.c | 6 +++++-
> t/t3418-rebase-continue.sh | 14 ++++++++++++++
> 2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index fba0dded64..63e7158e98 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -688,6 +688,10 @@ static void adjust_comment_line_char(const struct strbuf *sb)
> char candidates[] = "#;@!$%^&|:";
> char *candidate;
> const char *p;
> + size_t cutoff;
> +
> + /* Ignore comment chars in trailing comments (e.g., Conflicts:) */
> + cutoff = sb->len - ignored_log_message_bytes(sb->buf, sb->len);
This finds the "Conflicts:" line. I was surprised to see that the string
it looks for is hard coded and not translated, however the sequencer
(also surprisingly) does not translate that message either so it should
work.
>
> if (!memchr(sb->buf, candidates[0], sb->len)) {
> free(comment_line_str_to_free);
> @@ -700,7 +704,7 @@ static void adjust_comment_line_char(const struct strbuf *sb)
> candidate = strchr(candidates, *p);
> if (candidate)
> *candidate = ' ';
> - for (p = sb->buf; *p; p++) {
> + for (p = sb->buf; p + 1 < sb->buf + cutoff; p++) {
> if ((p[0] == '\n' || p[0] == '\r') && p[1]) {
> candidate = strchr(candidates, p[1]);
> if (candidate)
> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 127216f722..ccfe77af6c 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -328,6 +328,20 @@ test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebas
> test_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec
> '
>
> +test_expect_success 'no change in comment character due to conflicts markers with core.commentChar=auto' '
> + test_commit base file &&
If you used an existing file (F1 or F2) like most of the rest of the
tests in this file we could avoid creating this commit and save
ourselves a couple of processes.
> + git checkout -b branch-a &&
> + test_commit A file &&
> + git checkout -b branch-b base &&
> + test_commit B file &&
> + test_must_fail git rebase branch-a &&
> + printf "B\nA\n" >file &&
> + git add file &&
> + GIT_EDITOR="cat >actual" git -c core.commentChar=auto rebase --continue &&
> + # Check that "#" is still the comment character.
> + test_grep "^# Changes to be committed:$" actual
I agree that it is a good idea to anchor the start of the message, but
I'm not sure it is helpful to anchor the end of the message as we don't
want the test to fail just because an unrelated change adds some
whitespace to the end of this line. I'd be tempted to drop the ':' for
the same reason.
Thanks for fixing this
Phillip
> +'
> +
> test_orig_head_helper () {
> test_when_finished 'git rebase --abort &&
> git checkout topic &&
next prev parent reply other threads:[~2025-06-27 8:34 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-06-26 13:22 [GSOC PATCH] commit: avoid scanning trailing comments when 'core.commentChar' is "auto" Ayush Chandekar
2025-06-26 14:33 ` Junio C Hamano
2025-06-26 21:30 ` Ayush Chandekar
2025-06-26 15:40 ` Kristoffer Haugsbakk
2025-06-26 21:28 ` Ayush Chandekar
2025-06-26 22:16 ` [GSOC PATCH v2] " Ayush Chandekar
2025-06-27 8:34 ` Phillip Wood [this message]
2025-06-27 14:52 ` Junio C Hamano
2025-06-28 10:37 ` Ayush Chandekar
2025-06-28 13:38 ` Phillip Wood
2025-06-28 14:33 ` Ayush Chandekar
2025-06-30 8:59 ` Phillip Wood
2025-06-30 17:34 ` Ayush Chandekar
2025-06-28 15:10 ` Phillip Wood
2025-06-30 14:11 ` Junio C Hamano
2025-06-28 10:18 ` Ayush Chandekar
2025-06-27 9:04 ` Christian Couder
2025-06-30 18:25 ` [GSOC PATCH v3] " Ayush Chandekar
2025-07-01 13:17 ` Phillip Wood
2025-07-01 18:33 ` Ayush Chandekar
2025-07-01 19:31 ` Phillip Wood
2025-07-02 23:46 ` Ayush Chandekar
2025-07-04 8:23 ` Phillip Wood
2025-07-08 15:47 ` Ayush Chandekar
2025-07-09 14:17 ` Phillip Wood
2025-07-15 18:51 ` [GSOC PATCH 0/2] commit: improve behaviour of core.commentChar=auto for comments in commit messages Ayush Chandekar
2025-07-15 18:51 ` [GSOC PATCH 1/2] commit: avoid scanning trailing comments when 'core.commentChar' is "auto" Ayush Chandekar
2025-07-15 18:57 ` [GSOC PATCH v4 " Ayush Chandekar
2025-07-15 18:51 ` [GSOC PATCH 2/2] config: set comment_line_str to "#" when core.commentChar=auto Ayush Chandekar
2025-07-15 18:57 ` [GSOC PATCH v4 " Ayush Chandekar
2025-07-15 21:23 ` [GSOC PATCH " Junio C Hamano
2025-07-15 22:15 ` Ayush Chandekar
2025-07-15 23:30 ` Junio C Hamano
2025-07-16 11:04 ` Ayush Chandekar
2025-07-16 15:21 ` Junio C Hamano
2025-07-16 15:24 ` Phillip Wood
2025-07-16 15:29 ` Junio C Hamano
2025-07-15 18:56 ` [GSOC PATCH v4 0/2] commit: improve behaviour of core.commentChar=auto for comments in commit messages Ayush Chandekar
2025-07-16 11:43 ` [GSOC PATCH v5 " Ayush Chandekar
2025-07-16 11:43 ` [GSOC PATCH v5 1/2] commit: avoid scanning trailing comments when 'core.commentChar' is "auto" Ayush Chandekar
2025-07-16 11:43 ` [GSOC PATCH v5 2/2] config: set comment_line_str to "#" when core.commentChar=auto Ayush Chandekar
2025-07-16 15:28 ` Junio C Hamano
2025-07-16 14:28 ` [GSOC PATCH v5 0/2] commit: improve behaviour of core.commentChar=auto for comments in commit messages Phillip Wood
2025-07-16 15:29 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=91982162-b138-4bb1-81fd-6f9185801c99@gmail.com \
--to=phillip.wood123@gmail$(echo .)com \
--cc=ayu.chandekar@gmail$(echo .)com \
--cc=christian.couder@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=kristofferhaugsbakk@fastmail$(echo .)com \
--cc=phillip.wood@dunelm$(echo .)org.uk \
--cc=shyamthakkar001@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox