| Message ID | pull.1681.v3.git.1710280020508.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 42d5c033945e4fc41d7268bfe4284d37651986b8 |
| Headers | show |
| Series | [v3] config: add --comment option to add a comment | expand |
"Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Ralph Seichter <github@seichter.de> Thanks for updating. The proposed commit log message reads much smoother. > Users need to be able to distinguish between config entries made > using automation and entries made by a human. Automation can add > comments containing a URL pointing to explanations for the change > made, avoiding questions from users as to why their config file > was changed by a third party. While I can understand what you want to say, primarily because we were both on this topic for some time by now, a casual reader of the above would say: Sure, but a human also would add comment to clarify what motivated the setting to be added. The fact that one entry has comment does not help distinguishing between automation and human input all that much. More importantly, the reason why users would want to mark entries is not limited to telling automation vs human. So, it seems to me that the paragraph needs a bit more work. Here is my attempt. While the subcommands (including "git config") of "git" all can skip comment strings in the configuration files, and users do write comments in the configuration files with editors, there is no easy way that can be used by scripts and automation to leave comments when they add/modify configuration entries. Introduce a "--comment" option that can be used with the "git config" command when it is used for writing, so that each affected entry can be annotated with a comment that comes after the "variable = value" on the same line. The option argument is deliberately limited to a single-line comment to match the line oriented nature of the configuration file, and "git config" will error out early when fed a multi-line string. This way, removal of the variable will also remove the comment given to it by this mechanism. > The implementation ensures that a # character is unconditionally > prepended to the provided comment string, and that I thought we already discussed this and unconditional "#comment" has already been declared a non starter. I'll follow this review response with a few patches that are designed to apply on top to show a more ergonomic way to do this. > the comment > text is appended as a suffix to the changed key-value-pair in the > same line of text. Multi-line comments (i.e. comments containing > linefeed) are rejected as errors, causing Git to exit without > making changes. These will become redundant if we rewrite the earlier paragraph like illustrated above. > Comments are aimed at humans who inspect or change their Git > config using a pager or editor. Comments are not meant to be > read or displayed by git-config at a later time. As we sold why comments are useful and lack of mechanical way to add one hurts earlier in the proposed log message, if we rewrite the earlier paragraph like illustrated above, this would become redundant. I also do not think we need to limit future direction with the last sentence. I do not foresee a reason to forbid other developers from adding an option to the "get" subcommand, e.g. $ git config --get --with-comments vari.able vari.able = value # added on Fri Mar 15 15:02:37 2024 > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt > index dff39093b5e..e608d5ffef2 100644 > --- a/Documentation/git-config.txt > +++ b/Documentation/git-config.txt > @@ -9,9 +9,9 @@ git-config - Get and set repository or global options > SYNOPSIS > -------- > [verse] > -'git config' [<file-option>] [--type=<type>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]] > -'git config' [<file-option>] [--type=<type>] --add <name> <value> > -'git config' [<file-option>] [--type=<type>] [--fixed-value] --replace-all <name> <value> [<value-pattern>] > +'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]] > +'git config' [<file-option>] [--type=<type>] [--comment=<value>] --add <name> <value> > +'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] --replace-all <name> <value> [<value-pattern>] > 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get <name> [<value-pattern>] > 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all <name> [<value-pattern>] > 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp <name-regex> [<value-pattern>] "--comment=<value>" will become awkward when we write about how the comment message is placed after the value in the description. You'll see, in the patch I promised to follow this review with, I've used --comment=<message> instead to avoid that issue. > @@ -87,6 +87,11 @@ OPTIONS > values. This is the same as providing '^$' as the `value-pattern` > in `--replace-all`. > > +--comment <value>:: > + Append a comment to new or modified lines. A '#' character will be > + unconditionally prepended to the value. The value must not contain > + linefeed characters (no multi-line comments are permitted). About the "unconditional" part I won't repeat. As to the formatting style, the recent trend is to write placeholder emphasized and inside angle bracket, e.g. --comment <message>:: Append a comment _<message>_ after the value on the same line. ... This illustration is only to show the formatting, and not what the description says. > [section] > - penguin = very blue > Movie = BadPhysics > UPPERCASE = true > - penguin = kingpin > + penguin = gentoo #Pygoscelis papua > + disposition = peckish #find fish > + foo = bar ## abc The first one, "very blue", will be gone, which I found a bit surprising, but it is OK. This is because "--replace-all" used first will eat all the "penguin" and leaves only a single one. > [Sections] > WhatEver = Second > EOF > +test_expect_success 'append comments' ' > + git config --replace-all --comment="Pygoscelis papua" section.penguin gentoo && > + git config --comment="find fish" section.disposition peckish && > + git config --comment="# abc" section.foo bar && > + test_cmp expect .git/config > +' Notice that none of these would produce the most natural (at least in the context of Git command set) variable = value # comment You would have to say --comment=" comment" which is rather unfortunate. -------- >8 --------------- >8 --------------- >8 --------------- >8 -------- From: Junio C Hamano <gitster@pobox.com> Date: Fri, 15 Mar 2024 12:43:58 -0700 Subject: [PATCH] config: fix --comment formatting When git adds comments itself (like "rebase -i" todo list and "commit -e" log message editor), it always gives a comment introducer "#" followed by a Space before the message, except for the recently introduced "git config --comment", where the users are forced to say " this is my comment" if they want to add their comment in this usual format; otherwise their comment string will end up without a space after the "#". Make it more ergonomic, while keeping it possible to also use this unusual style, by massaging the comment string at the UI layer with a set of simple rules: * If the given comment string begins with '#', it is passed intact. * Otherwise, "# " is prefixed. * A string with LF in it cannot be used as a comment string. Right now there is only one "front-end" that accepts end-user comment string and calls the underlying machinery to add or modify configuration file with comments, but to make sure that the future callers perform similar massaging as they see fit, add a sanity check logic in git_config_set_multivar_in_file_gently(), which is the single choke point in the codepaths that consumes the comment string. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Even with this fix, the code unconditionally places a single Space character after the value before the '#' comment introducer. If we wanted to allow it to be replaced with other kind of whitespaces, we could tweak the massaging logic further. The updated tweaking rule may become * If the given comment string begins with one or more whitespace characters followed by '#', it is passed intact. * If the given comment string begins with '#', then a SP is prefixed. * Otherwise, " # " (Space, '#', Space) is prefixed. * A string with LF in it cannot be used as a comment string. And the sanity checking rule may become * comment string after the above massaging that comes into git_config_set_multivar_in_file_gently() must - begin with one or more whitespace characters followed by '#'. - not have a LF in it. Finally the code to add the massaged comment will simply become: strbuf_addf(&sb, "%s%s\n", quote, comment); I personally think that is over-engineered, so this commit stops short of doing any of that. If we were to do so, the tweaking logic must be encapsulated into a helper function, so that the second and subsequent "front-end" can reuse it. Documentation/git-config.txt | 15 ++++++++------- builtin/config.c | 15 +++++++++++---- config.c | 20 ++++++++++++++------ t/t1300-config.sh | 9 +++++---- 4 files changed, 38 insertions(+), 21 deletions(-) diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index e608d5ffef..af374ee2e0 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -9,9 +9,9 @@ git-config - Get and set repository or global options SYNOPSIS -------- [verse] -'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]] -'git config' [<file-option>] [--type=<type>] [--comment=<value>] --add <name> <value> -'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] --replace-all <name> <value> [<value-pattern>] +'git config' [<file-option>] [--type=<type>] [--comment=<message>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]] +'git config' [<file-option>] [--type=<type>] [--comment=<message>] --add <name> <value> +'git config' [<file-option>] [--type=<type>] [--comment=<message>] [--fixed-value] --replace-all <name> <value> [<value-pattern>] 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get <name> [<value-pattern>] 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all <name> [<value-pattern>] 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp <name-regex> [<value-pattern>] @@ -87,10 +87,11 @@ OPTIONS values. This is the same as providing '^$' as the `value-pattern` in `--replace-all`. ---comment <value>:: - Append a comment to new or modified lines. A '#' character will be - unconditionally prepended to the value. The value must not contain - linefeed characters (no multi-line comments are permitted). +--comment <message>:: + Append a comment at the end of new or modified lines. + Unless _<message>_ begins with "#", a string "# " (hash + followed by a space) is prepended to it. The _<message>_ must not + contain linefeed characters (no multi-line comments are permitted). --get:: Get the value for a given key (optionally filtered by a regex diff --git a/builtin/config.c b/builtin/config.c index c54e9941a5..e859a659f4 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -174,7 +174,7 @@ static struct option builtin_config_options[] = { OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")), OPT_BOOL(0, "show-scope", &show_scope, N_("show scope of config (worktree, local, global, system, command)")), OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")), - OPT_STRING(0, "comment", &comment, N_("value"), N_("human-readable comment string (# will be prepended automatically)")), + OPT_STRING(0, "comment", &comment, N_("value"), N_("human-readable comment string (# will be prepended as needed)")), OPT_END(), }; @@ -800,9 +800,9 @@ int cmd_config(int argc, const char **argv, const char *prefix) } if (comment && - !(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) { - error(_("--comment is only applicable to add/set/replace operations")); - usage_builtin_config(); + !(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) { + error(_("--comment is only applicable to add/set/replace operations")); + usage_builtin_config(); } /* check usage of --fixed-value */ @@ -841,6 +841,13 @@ int cmd_config(int argc, const char **argv, const char *prefix) flags |= CONFIG_FLAGS_FIXED_VALUE; } + if (comment) { + if (strchr(comment, '\n')) + die(_("no multi-line comment allowed: '%s'"), comment); + if (comment[0] != '#') + comment = xstrfmt("# %s", comment); + } + if (actions & PAGING_ACTIONS) setup_auto_pager("config", 1); diff --git a/config.c b/config.c index 2f3f6d123c..15019cb9a5 100644 --- a/config.c +++ b/config.c @@ -3043,12 +3043,9 @@ static ssize_t write_pair(int fd, const char *key, const char *value, break; } - if (comment) { - if (strchr(comment, '\n')) - die(_("multi-line comments are not permitted: '%s'"), comment); - else - strbuf_addf(&sb, "%s #%s\n", quote, comment); - } else + if (comment) + strbuf_addf(&sb, "%s %s\n", quote, comment); + else strbuf_addf(&sb, "%s\n", quote); ret = write_in_full(fd, sb.buf, sb.len); @@ -3214,6 +3211,17 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, size_t contents_sz; struct config_store_data store = CONFIG_STORE_INIT; + if (comment) { + /* + * The front-end must have massaged the comment string + * properly before calling us. + */ + if (strchr(comment, '\n')) + BUG("multi-line comments are not permitted: '%s'", comment); + if (comment[0] != '#') + BUG("comment should begin with '#': '%s'", comment); + } + /* parse-key returns negative; flip the sign to feed exit(3) */ ret = 0 - git_config_parse_key(key, &store.key, &store.baselen); if (ret) diff --git a/t/t1300-config.sh b/t/t1300-config.sh index ac7b02e6b0..d5dfb45877 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -71,16 +71,17 @@ cat > expect << EOF [section] Movie = BadPhysics UPPERCASE = true - penguin = gentoo #Pygoscelis papua - disposition = peckish #find fish - foo = bar ## abc + penguin = gentoo # Pygoscelis papua + disposition = peckish # find fish + foo = bar #abc [Sections] WhatEver = Second EOF + test_expect_success 'append comments' ' git config --replace-all --comment="Pygoscelis papua" section.penguin gentoo && git config --comment="find fish" section.disposition peckish && - git config --comment="# abc" section.foo bar && + git config --comment="#abc" section.foo bar && test_cmp expect .git/config '
On Fri, Mar 15, 2024 at 6:19 PM Junio C Hamano <gitster@pobox.com> wrote: > ---comment <value>:: > - Append a comment to new or modified lines. A '#' character will be > - unconditionally prepended to the value. The value must not contain > - linefeed characters (no multi-line comments are permitted). > +--comment <message>:: > + Append a comment at the end of new or modified lines. > + Unless _<message>_ begins with "#", a string "# " (hash > + followed by a space) is prepended to it. The _<message>_ must not > + contain linefeed characters (no multi-line comments are permitted). In an earlier review, you wrote about the potential difficulties and issues surrounding comments other than the simple "inline" comments tackled by this patch. Do we foresee a future in which git-config may be extended to handle automation of comments other than inline? If so, do we really want to lock ourselves into having the generic option `--comment=<message>` be restricted to only inline comments? Or would it be better to plan for the future by instead having some sort of annotation which indicates that we are requesting (or dealing with) only inline comments, such as `--inline-comment=<message>` or `--comment=inline:<message>`?
Eric Sunshine <sunshine@sunshineco.com> writes: > tackled by this patch. Do we foresee a future in which git-config may > be extended to handle automation of comments other than inline? If so, > do we really want to lock ourselves into having the generic option > `--comment=<message>` be restricted to only inline comments? Or would > it be better to plan for the future by instead having some sort of > annotation which indicates that we are requesting (or dealing with) > only inline comments, such as `--inline-comment=<message>` or > `--comment=inline:<message>`? I do not see a need for anything, while we fail a request with a multi-line message and say "don't feed me a multi-line message". When we start supporting multi-line comment (which I do not think is a good idea at all, by the way), the code can switch between [vari] able = value # single line comment # a comment with # more than one line able = value depending on what is in <message>, so "inline:" prefix does not even have to exist, I would imagine. Of course you could even go fancier and allow something like this with unnecessary tailing LF ... $ git config --comment='another single line comment ' ... as a signal to turn a single line commet on a separate line. [vari] able = value # single line comment # a comment with # more than one line able = value # another single value comment able = value There will most likely need where e.g., above or below, around the "var = val" line to place the comment as well, so I do not see much value in investing more brain cycles on what the "--comment" option should look like, while we only support single-liners and explicitly reject multi-line messages.
Junio C Hamano <gitster@pobox.com> writes: > There will most likely need where e.g., above or below, around the "need" -> "need for adding a new option to specify". In sort, we would need to have more than just "use this message" anyway, and cramming everything into the single "--comment" option, even if it were feasible, would not be a good idea. > "var = val" line to place the comment as well, so I do not see much > value in investing more brain cycles on what the "--comment" option > should look like, while we only support single-liners and explicitly > reject multi-line messages.
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index dff39093b5e..e608d5ffef2 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -9,9 +9,9 @@ git-config - Get and set repository or global options SYNOPSIS -------- [verse] -'git config' [<file-option>] [--type=<type>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]] -'git config' [<file-option>] [--type=<type>] --add <name> <value> -'git config' [<file-option>] [--type=<type>] [--fixed-value] --replace-all <name> <value> [<value-pattern>] +'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]] +'git config' [<file-option>] [--type=<type>] [--comment=<value>] --add <name> <value> +'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] --replace-all <name> <value> [<value-pattern>] 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get <name> [<value-pattern>] 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all <name> [<value-pattern>] 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp <name-regex> [<value-pattern>] @@ -87,6 +87,11 @@ OPTIONS values. This is the same as providing '^$' as the `value-pattern` in `--replace-all`. +--comment <value>:: + Append a comment to new or modified lines. A '#' character will be + unconditionally prepended to the value. The value must not contain + linefeed characters (no multi-line comments are permitted). + --get:: Get the value for a given key (optionally filtered by a regex matching the value). Returns error code 1 if the key was not diff --git a/builtin/config.c b/builtin/config.c index b55bfae7d66..c54e9941a5f 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -44,6 +44,7 @@ static struct config_options config_options; static int show_origin; static int show_scope; static int fixed_value; +static const char *comment; #define ACTION_GET (1<<0) #define ACTION_GET_ALL (1<<1) @@ -173,6 +174,7 @@ static struct option builtin_config_options[] = { OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")), OPT_BOOL(0, "show-scope", &show_scope, N_("show scope of config (worktree, local, global, system, command)")), OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")), + OPT_STRING(0, "comment", &comment, N_("value"), N_("human-readable comment string (# will be prepended automatically)")), OPT_END(), }; @@ -797,6 +799,12 @@ int cmd_config(int argc, const char **argv, const char *prefix) usage_builtin_config(); } + if (comment && + !(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) { + error(_("--comment is only applicable to add/set/replace operations")); + usage_builtin_config(); + } + /* check usage of --fixed-value */ if (fixed_value) { int allowed_usage = 0; @@ -880,7 +888,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) check_write(); check_argc(argc, 2, 2); value = normalize_value(argv[0], argv[1], &default_kvi); - ret = git_config_set_in_file_gently(given_config_source.file, argv[0], value); + ret = git_config_set_in_file_gently(given_config_source.file, argv[0], comment, value); if (ret == CONFIG_NOTHING_SET) error(_("cannot overwrite multiple values with a single value\n" " Use a regexp, --add or --replace-all to change %s."), argv[0]); @@ -891,7 +899,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) value = normalize_value(argv[0], argv[1], &default_kvi); ret = git_config_set_multivar_in_file_gently(given_config_source.file, argv[0], value, argv[2], - flags); + comment, flags); } else if (actions == ACTION_ADD) { check_write(); @@ -900,7 +908,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) ret = git_config_set_multivar_in_file_gently(given_config_source.file, argv[0], value, CONFIG_REGEX_NONE, - flags); + comment, flags); } else if (actions == ACTION_REPLACE_ALL) { check_write(); @@ -908,7 +916,7 @@ int cmd_config(int argc, const char **argv, const char *prefix) value = normalize_value(argv[0], argv[1], &default_kvi); ret = git_config_set_multivar_in_file_gently(given_config_source.file, argv[0], value, argv[2], - flags | CONFIG_FLAGS_MULTI_REPLACE); + comment, flags | CONFIG_FLAGS_MULTI_REPLACE); } else if (actions == ACTION_GET) { check_argc(argc, 1, 2); @@ -936,17 +944,17 @@ int cmd_config(int argc, const char **argv, const char *prefix) if (argc == 2) return git_config_set_multivar_in_file_gently(given_config_source.file, argv[0], NULL, argv[1], - flags); + NULL, flags); else return git_config_set_in_file_gently(given_config_source.file, - argv[0], NULL); + argv[0], NULL, NULL); } else if (actions == ACTION_UNSET_ALL) { check_write(); check_argc(argc, 1, 2); return git_config_set_multivar_in_file_gently(given_config_source.file, argv[0], NULL, argv[1], - flags | CONFIG_FLAGS_MULTI_REPLACE); + NULL, flags | CONFIG_FLAGS_MULTI_REPLACE); } else if (actions == ACTION_RENAME_SECTION) { check_write(); diff --git a/builtin/gc.c b/builtin/gc.c index cb80ced6cb5..342907f7bdb 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -1553,7 +1553,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix) die(_("$HOME not set")); rc = git_config_set_multivar_in_file_gently( config_file, "maintenance.repo", maintpath, - CONFIG_REGEX_NONE, 0); + CONFIG_REGEX_NONE, NULL, 0); free(global_config_file); if (rc) @@ -1620,7 +1620,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi if (!config_file) die(_("$HOME not set")); rc = git_config_set_multivar_in_file_gently( - config_file, key, NULL, maintpath, + config_file, key, NULL, maintpath, NULL, CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE); free(global_config_file); diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index fda50f2af1e..e4e18adb575 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1283,7 +1283,7 @@ static void sync_submodule(const char *path, const char *prefix, submodule_to_gitdir(&sb, path); strbuf_addstr(&sb, "/config"); - if (git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url)) + if (git_config_set_in_file_gently(sb.buf, remote_key, NULL, sub_origin_url)) die(_("failed to update remote for submodule '%s'"), path); diff --git a/builtin/worktree.c b/builtin/worktree.c index 9c76b62b02d..a20cc8820e5 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -365,12 +365,12 @@ static void copy_filtered_worktree_config(const char *worktree_git_dir) if (!git_configset_get_bool(&cs, "core.bare", &bare) && bare && git_config_set_multivar_in_file_gently( - to_file, "core.bare", NULL, "true", 0)) + to_file, "core.bare", NULL, "true", NULL, 0)) error(_("failed to unset '%s' in '%s'"), "core.bare", to_file); if (!git_configset_get(&cs, "core.worktree") && git_config_set_in_file_gently(to_file, - "core.worktree", NULL)) + "core.worktree", NULL, NULL)) error(_("failed to unset '%s' in '%s'"), "core.worktree", to_file); diff --git a/config.c b/config.c index 3cfeb3d8bd9..2f3f6d123c6 100644 --- a/config.c +++ b/config.c @@ -3001,6 +3001,7 @@ static ssize_t write_section(int fd, const char *key, } static ssize_t write_pair(int fd, const char *key, const char *value, + const char *comment, const struct config_store_data *store) { int i; @@ -3041,7 +3042,14 @@ static ssize_t write_pair(int fd, const char *key, const char *value, strbuf_addch(&sb, value[i]); break; } - strbuf_addf(&sb, "%s\n", quote); + + if (comment) { + if (strchr(comment, '\n')) + die(_("multi-line comments are not permitted: '%s'"), comment); + else + strbuf_addf(&sb, "%s #%s\n", quote, comment); + } else + strbuf_addf(&sb, "%s\n", quote); ret = write_in_full(fd, sb.buf, sb.len); strbuf_release(&sb); @@ -3130,9 +3138,9 @@ static void maybe_remove_section(struct config_store_data *store, } int git_config_set_in_file_gently(const char *config_filename, - const char *key, const char *value) + const char *key, const char *comment, const char *value) { - return git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, 0); + return git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, comment, 0); } void git_config_set_in_file(const char *config_filename, @@ -3153,7 +3161,7 @@ int repo_config_set_worktree_gently(struct repository *r, if (r->repository_format_worktree_config) { char *file = repo_git_path(r, "config.worktree"); int ret = git_config_set_multivar_in_file_gently( - file, key, value, NULL, 0); + file, key, value, NULL, NULL, 0); free(file); return ret; } @@ -3195,6 +3203,7 @@ void git_config_set(const char *key, const char *value) int git_config_set_multivar_in_file_gently(const char *config_filename, const char *key, const char *value, const char *value_pattern, + const char *comment, unsigned flags) { int fd = -1, in_fd = -1; @@ -3245,7 +3254,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, free(store.key); store.key = xstrdup(key); if (write_section(fd, key, &store) < 0 || - write_pair(fd, key, value, &store) < 0) + write_pair(fd, key, value, comment, &store) < 0) goto write_err_out; } else { struct stat st; @@ -3399,7 +3408,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename, if (write_section(fd, key, &store) < 0) goto write_err_out; } - if (write_pair(fd, key, value, &store) < 0) + if (write_pair(fd, key, value, comment, &store) < 0) goto write_err_out; } @@ -3444,7 +3453,7 @@ void git_config_set_multivar_in_file(const char *config_filename, const char *value_pattern, unsigned flags) { if (!git_config_set_multivar_in_file_gently(config_filename, key, value, - value_pattern, flags)) + value_pattern, NULL, flags)) return; if (value) die(_("could not set '%s' to '%s'"), key, value); @@ -3467,7 +3476,7 @@ int repo_config_set_multivar_gently(struct repository *r, const char *key, int res = git_config_set_multivar_in_file_gently(file, key, value, value_pattern, - flags); + NULL, flags); free(file); return res; } diff --git a/config.h b/config.h index 5dba984f770..a85a8271696 100644 --- a/config.h +++ b/config.h @@ -290,7 +290,7 @@ int git_config_pathname(const char **, const char *, const char *); int git_config_expiry_date(timestamp_t *, const char *, const char *); int git_config_color(char *, const char *, const char *); -int git_config_set_in_file_gently(const char *, const char *, const char *); +int git_config_set_in_file_gently(const char *, const char *, const char *, const char *); /** * write config values to a specific config file, takes a key/value pair as @@ -336,7 +336,7 @@ int git_config_parse_key(const char *, char **, size_t *); int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned); void git_config_set_multivar(const char *, const char *, const char *, unsigned); int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned); -int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned); +int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, const char *, unsigned); /** * takes four parameters: diff --git a/sequencer.c b/sequencer.c index f49a871ac06..4c91ca5a844 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3460,54 +3460,54 @@ static int save_opts(struct replay_opts *opts) if (opts->no_commit) res |= git_config_set_in_file_gently(opts_file, - "options.no-commit", "true"); + "options.no-commit", NULL, "true"); if (opts->edit >= 0) - res |= git_config_set_in_file_gently(opts_file, "options.edit", + res |= git_config_set_in_file_gently(opts_file, "options.edit", NULL, opts->edit ? "true" : "false"); if (opts->allow_empty) res |= git_config_set_in_file_gently(opts_file, - "options.allow-empty", "true"); + "options.allow-empty", NULL, "true"); if (opts->allow_empty_message) res |= git_config_set_in_file_gently(opts_file, - "options.allow-empty-message", "true"); + "options.allow-empty-message", NULL, "true"); if (opts->keep_redundant_commits) res |= git_config_set_in_file_gently(opts_file, - "options.keep-redundant-commits", "true"); + "options.keep-redundant-commits", NULL, "true"); if (opts->signoff) res |= git_config_set_in_file_gently(opts_file, - "options.signoff", "true"); + "options.signoff", NULL, "true"); if (opts->record_origin) res |= git_config_set_in_file_gently(opts_file, - "options.record-origin", "true"); + "options.record-origin", NULL, "true"); if (opts->allow_ff) res |= git_config_set_in_file_gently(opts_file, - "options.allow-ff", "true"); + "options.allow-ff", NULL, "true"); if (opts->mainline) { struct strbuf buf = STRBUF_INIT; strbuf_addf(&buf, "%d", opts->mainline); res |= git_config_set_in_file_gently(opts_file, - "options.mainline", buf.buf); + "options.mainline", NULL, buf.buf); strbuf_release(&buf); } if (opts->strategy) res |= git_config_set_in_file_gently(opts_file, - "options.strategy", opts->strategy); + "options.strategy", NULL, opts->strategy); if (opts->gpg_sign) res |= git_config_set_in_file_gently(opts_file, - "options.gpg-sign", opts->gpg_sign); + "options.gpg-sign", NULL, opts->gpg_sign); for (size_t i = 0; i < opts->xopts.nr; i++) res |= git_config_set_multivar_in_file_gently(opts_file, "options.strategy-option", - opts->xopts.v[i], "^$", 0); + opts->xopts.v[i], "^$", NULL, 0); if (opts->allow_rerere_auto) res |= git_config_set_in_file_gently(opts_file, - "options.allow-rerere-auto", + "options.allow-rerere-auto", NULL, opts->allow_rerere_auto == RERERE_AUTOUPDATE ? "true" : "false"); if (opts->explicit_cleanup) res |= git_config_set_in_file_gently(opts_file, - "options.default-msg-cleanup", + "options.default-msg-cleanup", NULL, describe_cleanup_mode(opts->default_msg_cleanup)); return res; } diff --git a/submodule-config.c b/submodule-config.c index 54130f6a385..11428b4adad 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -978,7 +978,7 @@ int config_set_in_gitmodules_file_gently(const char *key, const char *value) { int ret; - ret = git_config_set_in_file_gently(GITMODULES_FILE, key, value); + ret = git_config_set_in_file_gently(GITMODULES_FILE, key, NULL, value); if (ret < 0) /* Maybe the user already did that, don't error out here */ warning(_("Could not update .gitmodules entry %s"), key); diff --git a/submodule.c b/submodule.c index 213da79f661..86630932d09 100644 --- a/submodule.c +++ b/submodule.c @@ -2052,7 +2052,7 @@ void submodule_unset_core_worktree(const struct submodule *sub) submodule_name_to_gitdir(&config_path, the_repository, sub->name); strbuf_addstr(&config_path, "/config"); - if (git_config_set_in_file_gently(config_path.buf, "core.worktree", NULL)) + if (git_config_set_in_file_gently(config_path.buf, "core.worktree", NULL, NULL)) warning(_("Could not unset core.worktree setting in submodule '%s'"), sub->path); diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 31c38786870..ac7b02e6b07 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -69,13 +69,24 @@ test_expect_success 'replace with non-match (actually matching)' ' cat > expect << EOF [section] - penguin = very blue Movie = BadPhysics UPPERCASE = true - penguin = kingpin + penguin = gentoo #Pygoscelis papua + disposition = peckish #find fish + foo = bar ## abc [Sections] WhatEver = Second EOF +test_expect_success 'append comments' ' + git config --replace-all --comment="Pygoscelis papua" section.penguin gentoo && + git config --comment="find fish" section.disposition peckish && + git config --comment="# abc" section.foo bar && + test_cmp expect .git/config +' + +test_expect_success 'Prohibited LF in comment' ' + test_must_fail git config --comment="a${LF}b" section.k v +' test_expect_success 'non-match result' 'test_cmp expect .git/config' diff --git a/worktree.c b/worktree.c index b02a05a74a3..cf5eea8c931 100644 --- a/worktree.c +++ b/worktree.c @@ -807,9 +807,9 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath, static int move_config_setting(const char *key, const char *value, const char *from_file, const char *to_file) { - if (git_config_set_in_file_gently(to_file, key, value)) + if (git_config_set_in_file_gently(to_file, key, NULL, value)) return error(_("unable to set %s in '%s'"), key, to_file); - if (git_config_set_in_file_gently(from_file, key, NULL)) + if (git_config_set_in_file_gently(from_file, key, NULL, NULL)) return error(_("unable to unset %s in '%s'"), key, from_file); return 0; }