"Ralph Seichter via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > From: Ralph Seichter <github@xxxxxxxxxxx> > > Introduce the ability to append comments to modifications > made using git-config. Example usage: > > git config --comment "changed via script" \ > --add safe.directory /home/alice/repo.git > > based on the proposed patch, the output produced is: > > [safe] > directory = /home/alice/repo.git #changed via script For readability, you probably would want to have a SP before the given string, i.e., variable = "value" # message comes here > * Motivation: > > The use case which triggered my submitting this patch is These two lines should not be needed. It is customary to just state that the users need to be able to do X and adding feature Y is one way to allow that, without such preamble. > my need 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. > > * Design considerations and implementation details: > > The implementation ensures that a # character is always > prepended to the provided comment string, and that the It is unclear what happens when a user gives comment that already has the comment introducer, e.g., --comment="# this is comment". > comment text is appended as a suffix to the changed key- > value-pair in the same line of text. Multiline comments > are deliberately not supported, because their usefulness "not supported" can take many shapes, ranging from "producing a random result and may corrupt the resulting configuration file" and "the second and subsequent lines are silently ignored", to "results in an error, stops the command before doing anything". > does not justifiy the possible problems they pose when > it comes to removing ML comments later. "justify" > > * Target audience: > > Regarding the intended consumers of the comments made: The above two lines say the same thing and are unnecessary, especially before a sentence that begins with "They are aimed at". > They 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. > > Signed-off-by: Ralph Seichter <github@xxxxxxxxxxx> > --- > config: add --comment option to add a comment > > config: add --comment option to add a comment > > Introduce the ability to append comments to modifications made using > ... > displayed by git-config at a later time. No need to repeat a wall of text below the three-dash line if they do not give additional information on top of what is already in the proposed commit log message. > diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt > index dff39093b5e..ee8cd251b24 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>] There is Patrick's proposal to revamp the UI of this command, https://lore.kernel.org/git/cover.1709724089.git.ps@xxxxxx/ which may make the above simpler (e.g., there won't be two lines that talks about "set" commands, with and without "--add" option, for example). This topic may want to at least wait for the conclusion of that design discussion, and possibly its implementation. > @@ -87,6 +87,10 @@ OPTIONS > values. This is the same as providing '^$' as the `value-pattern` > in `--replace-all`. > > +--comment:: > + Append a comment to new or modified lines. A '#' character > + will be automatically prepended to the value. Other options that take mandatory parameter are are described with their parameter on the item heading line. It may help to somehow mention that the "appending" is done at the end on the same line. Perhaps something along this line. --comment <message>:: Append the _<message>_ at the end of the new or modified lines with the value. A comment introducer ` # ` is prepended to _<message>_ if it does not already have one. The command errors out if given a _<message>_ that spans multiple lines. > @@ -797,6 +799,11 @@ 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))) { This overly long line is both visually annoying and harder to maintain. If you can design a solution that makes it easier to read a future patch that adds support of the comment option to more actions (or removes it from an action), that would be great. Otherwise, do at least: if (comment && !(actions & (...)) { > + error(_("--comment is only applicable to add/set/replace operations")); > + usage_builtin_config(); > + } > + If I were doing this, I'd probably add this new block after the fixed-value thing, as it is bad manners to add new code _before_ existing ones, as if your new invention were somehow more important than all the others, when the order does not matter. If there is a valid technical reason (e.g., the new code modifies the state of the program that affects the beahviour of the later and existing parts of the code), the above comment of course does not apply. > /* check usage of --fixed-value */ > if (fixed_value) { > int allowed_usage = 0; > @@ -880,7 +887,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); It is somewhat annoying that we have to see so much code churn to add this new parameter X-<. I notice that we are sending "comment" to the underlying machinery without doing ANY sanity checking (like, "ah, we got a message without the '#' prefix, so we should add '# ' in front of it before asking the API to add that comment string to the value line"). We may also want to have a code that says: Yikes, this message has LF in it, but the underlying machinery does not check for it and end up corrupting the configuration file by creating [section] var = value #comment that spans on two lines when given LF=' ' git config --comment="comment${LF}that spans..." section.var value or something. The underlying machinery should be updated to die() when given such a message instead of silently corrupting the resulting file, but the front-end that receives the end-user input should check for obviously problematic payload before bothering the API with it. > - strbuf_addf(&sb, "%s\n", quote); > + if (comment) > + strbuf_addf(&sb, "%s #%s\n", quote, comment); > + else > + strbuf_addf(&sb, "%s\n", quote); > 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 *); The original was probably OK without naming these parameters of the same type, as it is (arguably) obvious to have the filename first, because that is what differenciates the "in-file" variant. And then key followed by value, because that is the usual "assignment" order people would naturally expect. But it was already on the borderline. The idea is that we do not have to (but still can) name the parameters in our function declarations when it is obvious from their types what they are. Addition of this comment thing will push us way over the line. The same comment applies to many of the function declarations touched by this patch. If I were doing this patch, I'd have at least one clean-up patch that updates this line to read: int git_config_set_in_file_gently(const char *filename, const char *key, const char *value); And then write a separate patch to add the "--comment" feature. I however have some suspicion that we might move away from "listing many random parameters" style to "having only the essential parameters, together with a single pointer to a structure that holds the optional bits" style, especially when the UI revamp Patrick proposed comes. In the case of config API, <key, value> is the essential bit in the write direction, and value-pattern restriction and the bit that says key is an regexp would be in "the optional bits" group. This <comment> thing will also be in the latter class. > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > index 31c38786870..daddaedd23c 100755 > --- a/t/t1300-config.sh > +++ b/t/t1300-config.sh > @@ -69,13 +69,18 @@ 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 > [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 && > + test_cmp expect .git/config > +' Add test for at least two more cases and show what should happen. --comment="# the user already has the pound" --comment="this is being ${LF} naughty to break configuration" Thanks.
