config: warn on core.commentString=auto

As support for this setting was deprecated in the last commit print a
warning (or die when WITH_BREAKING_CHANGES is enabled) if it is set.
Avoid bombarding the user with warnings by only printing it (a) when
running commands that call "git commit" and (b) only once per command.

Some scaffolding is added to repo_read_config() to allow it to
detect deprecated config settings and warn about them. As both
"core.commentChar" and "core.commentString" set the comment
character we record which one of them is used and tailor the
warning message appropriately.

Note the odd combination of die_message() followed by die(NULL)
is to allow the next commit to insert a call to advise() in the middle.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/commit.c b/builtin/commit.c
index d25cc07a355aaa35d0192daab129a7c27ac0d2ba..f821fdcfcc35600589901d5ce3bae576185251da 100644 (file)
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1783,6 +1783,9 @@ int cmd_commit(int argc,
        show_usage_with_options_if_asked(argc, argv,
                                         builtin_commit_usage, builtin_commit_options);
 
+#ifndef WITH_BREAKING_CHANGES
+       warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
        prepare_repo_settings(the_repository);
        the_repository->settings.command_requires_full_index = 0;
 

diff --git a/builtin/merge.c b/builtin/merge.c
index dc4cb8fb14dbf314ecc3248ff27c4039101c7466..794cb7bb269eb1fa3707a94de31721ae9d857086 100644 (file)
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1378,6 +1378,9 @@ int cmd_merge(int argc,
        show_usage_with_options_if_asked(argc, argv,
                                         builtin_merge_usage, builtin_merge_options);
 
+#ifndef WITH_BREAKING_CHANGES
+       warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
        prepare_repo_settings(the_repository);
        the_repository->settings.command_requires_full_index = 0;
 

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 72a52bdfb9872e9b221ba2df1aa8cb6fa74541c8..962917ec48097a477f5b5b4470fbd5813414082b 100644 (file)
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1242,6 +1242,9 @@ int cmd_rebase(int argc,
                                         builtin_rebase_usage,
                                         builtin_rebase_options);
 
+#ifndef WITH_BREAKING_CHANGES
+       warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
        prepare_repo_settings(the_repository);
        the_repository->settings.command_requires_full_index = 0;
 

diff --git a/builtin/revert.c b/builtin/revert.c
index e07c2217fe846be86c482b1ef27ab078fb2d018a..b197848bb0a475968845abd5e843294dee6bc9cc 100644 (file)
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -4,6 +4,7 @@
 #include "builtin.h"
 #include "parse-options.h"
 #include "diff.h"
+#include "environment.h"
 #include "gettext.h"
 #include "revision.h"
 #include "rerere.h"
@@ -285,6 +286,9 @@ int cmd_revert(int argc,
        struct replay_opts opts = REPLAY_OPTS_INIT;
        int res;
 
+#ifndef WITH_BREAKING_CHANGES
+       warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
        opts.action = REPLAY_REVERT;
        sequencer_init_config(&opts);
        res = run_sequencer(argc, argv, prefix, &opts);
@@ -302,6 +306,9 @@ struct repository *repo UNUSED)
        struct replay_opts opts = REPLAY_OPTS_INIT;
        int res;
 
+#ifndef WITH_BREAKING_CHANGES
+       warn_on_auto_comment_char = true;
+#endif /* !WITH_BREAKING_CHANGES */
        opts.action = REPLAY_PICK;
        sequencer_init_config(&opts);
        res = run_sequencer(argc, argv, prefix, &opts);

diff --git a/config.c b/config.c
index 97ffef427001111eff159bb2ceee7557c3521f1e..18b42197095a0088c409b3df259d1f1ceec900a6 100644 (file)
--- a/config.c
+++ b/config.c
@@ -11,6 +11,7 @@
 #include "date.h"
 #include "branch.h"
 #include "config.h"
+#include "dir.h"
 #include "parse.h"
 #include "convert.h"
 #include "environment.h"
@@ -1951,10 +1952,110 @@ int git_configset_get_pathname(struct config_set *set, const char *key, char **d
                return 1;
 }
 
+struct comment_char_config {
+       unsigned last_key_id;
+       bool auto_set;
+};
+
+#define COMMENT_CHAR_CFG_INIT { 0 }
+
+static const char *comment_key_name(unsigned id)
+{
+       static const char *name[] = {
+               "core.commentChar",
+               "core.commentString",
+       };
+
+       if (id >= ARRAY_SIZE(name))
+               BUG("invalid comment key id");
+
+       return name[id];
+}
+
+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;
+
+       config->last_key_id = key_id;
+       config->auto_set = value && !strcmp(value, "auto");
+}
+
+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);
+
+       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);
+}
+
+static int repo_config_callback(const char *key, const char *value,
+                               const struct config_context *ctx, void *data)
+{
+       struct repo_config *config = data;
+
+       comment_char_callback(key, value, ctx, &config->comment_char_config);
+       return config_set_callback(key, value, ctx, config->repo->config);
+}
+
 /* Functions use to read configuration from a repository */
 static void repo_read_config(struct repository *repo)
 {
        struct config_options opts = { 0 };
+       struct repo_config config = REPO_CONFIG_INIT(repo);
 
        opts.respect_includes = 1;
        opts.commondir = repo->commondir;
@@ -1966,8 +2067,8 @@ static void repo_read_config(struct repository *repo)
                git_configset_clear(repo->config);
 
        git_configset_init(repo->config);
-       if (config_with_options(config_set_callback, repo->config, NULL,
-                               repo, &opts) < 0)
+       if (config_with_options(repo_config_callback, &config, NULL, repo,
+                               &opts) < 0)
                /*
                 * config_with_options() normally returns only
                 * zero, as most errors are fatal, and
@@ -1980,6 +2081,7 @@ static void repo_read_config(struct repository *repo)
                 * immediately.
                 */
                die(_("unknown error occurred while reading the configuration files"));
+       check_deprecated_config(&config);
 }
 
 static void git_config_check_init(struct repository *repo)
