patchcommit: avoid scanning trailing comments when 'core.commentChar' is "auto"

> 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.
>
> Background:
>
> The "auto" value for core.commentchar was introduced in the commit
> `84c9dc2` (commit: allow core.commentChar=auto for character auto
> selection) but did not exhibt this issue at that time.

> 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);
>  
>  	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)

>
> Ayush Chandekar <ayu.chandekar@gmail.com> writes:
>
> > 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.
> >
> > Background:
> >
> > The "auto" value for core.commentchar was introduced in the commit
> > `84c9dc2` (commit: allow core.commentChar=auto for character auto
> > selection) but did not exhibt this issue at that time.
>
> Use "git log -1 --format=reference", i.e.
>
> 84c9dc2c (commit: allow core.commentChar=auto for character auto
> selection, 2014-05-17)
>

> 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.
>
> Background:
>
> The "auto" value for core.commentchar was introduced in the commit
> `84c9dc2` (commit: allow core.commentChar=auto for character auto
> selection) but did not exhibt this issue at that time.
>
> The bug was introduced in commit `a6c2654` (rebase -m: fix --signoff
> with conflicts) 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>
> ---

> diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> index 127216f722..a8e89a250b 100755
> --- a/t/t3418-rebase-continue.sh
> +++ b/t/t3418-rebase-continue.sh
> @@ -328,6 +328,24 @@ 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 &&
> +	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 &&
> +	write_script fake-editor <<-\EOF &&
> +	exit 0
> +	EOF
> +	FAKE_EDITOR="$(pwd)/fake-editor" &&
> +	GIT_EDITOR="\"\$FAKE_EDITOR\"" git -c core.commentChar=auto rebase --continue &&

> +'
> +
>  test_orig_head_helper () {
>  	test_when_finished 'git rebase --abort &&
>  		git checkout topic &&
> --
> 2.49.0

> >
> > +test_expect_success 'no change in comment character due to conflicts
> > markers with core.commentChar=auto' '
> > +     test_commit base file &&
> > +     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 &&
> > +     write_script fake-editor <<-\EOF &&
> > +     exit 0
> > +     EOF
> > +     FAKE_EDITOR="$(pwd)/fake-editor" &&
> > +     GIT_EDITOR="\"\$FAKE_EDITOR\"" git -c core.commentChar=auto rebase --continue &&
>
> How about
>
>     GIT_EDITOR="cat >actual"
>
> Then you can `test_grep` on that.  Like in
>
> https://lore.kernel.org/git/5ed77fab-678d-4a06-bbd0-ea25462a7562@gmail.com/
>
> > +     # Check that "#" is still the comment character.
> > +     test_grep "# Changes to be committed:" .git/COMMIT_EDITMSG
>

> 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.

> 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);

>   
>   	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 &&

> +	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

