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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 27, 2025, 14:52 UTC
Message-ID: <xmqqms9t8cfd.fsf@gitster.g>
In-Reply-To: <91982162-b138-4bb1-81fd-6f9185801c99@gmail.com>

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 9 quoted lines

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

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

Excellent suggestion.

Show 7 quoted lines

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

Again, excellent.

> Thanks for fixing this
>
> Phillip

Thanks.