@@ -2667,6 +2769,14 @@ int repo_config_set_multivar_in_file_gently(struct repository *r,
        char *contents = NULL;
        size_t contents_sz;
        struct config_store_data store = CONFIG_STORE_INIT;
+       bool saved_check_deprecated_config = r->check_deprecated_config;
+
+       /*
+        * Do not warn or die if there are deprecated config settings as
+        * we want the user to be able to change those settings by running
+        * "git config".
+        */
+       r->check_deprecated_config = false;
 
        validate_comment_string(comment);
 
@@ -2898,6 +3008,7 @@ out_free:
        if (in_fd >= 0)
                close(in_fd);
        config_store_data_clear(&store);
+       r->check_deprecated_config = saved_check_deprecated_config;
        return ret;
 
 write_err_out:

diff --git a/environment.c b/environment.c
index 4c87876d483143221c001b9b4da42d3e8a3986b0..1ffa2ff30b2345b7d6bd40b7e803526d79176ae6 100644 (file)
--- a/environment.c
+++ b/environment.c
@@ -124,6 +124,7 @@ const char *comment_line_str = "#";
 char *comment_line_str_to_free;
 #ifndef WITH_BREAKING_CHANGES
 int auto_comment_line_char;
+bool warn_on_auto_comment_char;
 #endif /* !WITH_BREAKING_CHANGES */
 
 /* This is set by setup_git_directory_gently() and/or git_default_config() */

diff --git a/environment.h b/environment.h
index e75c4abb388670e951aff1fb617f77202c8b57d0..51898c99cd1e451a47c1a4aae32869cfbddbce45 100644 (file)
--- a/environment.h
+++ b/environment.h
@@ -210,6 +210,7 @@ extern const char *comment_line_str;
 extern char *comment_line_str_to_free;
 #ifndef WITH_BREAKING_CHANGES
 extern int auto_comment_line_char;
+extern bool warn_on_auto_comment_char;
 #endif /* !WITH_BREAKING_CHANGES */
 
 # endif /* USE_THE_REPOSITORY_VARIABLE */

diff --git a/repository.c b/repository.c
index ecd691181fc97d59241a53765d898300b1f9cf99..8af73923d344b9ab65ffe9cc455eb01115319e47 100644 (file)
--- a/repository.c
+++ b/repository.c
@@ -57,6 +57,7 @@ void initialize_repository(struct repository *repo)
        repo->parsed_objects = parsed_object_pool_new(repo);
        ALLOC_ARRAY(repo->index, 1);
        index_state_init(repo->index, repo);
+       repo->check_deprecated_config = true;
 
        /*
         * When a command runs inside a repository, it learns what

diff --git a/repository.h b/repository.h
index 042dc93f0f2f49d75db9db6eb51be8824151fa02..5808a5d610846a0e42233f66e56dcbcebbd3ecd0 100644 (file)
--- a/repository.h
+++ b/repository.h
@@ -161,6 +161,9 @@ struct repository {
 
        /* Indicate if a repository has a different 'commondir' from 'gitdir' */
        unsigned different_commondir:1;
+
+       /* Should repo_config() check for deprecated settings */
+       bool check_deprecated_config;
 };
 
 #ifdef USE_THE_REPOSITORY_VARIABLE

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index ce0aebb9a7ec7d6b207c33909f0b6c2dfd1f20c1..3b2a46c25ce69f70a711567aa5d3a13d835691c0 100755 (executable)
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1184,8 +1184,13 @@ test_expect_success !WITH_BREAKING_CHANGES 'rebase -i respects core.commentchar=
        test_when_finished "git rebase --abort || :" &&
        (
                test_set_editor "$(pwd)/copy-edit-script.sh" &&
-               git rebase -i HEAD^
+               git rebase -i HEAD^ 2>err
        ) &&
+       sed -n "s/^warning: //p" err >actual &&
+       cat >expect <<-EOF &&
+       Support for ${SQ}core.commentChar=auto${SQ} is deprecated and will be removed in Git 3.0
+       EOF
+       test_cmp expect actual &&
        test -z "$(grep -ve "^#" -e "^\$" -e "^pick" edit-script)"
 '
 

diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh
index 65b4519a715094fea989c9708e0932eac820fd4b..a9dc1e416d194730699f9bd5a6acf6b4d320f175 100755 (executable)
--- a/t/t7502-commit-porcelain.sh
+++ b/t/t7502-commit-porcelain.sh
@@ -958,7 +958,12 @@ test_expect_success 'commit --status with custom comment character' '
 
 test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar' '
        test_commit "#foo" foo &&
-       GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend &&
+       GIT_EDITOR=.git/FAKE_EDITOR git -c core.commentChar=auto commit --amend 2>err &&
+       sed -n "s/^warning: //p" err >actual &&
+       cat >expect <<-EOF &&
+       Support for ${SQ}core.commentChar=auto${SQ} is deprecated and will be removed in Git 3.0
+       EOF
+       test_cmp expect actual &&
        test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG
 '
 
@@ -982,4 +987,14 @@ EOF
        )
 '
 
+test_expect_success WITH_BREAKING_CHANGES 'core.commentChar=auto is rejected' '
+       test_config core.commentChar auto &&
+       test_must_fail git rev-parse --git-dir 2>err &&
+       sed -n "s/^fatal: //p" err >actual &&
+       cat >expect <<-EOF &&
+       Support for ${SQ}core.commentChar=auto${SQ} has been removed in Git 3.0
+       EOF
+       test_cmp expect actual
+'
+
 test_done