diff options
| -rw-r--r-- | Documentation/BreakingChanges.adoc | 5 | ||||
| -rw-r--r-- | Documentation/config/core.adoc | 20 | ||||
| -rw-r--r-- | builtin/commit.c | 7 | ||||
| -rw-r--r-- | builtin/merge.c | 3 | ||||
| -rw-r--r-- | builtin/rebase.c | 3 | ||||
| -rw-r--r-- | builtin/revert.c | 7 | ||||
| -rw-r--r-- | config.c | 297 | ||||
| -rw-r--r-- | environment.c | 15 | ||||
| -rw-r--r-- | environment.h | 3 | ||||
| -rw-r--r-- | repository.c | 1 | ||||
| -rw-r--r-- | repository.h | 3 | ||||
| -rwxr-xr-x | t/t3404-rebase-interactive.sh | 19 | ||||
| -rwxr-xr-x | t/t3418-rebase-continue.sh | 2 | ||||
| -rwxr-xr-x | t/t7502-commit-porcelain.sh | 52 |
14 files changed, 424 insertions, 13 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. <xmqqa59i45wc.fsf@gitster.g> + == 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 3fbe83eef1..08739bb9d4 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 8a5dee384d..384d0b4e93 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -695,6 +695,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[] = "#;@!$%^&|:"; @@ -732,6 +733,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) @@ -928,8 +930,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 */ @@ -1793,6 +1797,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 b235af730a..c421a11b0b 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1379,6 +1379,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 3c85768d29..67c0352bf8 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 c3f92b585d..bedc40f368 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); @@ -8,9 +8,11 @@ #include "git-compat-util.h" #include "abspath.h" +#include "advice.h" #include "date.h" #include "branch.h" #include "config.h" +#include "dir.h" #include "parse.h" #include "convert.h" #include "environment.h" @@ -1948,10 +1950,290 @@ 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; + 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 { \ + .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) +{ + 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, void *data) +{ + struct comment_char_config *config = data; + const struct key_value_info *kvi = ctx->kvi; + 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"); + 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 = _("<comment string>"); + + /* + * 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; +}; + +#define REPO_CONFIG_INIT(repo_) { \ + .comment_char_config = COMMENT_CHAR_CFG_INIT, \ + .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 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 repository *repo, + 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)); + add_comment_char_advice(repo, config); +} +#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->repo, + &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; @@ -1963,8 +2245,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 @@ -1977,6 +2259,8 @@ static void repo_read_config(struct repository *repo) * immediately. */ 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) @@ -2664,6 +2948,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); @@ -2895,6 +3187,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 0e72fdac85..a770b5921d 100644 --- a/environment.c +++ b/environment.c @@ -121,7 +121,10 @@ 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; +bool warn_on_auto_comment_char; +#endif /* !WITH_BREAKING_CHANGES */ /* This is set by setup_git_directory_gently() and/or git_default_config() */ char *git_work_tree_cfg; @@ -463,16 +466,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; - else if (value[0]) { + 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..51898c99cd 100644 --- a/environment.h +++ b/environment.h @@ -208,7 +208,10 @@ 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; +extern bool warn_on_auto_comment_char; +#endif /* !WITH_BREAKING_CHANGES */ # endif /* USE_THE_REPOSITORY_VARIABLE */ #endif /* ENVIRONMENT_H */ diff --git a/repository.c b/repository.c index 97f0578381..6faf5c7398 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 34d6ad0770..e778dd8ae4 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 @@ -1184,8 +1184,23 @@ test_expect_success 'rebase -i respects core.commentchar=auto' ' 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/^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 <comment string> + + where ${SQ}<comment string>${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/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..05f6da4ad9 100755 --- a/t/t7502-commit-porcelain.sh +++ b/t/t7502-commit-porcelain.sh @@ -956,13 +956,39 @@ 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 && + 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 <comment string> + + where ${SQ}<comment string>${SQ} is the string you wish to use. + EOF + test_cmp expect actual && 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 @@ -982,4 +1008,24 @@ 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/^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 <comment string> + + where ${SQ}<comment string>${SQ} is the string you wish to use. + EOF + test_cmp expect actual +' + test_done |