From fdae4114a696014b6bf28ad9b1bc076bd8d7eec8 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Tue, 26 Aug 2025 14:35:26 +0100 Subject: [PATCH 1/3] breaking-changes: deprecate support for core.commentString=auto When "core.commentString" is set to "auto" then "git commit" will automatically select the comment character ensuring that it is not the first character on any of the lines in the commit message. This was introduced by commit 84c9dc2c5a2 (commit: allow core.commentChar=auto for character auto selection, 2014-05-17). The motivation seems to be to avoid commenting out lines from the existing message when amending a commit that was created with a message from a file. Unfortunately this feature does not work with: * commit message templates that contain comments. * prepare-commit-msg hooks that introduce comments. * "git commit --cleanup=strip --edit -F " which means that it is incompatible with - the "fixup" and "squash" commands of "git rebase -i" as the comments added by those commands are then treated as part of the commit message. - the conflict comments added to the commit message by "git cherry-pick", "git rebase" etc. as these comments are then treated as part of the commit message. It is also ignored by "git notes" when amending a note. The issues with comments coming from a template, hook or file are a consequence of the design of this feature and are therefore hard to fix. As the costs of this feature outweigh the benefits, deprecate it and remove it in Git 3.0. If someone comes up with some patches that fix all the issues in a maintainable way then I'd be happy to see this change reverted. The next commits will add a warning and some advice for users on how they can update their config settings. Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- Documentation/BreakingChanges.adoc | 5 +++++ Documentation/config/core.adoc | 18 +++++++++++++++++- builtin/commit.c | 4 ++++ environment.c | 10 ++++++++-- environment.h | 2 ++ t/t3404-rebase-interactive.sh | 2 +- t/t3418-rebase-continue.sh | 2 +- t/t7502-commit-porcelain.sh | 4 ++-- 8 files changed, 40 insertions(+), 7 deletions(-) diff --git a/Documentation/BreakingChanges.adoc b/Documentation/BreakingChanges.adoc index f8d2eba061..344ce50060 100644 --- a/Documentation/BreakingChanges.adoc +++ b/Documentation/BreakingChanges.adoc @@ -239,6 +239,11 @@ These features will be removed. + The command will be removed. +* Support for `core.commentString=auto` has been deprecated and will + be removed in Git 3.0. ++ +cf. + == Superseded features that will not be deprecated Some features have gained newer replacements that aim to improve the design in diff --git a/Documentation/config/core.adoc b/Documentation/config/core.adoc index 9fde1ab63a..7133f00c38 100644 --- a/Documentation/config/core.adoc +++ b/Documentation/config/core.adoc @@ -531,9 +531,25 @@ core.commentString:: commented, and removes them after the editor returns (default '#'). + -If set to "auto", `git-commit` would select a character that is not +ifndef::with-breaking-changes[] +If set to "auto", `git-commit` will select a character that is not the beginning character of any line in existing commit messages. +Support for this value is deprecated and will be removed in Git 3.0 +due to the following limitations: + +-- +* It is incompatible with adding comments in a commit message + template. This includes the conflicts comments added to + the commit message by `cherry-pick`, `merge`, `rebase` and + `revert`. +* It is incompatible with adding comments to the commit message + in the `prepare-commit-msg` hook. +* It is incompatible with the `fixup` and `squash` commands when + rebasing, +* It is not respected by `git notes` +-- ++ +endif::with-breaking-changes[] Note that these two variables are aliases of each other, and in modern versions of Git you are free to use a string (e.g., `//` or `⁑⁕⁑`) with `commentChar`. Versions of Git prior to v2.45.0 will ignore diff --git a/builtin/commit.c b/builtin/commit.c index 757f51eac8..d25cc07a35 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -683,6 +683,7 @@ static int author_date_is_interesting(void) return author_message || force_date; } +#ifndef WITH_BREAKING_CHANGES static void adjust_comment_line_char(const struct strbuf *sb) { char candidates[] = "#;@!$%^&|:"; @@ -720,6 +721,7 @@ static void adjust_comment_line_char(const struct strbuf *sb) free(comment_line_str_to_free); comment_line_str = comment_line_str_to_free = xstrfmt("%c", *p); } +#endif /* !WITH_BREAKING_CHANGES */ static void prepare_amend_commit(struct commit *commit, struct strbuf *sb, struct pretty_print_context *ctx) @@ -916,8 +918,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix, if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len) die_errno(_("could not write commit template")); +#ifndef WITH_BREAKING_CHANGES if (auto_comment_line_char) adjust_comment_line_char(&sb); +#endif /* !WITH_BREAKING_CHANGES */ strbuf_release(&sb); /* This checks if committer ident is explicitly given */ diff --git a/environment.c b/environment.c index a0ac5934b3..4c87876d48 100644 --- a/environment.c +++ b/environment.c @@ -122,7 +122,9 @@ int protect_ntfs = PROTECT_NTFS_DEFAULT; */ const char *comment_line_str = "#"; char *comment_line_str_to_free; +#ifndef WITH_BREAKING_CHANGES int auto_comment_line_char; +#endif /* !WITH_BREAKING_CHANGES */ /* This is set by setup_git_directory_gently() and/or git_default_config() */ char *git_work_tree_cfg; @@ -459,18 +461,22 @@ static int git_default_core_config(const char *var, const char *value, if (!strcmp(var, "core.commentchar") || !strcmp(var, "core.commentstring")) { - if (!value) + if (!value) { return config_error_nonbool(var); - else if (!strcasecmp(value, "auto")) { +#ifndef WITH_BREAKING_CHANGES + } else if (!strcasecmp(value, "auto")) { auto_comment_line_char = 1; FREE_AND_NULL(comment_line_str_to_free); comment_line_str = "#"; +#endif /* !WITH_BREAKING_CHANGES */ } else if (value[0]) { if (strchr(value, '\n')) return error(_("%s cannot contain newline"), var); comment_line_str = value; FREE_AND_NULL(comment_line_str_to_free); +#ifndef WITH_BREAKING_CHANGES auto_comment_line_char = 0; +#endif /* !WITH_BREAKING_CHANGES */ } else return error(_("%s must have at least one character"), var); return 0; diff --git a/environment.h b/environment.h index 8cfce41015..e75c4abb38 100644 --- a/environment.h +++ b/environment.h @@ -208,7 +208,9 @@ extern char *excludes_file; */ extern const char *comment_line_str; extern char *comment_line_str_to_free; +#ifndef WITH_BREAKING_CHANGES extern int auto_comment_line_char; +#endif /* !WITH_BREAKING_CHANGES */ # endif /* USE_THE_REPOSITORY_VARIABLE */ #endif /* ENVIRONMENT_H */ diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 6bac217ed3..ce0aebb9a7 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1176,7 +1176,7 @@ test_expect_success 'rebase -i respects core.commentchar' ' test B = $(git cat-file commit HEAD^ | sed -ne \$p) ' -test_expect_success 'rebase -i respects core.commentchar=auto' ' +test_expect_success !WITH_BREAKING_CHANGES 'rebase -i respects core.commentchar=auto' ' test_config core.commentchar auto && write_script copy-edit-script.sh <<-\EOF && cp "$1" edit-script diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh index b8a8dd77e7..f9b8999db5 100755 --- a/t/t3418-rebase-continue.sh +++ b/t/t3418-rebase-continue.sh @@ -328,7 +328,7 @@ test_expect_success 'there is no --no-reschedule-failed-exec in an ongoing rebas test_expect_code 129 git rebase --edit-todo --no-reschedule-failed-exec ' -test_expect_success 'no change in comment character due to conflicts markers with core.commentChar=auto' ' +test_expect_success !WITH_BREAKING_CHANGES 'no change in comment character due to conflicts markers with core.commentChar=auto' ' git checkout -b branch-a && test_commit A F1 && git checkout -b branch-b HEAD^ && diff --git a/t/t7502-commit-porcelain.sh b/t/t7502-commit-porcelain.sh index b37e2018a7..65b4519a71 100755 --- a/t/t7502-commit-porcelain.sh +++ b/t/t7502-commit-porcelain.sh @@ -956,13 +956,13 @@ test_expect_success 'commit --status with custom comment character' ' test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG ' -test_expect_success 'switch core.commentchar' ' +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 && test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG ' -test_expect_success 'switch core.commentchar but out of options' ' +test_expect_success !WITH_BREAKING_CHANGES 'switch core.commentchar but out of options' ' cat >text <<\EOF && # 1 ; 2 From a0e6aaea7da5134bdc784c6d68d4cc2125865330 Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Tue, 26 Aug 2025 14:35:27 +0100 Subject: [PATCH 2/3] 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 Signed-off-by: Junio C Hamano --- builtin/commit.c | 3 + builtin/merge.c | 3 + builtin/rebase.c | 3 + builtin/revert.c | 7 +++ config.c | 115 +++++++++++++++++++++++++++++++++- environment.c | 1 + environment.h | 1 + repository.c | 1 + repository.h | 3 + t/t3404-rebase-interactive.sh | 7 ++- t/t7502-commit-porcelain.sh | 17 ++++- 11 files changed, 157 insertions(+), 4 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index d25cc07a35..f821fdcfcc 100644 --- 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 dc4cb8fb14..794cb7bb26 100644 --- 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 72a52bdfb9..962917ec48 100644 --- 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 e07c2217fe..b197848bb0 100644 --- 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 97ffef4270..18b4219709 100644 --- 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 4c87876d48..1ffa2ff30b 100644 --- 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 e75c4abb38..51898c99cd 100644 --- 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 ecd691181f..8af73923d3 100644 --- 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 042dc93f0f..5808a5d610 100644 --- 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 ce0aebb9a7..3b2a46c25c 100755 --- 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 65b4519a71..a9dc1e416d 100755 --- 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 From ace1bb71503bc53b42ddfd68435c3af0adaf390f Mon Sep 17 00:00:00 2001 From: Phillip Wood Date: Tue, 26 Aug 2025 14:35:28 +0100 Subject: [PATCH 3/3] commit: print advice when core.commentString=auto Add some advice on how to change the config settings when "core.commentString=auto" or "core.commentChar=auto". The advice includes instructions for clearing the config setting or setting a fixed comment string. To try and be as specific as possible, the advice is customized based on the user's config. If "core.commentString=auto" is set in the system config and the user does not have write access then the advice omits the instructions to clear the config and recommends changing the global config instead. An alternative approach would be to advise the user to run "git config --show-origin" and leave them to figure out how to fix it themselves but that seems rather unfriendly. As we're forcing them to update their config we should try and make that as easy as possible. In order to generate this advice we need to record each file where either of the config keys is set and whether a key occurs more that once in a given file. This lets us generate the list of commands to remove all the keys and also tells us which key the "auto" setting comes from. As we want the user to update their config we do not provide a way for this advice to be disabled other than changing the value of "core.commentChar" or "core.commentString". Signed-off-by: Phillip Wood Signed-off-by: Junio C Hamano --- config.c | 194 ++++++++++++++++++++++++++++++++-- t/t3404-rebase-interactive.sh | 12 ++- t/t7502-commit-porcelain.sh | 37 ++++++- 3 files changed, 233 insertions(+), 10 deletions(-) diff --git a/config.c b/config.c index 18b4219709..18dcf341d5 100644 --- a/config.c +++ b/config.c @@ -8,6 +8,7 @@ #include "git-compat-util.h" #include "abspath.h" +#include "advice.h" #include "date.h" #include "branch.h" #include "config.h" @@ -1955,9 +1956,51 @@ int git_configset_get_pathname(struct config_set *set, const char *key, char **d struct comment_char_config { unsigned last_key_id; bool auto_set; + bool auto_set_in_file; + struct strintmap key_flags; + size_t alloc, nr; + struct comment_char_config_item { + unsigned key_id; + char *path; + enum config_scope scope; + } *item; }; -#define COMMENT_CHAR_CFG_INIT { 0 } +#define COMMENT_CHAR_CFG_INIT { \ + .key_flags = STRINTMAP_INIT, \ + } + +static void comment_char_config_release(struct comment_char_config *config) +{ + strintmap_clear(&config->key_flags); + for (size_t i = 0; i < config->nr; i++) + free(config->item[i].path); + free(config->item); +} + +/* Used to track whether the key occurs more than once in a given file */ +#define KEY_SEEN_ONCE 1u +#define KEY_SEEN_TWICE 2u +#define COMMENT_KEY_SHIFT(id) (2 * (id)) +#define COMMENT_KEY_MASK(id) (3u << COMMENT_KEY_SHIFT(id)) + +static void set_comment_key_flags(struct comment_char_config *config, + const char *path, unsigned id, unsigned value) +{ + unsigned old = strintmap_get(&config->key_flags, path); + unsigned new = (old & ~COMMENT_KEY_MASK(id)) | + value << COMMENT_KEY_SHIFT(id); + + strintmap_set(&config->key_flags, path, new); +} + +static unsigned get_comment_key_flags(struct comment_char_config *config, + const char *path, unsigned id) +{ + unsigned value = strintmap_get(&config->key_flags, path); + + return (value & COMMENT_KEY_MASK(id)) >> COMMENT_KEY_SHIFT(id); +} static const char *comment_key_name(unsigned id) { @@ -1973,10 +2016,10 @@ static const char *comment_key_name(unsigned id) } static void comment_char_callback(const char *key, const char *value, - const struct config_context *ctx UNUSED, - void *data) + const struct config_context *ctx, void *data) { struct comment_char_config *config = data; + const struct key_value_info *kvi = ctx->kvi; unsigned key_id; if (!strcmp(key, "core.commentchar")) @@ -1988,8 +2031,136 @@ static void comment_char_callback(const char *key, const char *value, config->last_key_id = key_id; config->auto_set = value && !strcmp(value, "auto"); + if (kvi->origin_type != CONFIG_ORIGIN_FILE) { + return; + } else if (get_comment_key_flags(config, kvi->filename, key_id)) { + set_comment_key_flags(config, kvi->filename, key_id, + KEY_SEEN_TWICE); + } else { + struct comment_char_config_item *item; + + ALLOC_GROW_BY(config->item, config->nr, 1, config->alloc); + item = &config->item[config->nr - 1]; + item->key_id = key_id; + item->scope = kvi->scope; + item->path = xstrdup(kvi->filename); + set_comment_key_flags(config, kvi->filename, key_id, + KEY_SEEN_ONCE); + } + config->auto_set_in_file = config->auto_set; } +static void add_config_scope_arg(struct repository *repo, struct strbuf *buf, + struct comment_char_config_item *item) +{ + char *global_config = git_global_config(); + char *system_config = git_system_config(); + + if (item->scope == CONFIG_SCOPE_SYSTEM && access(item->path, W_OK)) { + /* + * If the user cannot write to the system config recommend + * setting the global config instead. + */ + strbuf_addstr(buf, "--global "); + } else if (fspatheq(item->path, system_config)) { + strbuf_addstr(buf, "--system "); + } else if (fspatheq(item->path, global_config)) { + strbuf_addstr(buf, "--global "); + } else if (fspatheq(item->path, + mkpath("%s/config", + repo_get_git_dir(repo)))) { + ; /* --local is the default */ + } else if (fspatheq(item->path, + mkpath("%s/config.worktree", + repo_get_common_dir(repo)))) { + strbuf_addstr(buf, "--worktree "); + } else { + const char *path = item->path; + const char *home = getenv("HOME"); + + strbuf_addstr(buf, "--file "); + if (home && !fspathncmp(path, home, strlen(home))) { + path += strlen(home); + if (!fspathncmp(path, "/", 1)) + path++; + strbuf_addstr(buf, "~/"); + } + sq_quote_buf_pretty(buf, path); + strbuf_addch(buf, ' '); + } + + free(global_config); + free(system_config); +} + +static bool can_unset_comment_char_config(struct comment_char_config *config) +{ + for (size_t i = 0; i < config->nr; i++) { + struct comment_char_config_item *item = &config->item[i]; + + if (item->scope == CONFIG_SCOPE_SYSTEM && + access(item->path, W_OK)) + return false; + } + + return true; +} + +static void add_unset_auto_comment_char_advice(struct repository *repo, + struct comment_char_config *config) +{ + struct strbuf buf = STRBUF_INIT; + + if (!can_unset_comment_char_config(config)) + return; + + for (size_t i = 0; i < config->nr; i++) { + struct comment_char_config_item *item = &config->item[i]; + + strbuf_addstr(&buf, " git config unset "); + add_config_scope_arg(repo, &buf, item); + if (get_comment_key_flags(config, item->path, item->key_id) == KEY_SEEN_TWICE) + strbuf_addstr(&buf, "--all "); + strbuf_addf(&buf, "%s\n", comment_key_name(item->key_id)); + } + advise(_("\nTo use the default comment string (#) please run\n\n%s"), + buf.buf); + strbuf_release(&buf); +} + +static void add_comment_char_advice(struct repository *repo, + struct comment_char_config *config) +{ + struct strbuf buf = STRBUF_INIT; + struct comment_char_config_item *item; + /* TRANSLATORS this is a place holder for the value of core.commentString */ + const char *placeholder = _(""); + + /* + * If auto is set in the last file that we saw advise the user how to + * update their config. + */ + if (!config->auto_set_in_file) + return; + + add_unset_auto_comment_char_advice(repo, config); + item = &config->item[config->nr - 1]; + strbuf_reset(&buf); + strbuf_addstr(&buf, " git config set "); + add_config_scope_arg(repo, &buf, item); + strbuf_addf(&buf, "%s %s\n", comment_key_name(item->key_id), + placeholder); + advise(_("\nTo set a custom comment string please run\n\n" + "%s\nwhere '%s' is the string you wish to use.\n"), + buf.buf, placeholder); + strbuf_release(&buf); +} + +#undef KEY_SEEN_ONCE +#undef KEY_SEEN_TWICE +#undef COMMENT_KEY_SHIFT +#undef COMMENT_KEY_MASK + struct repo_config { struct repository *repo; struct comment_char_config comment_char_config; @@ -2000,18 +2171,26 @@ struct repo_config { .repo = repo_, \ }; +static void repo_config_release(struct repo_config *config) +{ + comment_char_config_release(&config->comment_char_config); +} + #ifdef WITH_BREAKING_CHANGES -static void check_auto_comment_char_config(struct comment_char_config *config) +static void check_auto_comment_char_config(struct repository *repo, + 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)); + add_comment_char_advice(repo, config); die(NULL); } #else -static void check_auto_comment_char_config(struct comment_char_config *config) +static void check_auto_comment_char_config(struct repository *repo, + struct comment_char_config *config) { extern bool warn_on_auto_comment_char; const char *DEPRECATED_CONFIG_ENV = @@ -2031,6 +2210,7 @@ static void check_auto_comment_char_config(struct comment_char_config *config) warning(_("Support for '%s=auto' is deprecated and will be removed in " "Git 3.0"), comment_key_name(config->last_key_id)); + add_comment_char_advice(repo, config); } #endif /* WITH_BREAKING_CHANGES */ @@ -2039,7 +2219,8 @@ 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); + check_auto_comment_char_config(config->repo, + &config->comment_char_config); } static int repo_config_callback(const char *key, const char *value, @@ -2082,6 +2263,7 @@ static void repo_read_config(struct repository *repo) */ die(_("unknown error occurred while reading the configuration files")); check_deprecated_config(&config); + repo_config_release(&config); } static void git_config_check_init(struct repository *repo) diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 3b2a46c25c..cc97628d81 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1186,9 +1186,19 @@ test_expect_success !WITH_BREAKING_CHANGES 'rebase -i respects core.commentchar= test_set_editor "$(pwd)/copy-edit-script.sh" && git rebase -i HEAD^ 2>err ) && - sed -n "s/^warning: //p" err >actual && + sed -n "s/^hint: *\$//p; s/^hint: //p; 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 + + To use the default comment string (#) please run + + git config unset core.commentChar + + To set a custom comment string please run + + git config set core.commentChar + + where ${SQ}${SQ} is the string you wish to use. 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 a9dc1e416d..05f6da4ad9 100755 --- a/t/t7502-commit-porcelain.sh +++ b/t/t7502-commit-porcelain.sh @@ -958,10 +958,31 @@ 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 2>err && - sed -n "s/^warning: //p" err >actual && + cat >config-include <<-\EOF && + [core] + commentString=: + commentString=% + commentChar=auto + EOF + test_when_finished "rm config-include" && + test_config include.path "$(pwd)/config-include" && + test_config core.commentChar ! && + GIT_EDITOR=.git/FAKE_EDITOR git commit --amend 2>err && + sed -n "s/^hint: *\$//p; s/^hint: //p; 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 + + To use the default comment string (#) please run + + git config unset core.commentChar + git config unset --file ~/config-include --all core.commentString + git config unset --file ~/config-include core.commentChar + + To set a custom comment string please run + + git config set --file ~/config-include core.commentChar + + where ${SQ}${SQ} is the string you wish to use. EOF test_cmp expect actual && test_grep "^; Changes to be committed:" .git/COMMIT_EDITMSG @@ -990,9 +1011,19 @@ 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 && + sed -n "s/^hint: *\$//p; s/^hint: //p; s/^fatal: //p" err >actual && cat >expect <<-EOF && Support for ${SQ}core.commentChar=auto${SQ} has been removed in Git 3.0 + + To use the default comment string (#) please run + + git config unset core.commentChar + + To set a custom comment string please run + + git config set core.commentChar + + where ${SQ}${SQ} is the string you wish to use. EOF test_cmp expect actual '