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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 31, 2025, 21:17 UTC
Message-ID: <xmqqa54koyb2.fsf@gitster.g>
In-Reply-To: <8b57598042642dd0c56e39be03c1c45a62accfb0.1753975294.git.phillip.wood@dunelm.org.uk>

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

Show 9 quoted lines

> diff --git a/config.c b/config.c
> index 97ffef42700..c36ead76005 100644
> --- a/config.c
> +++ b/config.c
> @@ -8,9 +8,11 @@
>  
>  #include "git-compat-util.h"
>  #include "abspath.h"
> +#include "advice.h"

Hmph.  Do you still need this?

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.

> +static const char* comment_key_name(unsigned id)

The asterisk sticks to the identifier, not type.

Show 13 quoted lines

> +static void comment_char_callback(const char *key, const char *value,
> +				  const struct config_context *ctx UNUSED,
> +				  void *data)
> +{
> +	struct comment_char_config *config = data;
> +	unsigned key_id;
> +
> +	if (!strcmp(key, "core.commentchar"))
> +		key_id = 0;
> +	else if (!strcmp(key, "core.commentstring"))
> +		key_id = 1;
> +	else
> +		return;

Yuck.  We cannot help the joy of last-one-wins here X-<.

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

Show 38 quoted lines

> +struct repo_config {
> +	struct repository *repo;
> +	struct comment_char_config comment_char_config;
> +};
> +
> +#define REPO_CONFIG_INIT(repo_) {				\
> +		.comment_char_config = COMMENT_CHAR_CFG_INIT,	\
> +		.repo = repo_,					\
> +	};
> +
> +#ifdef WITH_BREAKING_CHANGES
> +static void check_auto_comment_char_config(struct comment_char_config *config)
> +{
> +	if (!config->auto_set)
> +		return;
> +
> +	die_message(_("Support for '%s=auto' has been removed in Git 3.0"),
> +		    comment_key_name(config->last_key_id));
> +	die(NULL);
> +}
> +#else
> +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.

Show 11 quoted lines

> +	warning(_("Support for '%s=auto' is deprecated and will be removed in "
> +		  "Git 3.0"), comment_key_name(config->last_key_id));
> +}
> +#endif /* WITH_BREAKING_CHANGES */
> +
> +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.