"Ralph Seichter via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Ralph Seichter <github@xxxxxxxxxxx>
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@xxxxxxxxx>
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@xxxxxxxxx>
---
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
'
--
2.44.0-248-g4f9b731bde
