Subject: Re: [PATCH v2 2/3] config: warn on core.commentString=auto

From: Phillip Wood <[email-masked]>
Date: Fri, 1 Aug 2025 11:37:24 +0100

Hi Junio

On 31/07/2025 22:17, Junio C Hamano wrote:
> Phillip Wood <[email-masked]> writes:
> 
>>   #include "git-compat-util.h"
>>   #include "abspath.h"
>> +#include "advice.h"
> 
> Hmph.  Do you still need this?

No, it's left over from splitting one patch into two. It should be in 
the next patch that adds a call to advise.

> I do not think a separate advice_if variable is warranted in this
> case.  They see a warning that says that their "auto" will not do
> anything useful in the future.  They will keep seeing it until they
> decide what to use, and once they decide and set a value that is
> different from "auto" to core.commentchar, they will stop seeing
> the warning.

I agree

>> +static const char* comment_key_name(unsigned id)
> 
> The asterisk sticks to the identifier, not type.

Oops I'll fix that

>> +
>> +	config->last_key_id = key_id;
>> +	config->auto_set = value && !strcmp(value, "auto");
>> +}
> 
> It probably becomes simpler (and easier to debug) if you made the
> type of .last_key_id member "const char *" to point at the variable
> name.  You are not switching on the .last_key_id member.  The only
> use of that member is to be fed to die().  And by doing so, you can
> drop comment_key_name().

I agree the integer id is pointless here, but it is useful in the next 
patch for tracking if a key occurs more than once in a given file.

>> +static void check_auto_comment_char_config(struct comment_char_config *config)
>> +{
>> +	extern bool warn_on_auto_comment_char;
>> +	const char *DEPRECATED_CONFIG_ENV =
>> +				"GIT_AUTO_COMMENT_CHAR_CONFIG_WARNING_GIVEN";
>> +
>> +	if (!config->auto_set || !warn_on_auto_comment_char)
>> +		return;
>> +
>> +	/*
>> +	 * Use an environment variable to ensure that subprocesses do not repeat
>> +	 * the warning.
>> +	 */
>> +	if (git_env_bool(DEPRECATED_CONFIG_ENV, false))
>> +		return;
>> +
>> +	setenv(DEPRECATED_CONFIG_ENV, "true", true);
> 
> I know this means well, but it might give users a better experience
> if we went a much simpler route.  In your top-level project with two
> submodules, you may have core.commentchar set to auto in the top-level
> and only one of the submodules, and then you let "git" go recursive.
> Wouldn't it be simpler for the user to diagnose which one(s) among
> the three repositories need fixing, if the stderr said something
> like:
> 
>      doing X
>      warning core.commentChar is set to auto
>      going into submodule A
>        doing X
>      going into submodule B
>        doing X
>        warning core.commentString is set to auto
> 
> I dunno.

There is definitely a trade off here. The motivation was to stop the 
sequencer printing the same warning every time it forked "git commit" 
and to stop each sumbodule warning about config set in the global config 
file. As you say the downside is that it hides warnings from a 
submodule's local config. I did wonder about somehow tracking the local 
config separately to the global and system config when setting the 
environment variable but it felt like it would get quite complicated 
tracking which .git/config we'd already warned about. We'd need to be 
clear about which repository the user needed to run 'git config' in as 
well which adds to the complications.

>> +static void check_deprecated_config(struct repo_config *config)
>> +{
>> +	if (!config->repo->check_deprecated_config)
>> +			return;
>> +
>> +	check_auto_comment_char_config(&config->comment_char_config);
> 
> The handling of .check_deprecated_config flag is a bit tricky, and
> it is great that this design allows us to write a similar
> check_foo_config() helper and make a call to it here, without
> having to worry about it again.

Thanks

Phillip