Subject: Re: [GSOC PATCH v2] commit: avoid scanning trailing comments when 'core.commentChar' is "auto"

From: Phillip Wood <[email-masked]>
Date: Fri, 27 Jun 2025 09:34:30 +0100

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/[email-masked]/

> 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 <[email-masked]>
> Mentored-by: Ghanshyam Thakkar <[email-masked]>
> Signed-off-by: Ayush Chandekar <[email-masked]>
> ---
> 
> 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 &&