Subject: Re: [PATCH v2] config: add --comment option to add a comment

From: Junio C Hamano <[email-masked]>
Date: Mon, 11 Mar 2024 05:55:16 -0700

"Ralph Seichter via GitGitGadget" <[email-masked]> writes:

> From: Ralph Seichter <[email-masked]>
>
> 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 <[email-masked]>
> ---
>     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/[email-masked]/

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.