> +'
> +
>   test_orig_head_helper () {
>   	test_when_finished 'git rebase --abort &&
>   		git checkout topic &&

>> +	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.

>> +	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.

>
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
> >> +    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.
>
> There is a funny chicken-and-egg problem, though.  It limits the
> search for "Conflicts" by using wt_status_locate_end() based on the
> current value of comment_line_str.  When core.commentstring is set
> to "auto", the code that reads the configuration does not touch the
> comment_line_str variable, which is initialized to '#'.  So
>
>         [core]
>             commentstring = '%'
>             commentstring = auto
>
> would have '%' in comment_line_str upon entering this codepath, let
> wt_status_locate_end() use '%' as the comment string to find the end
> of the log message, and then looks for "Conflicts:" in the result.
>
> Which may or may not be what you want.
>

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>> +	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.
> 
> There is a funny chicken-and-egg problem, though.  It limits the
> search for "Conflicts" by using wt_status_locate_end() based on the
> current value of comment_line_str.  When core.commentstring is set
> to "auto", the code that reads the configuration does not touch the
> comment_line_str variable, which is initialized to '#'.  So
> 
> 	[core]
> 	    commentstring = '%'
> 	    commentstring = auto
> 
> would have '%' in comment_line_str upon entering this codepath, let
> wt_status_locate_end() use '%' as the comment string to find the end
> of the log message, and then looks for "Conflicts:" in the result.
> 
> Which may or may not be what you want.

>
> On 27/06/2025 15:52, Junio C Hamano wrote:
> > Phillip Wood <phillip.wood123@gmail.com> writes:
> >
> >>> +   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.
> >
> > There is a funny chicken-and-egg problem, though.  It limits the
> > search for "Conflicts" by using wt_status_locate_end() based on the
> > current value of comment_line_str.  When core.commentstring is set
> > to "auto", the code that reads the configuration does not touch the
> > comment_line_str variable, which is initialized to '#'.  So
> >
> >       [core]
> >           commentstring = '%'
> >           commentstring = auto
> >
> > would have '%' in comment_line_str upon entering this codepath, let
> > wt_status_locate_end() use '%' as the comment string to find the end
> > of the log message, and then looks for "Conflicts:" in the result.
> >
> > Which may or may not be what you want.
>
> Oh, good point - I'd not looked at the config parsing. So we'd create
> conflict comments that look like
>
>      % Conflicts:
>      %    some-file.c
>
> but so long as the commit message did not contain a '#' character [1]
> "git commit" would select '#' as the automatic comment char and our
> conflicts lines would not be treated as a comment.
>
> Should we be resetting comment_line_str to '#' when core.commentString
> is set to "auto"? That wont help if the commit message contains a '#'
> but at least it would be consistently broken.
>
> We could move adjust_comment_line_char() into libgit.a, use that to
> select the comment character used in append_conflicts_hint() and set
> core.commentString to that character when we run "git commit". I think
> doing that would mean that appending conflict comments would always work
> with core.commentString=auto but it is a more complex solution as we
> would need to remember the comment character and then pass it to git
> commit once the user had fixed the conflicts.
>

> One final note - although the commit message mentions a change to "git
> rebase" I think this problem already affected cherry-pick, revert and
> merge before that change. In practice I suspect it is only cherry-pick
> where one is likely to see this problem because the template messages
> for the other two commands are unlikely to contain a '#'.
>
> Thanks
>
> Phillip
>
> [1] For some reason adjust_comment_line_char() will not select '#' as
> the comment char if it occurs anywhere in the message but the other
> candidates are selected so long as they are not the first character on
> any line.

> 
> So, my GSoC project is refactoring in order to reduce the global state
> in Git. I was trying to remove the global variables related to comment
> characters. What I tried is to create one single function which
> returns the comment string, and we could then pass a strbuf in case of
> core.commentString=auto. You can check my attempts on my fork here [2]
> (check repo_get_comment_line_str() in config.c), and also mentioned
> this in my blog [3]. I thought I had it figured out, but turns out I
> failed one test where core.commentString=auto. It was that moment I
> realised that I would need to remember the comment character or the
> strbuf in functions. Just wanted to share this in case anything
> strikes you when looking at the approach.

>
> Hi Ayush
>
> On 28/06/2025 15:33, Ayush Chandekar wrote:
> >
> > So, my GSoC project is refactoring in order to reduce the global state
> > in Git. I was trying to remove the global variables related to comment
> > characters. What I tried is to create one single function which
> > returns the comment string, and we could then pass a strbuf in case of
> > core.commentString=auto. You can check my attempts on my fork here [2]
> > (check repo_get_comment_line_str() in config.c), and also mentioned
> > this in my blog [3]. I thought I had it figured out, but turns out I
> > failed one test where core.commentString=auto. It was that moment I
> > realised that I would need to remember the comment character or the
> > strbuf in functions. Just wanted to share this in case anything
> > strikes you when looking at the approach.
>
> Thanks for that context. I'm not sure about having a single function
> that handles both cases. There is only one caller that cares about
> "auto" and the support for that has so many corner cases that don't work
> I'm putting a patch together to deprecate it and remove it when Git 3.0
> is released.
>
> Looking at your code it seems to break the "last one wins" between
> core.commentChar and core.commentString. Also deferring the parsing
> until the comment character is used changes the behavior of things like
> "git -c core.commentchar=$'\n' commit -p". Instead of erroring out
> straight away it will let the user carefully select what they want to
> commit and then die which is not very user friendly.
>

> Keeping the current parsing logic and storing the result in struct
> repository might be a better approach though we should think about how
> commands that run without a repository will be able to access the system
> and user config settings.
>

> >
> >       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.
>

> 
> Range-diff with v2:
> 1:  4e74e7a9a6 ! 1:  693f890a36 commit: avoid scanning trailing comments when 'core.commentChar' is "auto"
>      @@ Commit message
>       
>           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.
>      +    selection, 2014-05-17) but did not exhibit 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
>      @@ t/t3418-rebase-continue.sh: test_expect_success 'there is no --no-reschedule-fai
>        '
>        
>       +test_expect_success 'no change in comment character due to conflicts markers with core.commentChar=auto' '
>      -+	test_commit base file &&
>       +	git checkout -b branch-a &&
>      -+	test_commit A file &&
>      -+	git checkout -b branch-b base &&
>      -+	test_commit B file &&
>      ++	test_commit A F1 &&
>      ++	git checkout -b branch-b HEAD^ &&
>      ++	test_commit B F1 &&
>       +	test_must_fail git rebase branch-a &&
>      -+	printf "B\nA\n" >file &&
>      -+	git add file &&
>      ++	printf "B\nA\n" >F1 &&
>      ++	git add F1 &&
>       +	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
>      ++	test_grep "^# Changes to be committed" actual
>       +'
>       +
>        test_orig_head_helper () {

>
> Hi Ayush
>
> On 30/06/2025 19:25, Ayush Chandekar wrote:
> >
> > Range-diff with v2:
> > 1:  4e74e7a9a6 ! 1:  693f890a36 commit: avoid scanning trailing comments when 'core.commentChar' is "auto"
> >      @@ Commit message
> >
> >           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.
> >      +    selection, 2014-05-17) but did not exhibit 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
> >      @@ t/t3418-rebase-continue.sh: test_expect_success 'there is no --no-reschedule-fai
> >        '
> >
> >       +test_expect_success 'no change in comment character due to conflicts markers with core.commentChar=auto' '
> >      -+       test_commit base file &&
> >       +       git checkout -b branch-a &&
> >      -+       test_commit A file &&
> >      -+       git checkout -b branch-b base &&
> >      -+       test_commit B file &&
> >      ++       test_commit A F1 &&
> >      ++       git checkout -b branch-b HEAD^ &&
> >      ++       test_commit B F1 &&
> >       +       test_must_fail git rebase branch-a &&
> >      -+       printf "B\nA\n" >file &&
> >      -+       git add file &&
> >      ++       printf "B\nA\n" >F1 &&
> >      ++       git add F1 &&
> >       +       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
> >      ++       test_grep "^# Changes to be committed" actual
> >       +'
> >       +
> >        test_orig_head_helper () {
>
> The changes here look good but I think we want to update the config
> parsing as well so that comment_line_str is reset to '#' when
> core.commentString=auto. We probably want to do that in its own commit.
>
> Thanks
>
> Phillip
>

> On Tue, Jul 1, 2025 at 6:47 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> The changes here look good but I think we want to update the config
>> parsing as well so that comment_line_str is reset to '#' when
>> core.commentString=auto. We probably want to do that in its own commit.
> 
> maybe something like this?
> 
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -912,8 +912,10 @@ static int prepare_to_commit(const char
> *index_file, const char *prefix,
>          if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
>                  die_errno(_("could not write commit template"));
> 
> -       if (auto_comment_line_char)
> +       if (auto_comment_line_char){
> +               comment_line_str = "#";
>                  adjust_comment_line_char(&sb);
> +       }
>          strbuf_release(&sb);
> 
> or we can do it inside the `adjust_comment_line()` function.

>
> Hi Ayush
>
> On 01/07/2025 19:33, Ayush Chandekar wrote:
> > On Tue, Jul 1, 2025 at 6:47 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >>
> >> The changes here look good but I think we want to update the config
> >> parsing as well so that comment_line_str is reset to '#' when
> >> core.commentString=auto. We probably want to do that in its own commit.
> >
> > maybe something like this?
> >
> > --- a/builtin/commit.c
> > +++ b/builtin/commit.c
> > @@ -912,8 +912,10 @@ static int prepare_to_commit(const char
> > *index_file, const char *prefix,
> >          if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
> >                  die_errno(_("could not write commit template"));
> >
> > -       if (auto_comment_line_char)
> > +       if (auto_comment_line_char){
> > +               comment_line_str = "#";
> >                  adjust_comment_line_char(&sb);
> > +       }
> >          strbuf_release(&sb);
> >
> > or we can do it inside the `adjust_comment_line()` function.
>
> We need to do it when we parse the config so that
> append_conflicts_comment() uses '#' as the comment char. See the
> (whitespace damaged) diff below
>
> Thanks
>
> Phillip
>
> diff --git a/config.c b/config.c
> index eb60c293ab3..bb75bdc65d3 100644
> --- a/config.c
> +++ b/config.c
> @@ -1537,9 +1537,11 @@ static int git_default_core_config(const char
> *var, const char *value,
>               !strcmp(var, "core.commentstring")) {
>                   if (!value)
>                           return config_error_nonbool(var);
> -                else if (!strcasecmp(value, "auto"))
> +                else if (!strcasecmp(value, "auto")) {
>                           auto_comment_line_char = 1;
> -                else if (value[0]) {
> +                        FREE_AND_NULL(comment_line_str_to_free);
> +                        comment_line_str = "#";
> +                } else if (value[0]) {
>                           if (strchr(value, '\n'))
>                                   return error(_("%s cannot contain
> newline"), var);
>                           comment_line_str = value;
>

> On Wed, Jul 2, 2025 at 1:02 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> diff --git a/config.c b/config.c
>> index eb60c293ab3..bb75bdc65d3 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -1537,9 +1537,11 @@ static int git_default_core_config(const char
>> *var, const char *value,
>>                !strcmp(var, "core.commentstring")) {
>>                    if (!value)
>>                            return config_error_nonbool(var);
>> -                else if (!strcasecmp(value, "auto"))
>> +                else if (!strcasecmp(value, "auto")) {
>>                            auto_comment_line_char = 1;
>> -                else if (value[0]) {
>> +                        FREE_AND_NULL(comment_line_str_to_free);
>> +                        comment_line_str = "#";
>> +                } else if (value[0]) {
>>                            if (strchr(value, '\n'))
>>                                    return error(_("%s cannot contain
>> newline"), var);
>>                            comment_line_str = value;
>>
> 
> Thanks, I understood it.
> 
> What if we simply return the function `adjust_comment_line_char()` if
> we get a non-zero value from `ignored_log_message_bytes()`, i.e we
> won't scan the commit message in case conflict message exists, and we
> let the old code exist as it is?
> 
> +       if(ignored_log_message_bytes(sb->buf, sb->len))
> +               return;

>
> Hi Ayush
>
> On 03/07/2025 00:46, Ayush Chandekar wrote:
> > On Wed, Jul 2, 2025 at 1:02 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >> diff --git a/config.c b/config.c
> >> index eb60c293ab3..bb75bdc65d3 100644
> >> --- a/config.c
> >> +++ b/config.c
> >> @@ -1537,9 +1537,11 @@ static int git_default_core_config(const char
> >> *var, const char *value,
> >>                !strcmp(var, "core.commentstring")) {
> >>                    if (!value)
> >>                            return config_error_nonbool(var);
> >> -                else if (!strcasecmp(value, "auto"))
> >> +                else if (!strcasecmp(value, "auto")) {
> >>                            auto_comment_line_char = 1;
> >> -                else if (value[0]) {
> >> +                        FREE_AND_NULL(comment_line_str_to_free);
> >> +                        comment_line_str = "#";
> >> +                } else if (value[0]) {
> >>                            if (strchr(value, '\n'))
> >>                                    return error(_("%s cannot contain
> >> newline"), var);
> >>                            comment_line_str = value;
> >>
> >
> > Thanks, I understood it.
> >
> > What if we simply return the function `adjust_comment_line_char()` if
> > we get a non-zero value from `ignored_log_message_bytes()`, i.e we
> > won't scan the commit message in case conflict message exists, and we
> > let the old code exist as it is?
> >
> > +       if(ignored_log_message_bytes(sb->buf, sb->len))
> > +               return;
>
> So we'd ignore core.commentChar=auto if we detected conflict comments?
> That might be surprising to the user - it would mean that we'd always
> avoid adding the conflict comments to the commit message but we'd lose
> any lines that begin with the comment string. I think I'm leaning
> slightly towards the original solution but it is not clear to me that
> one option is much better that the other.
>
> Thanks
>
> Phillip
>

> 
> Now that we're planning to get rid of the 'auto' keyword from
> commentChar [1], do you think it would be better if we just ignored
> the keyword when we detect conflict comments? Also, how is it that a
> user will end up having lines starting with the character being the
> same as the conflict comment's character?

> If conflict comments already use a comment character that isn't "#", and
> core.commentChar is set "auto", Git will ignore these lines during the
> scan using ignored_log_message_bytes() and pick a new comment character
> based on the rest of the message. The newly chosen character may be
> different from the one used in the conflict comments and therefore,
> these are no longer treated as comments and end up in the final commit
> message.
>
> For example, during a rebase if the user previously set
> core.commentChar=% and then encounters a conflict, conflict comments
> like "% Conflicts:" are generated. If the user subsequently sets
> core.commentChar=auto before running `rebase --continue`, Git parses the
> "auto" setting and begins scanning. It first uses the existing
> 'comment_line_str' (which is '%') to detect and ignore conflict comments
> via ignored_log_message_bytes().
>
> Then, Git scans the rest of the message (excluding conflict comments),
> sees that none of the remaining lines start with '#' and decides to set
> comment_line_str to '#'. Since the final commit character differs from
> the one used in the conflict comments, those lines are no longer
> considered comments and get included in the final commit message.
>
> Set 'comment_line_str' to '#' when core.commentChar is set to 'auto' to
> reset any previously set value.
>
> While this does not solve the issue of conflict comment inclusion and
> the user visible behaviour stays tha same, it standardizes the behaviour
> of the code by always resetting 'comment_line_str' to '#' when
> core.commentChar=auto is parsed.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
> ---
>  config.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
>
> diff --git a/config.c b/config.c
> index eb60c293ab..bb75bdc65d 100644
> --- a/config.c
> +++ b/config.c
> @@ -1537,9 +1537,11 @@ static int git_default_core_config(const char *var, const char *value,
>  	    !strcmp(var, "core.commentstring")) {
>  		if (!value)
>  			return config_error_nonbool(var);
> -		else if (!strcasecmp(value, "auto"))
> +		else if (!strcasecmp(value, "auto")) {
>  			auto_comment_line_char = 1;
> -		else if (value[0]) {
> +			FREE_AND_NULL(comment_line_str_to_free);
> +			comment_line_str = "#";
> +		} else if (value[0]) {
>  			if (strchr(value, '\n'))
>  				return error(_("%s cannot contain newline"), var);
>  			comment_line_str = value;

>
> This patch is exactly what Phillip suggested in
>
> https://lore.kernel.org/git/9e96aaab-79a2-4632-94cd-d016d4a63b30@gmail.com/
>
> isn't it?  Makes sense to me.
>

> On Wed, Jul 16, 2025 at 2:53 AM Junio C Hamano <gitster@pobox.com> wrote:
>>
> [snip]
>>
>> This patch is exactly what Phillip suggested in
>>
>> https://lore.kernel.org/git/9e96aaab-79a2-4632-94cd-d016d4a63b30@gmail.com/
>>
>> isn't it?  Makes sense to me.
>>
>
> Yes, you're right. I should add the suggested-by trailer for this patch.

>
> Ayush Chandekar <ayu.chandekar@gmail.com> writes:
>
> > On Wed, Jul 16, 2025 at 2:53 AM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> > [snip]
> >>
> >> This patch is exactly what Phillip suggested in
> >>
> >> https://lore.kernel.org/git/9e96aaab-79a2-4632-94cd-d016d4a63b30@gmail.com/
> >>
> >> isn't it?  Makes sense to me.
> >>
> >
> > Yes, you're right. I should add the suggested-by trailer for this patch.
>
> I am not sure about that, though.  A verbatim copy is stronger than
> implementing what was suggested by another person.  If I were in
> your position, I'll probably say something like
>
>         The patch text was taken from Phillip Wood's message [*URL*],
>         with the commit log message written by me.
>
>         Based-on-a-patch-by: Phillip Wood <...>
>         Signed-off-by: Ayush Chandekar <...>
>
> In any case, this overlaps both textually but also intent-wise with
> Phillip's "let's mark core.commentchar=auto deprecated and remove
> the support at 3.0 boundary", which is planned to be rerolled to
> make it a failure when the user uses core.commentchar=auto.  It
> would be a while before we tag Git 3.0, so the fix in this topic
> will be necessary until then.
>
> Thanks.
>

> Ayush Chandekar <ayu.chandekar@gmail.com> writes:
> 
>> Yeah, Phillip should actually get the primary credit for this patch
>> and Suggested-by does not do enough justice.
>> I will send a new version right away.
> 
> Thanks.  Don't forget to ask him to sign-off.

> On 16/07/2025 16:21, Junio C Hamano wrote:
>> Ayush Chandekar <ayu.chandekar@gmail.com> writes:
>> 
>>> Yeah, Phillip should actually get the primary credit for this patch
>>> and Suggested-by does not do enough justice.
>>> I will send a new version right away.
>> Thanks.  Don't forget to ask him to sign-off.
>
> Here it is
>
> Signed-off-by: <Phillip Wood phillip.wood@dunelm.org.uk>

> If conflict comments already use a comment character that isn't "#", and
> ...
> The patch text is based on Phillip Wood's message:
> https://lore.kernel.org/git/9e96aaab-79a2-4632-94cd-d016d4a63b30@gmail.com/
> and the commit log message is wriiten by me.
>
> Based-on-a-patch-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Ghanshyam Thakkar <shyamthakkar001@gmail.com>
> Signed-off-by: Ayush Chandekar <ayu.chandekar@gmail.com>
> ---

>  config.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/config.c b/config.c
> index eb60c293ab..bb75bdc65d 100644
> --- a/config.c
> +++ b/config.c
> @@ -1537,9 +1537,11 @@ static int git_default_core_config(const char *var, const char *value,
>  	    !strcmp(var, "core.commentstring")) {
>  		if (!value)
>  			return config_error_nonbool(var);
> -		else if (!strcasecmp(value, "auto"))
> +		else if (!strcasecmp(value, "auto")) {
>  			auto_comment_line_char = 1;
> -		else if (value[0]) {
> +			FREE_AND_NULL(comment_line_str_to_free);
> +			comment_line_str = "#";
> +		} else if (value[0]) {
>  			if (strchr(value, '\n'))
>  				return error(_("%s cannot contain newline"), var);
>  			comment_line_str = value;

> 
> Hey everyone,
> 
> The aim of this patch series is to improve the behaviour of core.commentChar=auto by the following patches:
> 1/2 - Fix a bug which reads comment character of the comments in commit message leading to change in the value of `comment_line_str` and thus resulting the comments in the final commit message.
> 2/2 - Standardizes the behaviour of code by resetting the 'comment_line_str' to "#" when 'core.commentChar' is set to "auto".

> Thanks to Junio, Phillip and Kristoffer for reviewing the patches and also Christian for the reviews and mentoring me.
> 
> The only difference between this version (v5) and the previous one is that I've added credit to Phillip for patch (2/2).
> 
> Ayush Chandekar (2):
>    commit: avoid scanning trailing comments when 'core.commentChar' is
>      "auto"
>    config: set comment_line_str to "#" when core.commentChar=auto
> 
>   builtin/commit.c           |  6 +++++-
>   config.c                   |  6 ++++--
>   t/t3418-rebase-continue.sh | 13 +++++++++++++
>   3 files changed, 22 insertions(+), 3 deletions(-)
>