Phillip Wood <phillip.wood123@xxxxxxxxx> writes:
> 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.
> +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().
> +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.
> + 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.
