Allow git-config to append a comment

From: Ralph Seichter <[email protected]>

Introduce the ability to append comments to modifications made using git-config. Example usage:

  git config --comment "I changed this. --A. Script" \
    --add safe.directory /home/alice/somerepo.git

The implementation ensures that a # character is always prepended to the provided comment string.

---
Allow git-config to append a comment

Introduce the ability to append comments to modifications made using git-config. Example usage:

git config --comment "I changed this. --A. Script" \
--add safe.directory /home/alice/somerepo.git

The implementation ensures that a # character is always prepended to the provided comment string.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1681%2Frseichter%2Fissue-1680-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1681/rseichter/issue-1680-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1681

 Documentation/git-config.txt | 10 +++++++---
 builtin/config.c             | 21 ++++++++++++++-------
 builtin/gc.c                 |  4 ++--
 builtin/submodule--helper.c  |  2 +-
 builtin/worktree.c           |  4 ++--
 config.c                     | 21 +++++++++++++--------
 config.h                     |  4 ++--
 sequencer.c                  | 28 ++++++++++++++--------------
 submodule-config.c           |  2 +-
 submodule.c                  |  2 +-
 t/t1300-config.sh            |  9 +++++++--
 worktree.c                   |  4 ++--
 12 files changed, 66 insertions(+), 45 deletions(-)

index dff39093b5e..ee8cd251b24 100644
@@ -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,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.
+
 --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

index b55bfae7d66..2aab3c0baf3 100644
@@ -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,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))) {
+		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 +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);
 		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 +898,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 +907,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 +915,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 +943,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();

index cb80ced6cb5..342907f7bdb 100644
@@ -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);
 

index fda50f2af1e..e4e18adb575 100644
@@ -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);
 

index 9c76b62b02d..a20cc8820e5 100644
@@ -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);
 

index 3cfeb3d8bd9..a22594eabd9 100644
@@ -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,10 @@ 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)
+		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 +3134,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 +3157,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 +3199,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 +3250,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 +3404,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 +3449,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 +3472,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;
 }

index 5dba984f770..a85a8271696 100644
@@ -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:

index f49a871ac06..4c91ca5a844 100644
@@ -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;
 }

index 54130f6a385..11428b4adad 100644
@@ -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);

index 213da79f661..86630932d09 100644
@@ -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);
 

index 31c38786870..daddaedd23c 100755
@@ -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
+'
 
 test_expect_success 'non-match result' 'test_cmp expect .git/config'
 

index b02a05a74a3..cf5eea8c931 100644
@@ -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;
 }

base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d

gitgitgadget

Thanks for a patch.

Make sure your title will mix well in "git shortlog --no-merges" output from recent commits by other contributors. See Documentation/SubmittingPatches[[describe-changes]]. Perhaps

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

If you are illustrating a sample input, please also explain what output it produces. What do the resulting lines in the config file look like after you run this command?

I find the overall idea somewhat incomplete. Perhaps stepping back a bit and thinking through the _whole_ use case of "comment" in the configuration files is in order, before thinking about an easier way to add one. Why are you adding "# comment" to your config file? Who reads these comments, with what tool, and for what purpose? What will they do after they learn something from what their former selves wrote as comments?

Let's imagine we had this in our global configuration file in the beginning:

    $ cat ~/.gitconfig
	...
    [safe]
	directory = /home/bob/otherrepo.git
	...

and then we run this:

    $ git config --global --comment 'the reason why I added ~alice/
    is because ...' --add safe.directory /home/alice/somerepo.git
    $ git config --global --add safe.directory /home/charlie/somerepo.git

which may modify the configuration file with the new entry with a comment. The part about safe.directory might now read like so:

[safe]
directory = /home/bob/otherrepo.git
# the reason why I added ~alice/
# is because ...
directory = /home/alice/somerepo.git
directory = /home/charlie/somerepo.git

Now how do we find out about this comment? "git config -l" would not give us that. Are we expected to look at it in our editor or pager?

    $ vi ~/.gitconfig
    $ less ~/.gitconfig

Why are we interested in looking at these comments in the first place [*]?

Side note: I do not ask "why are we interested in leaving these comments in the file"---it goes without saying that it is because we want to be able to later read them.

Presumably, we are trying to remind ourselves why we added a particular variable=value. By learning that, what are we going to do next? Perhaps once the reason to mark /home/alice/somerepo.git as a safe.directory disappears, we would want to remove this entry?

But then, after running

    $ less ~/.gitconfig

and then learning that we no longer need /home/alice/somerepo.git marked as a safe.directory, how would we remove it from the configuration file, and what happens to the comment?

    $ git config --global --unset safe.directory /home/alice/somerepo.git

may remove the value, but it would not remove the comment, and you'll be left with something like this:

[safe]
directory = /home/bob/otherrepo.git
# the reason why I added ~alice/
# is because ...
directory = /home/charlie/somerepo.git

The remaining comment looks as if it talks about "charlie", but it simply is stale.

Should it have removed the comment, then? I actually do not think so. A comment before a particular variable definition may not be the result of the use of this new "config --comment" option, but perhaps the user added it manually. Or even worse, the comment may not even be about an individual entry. Imagine

[section]
# definitions for per-user stuff begin here var = /home/alice
# these two directories are actually charlie's var = /home/bob
var = /home/charlie

# definitions for per-project stuff
var = /project/a
var = /project/b

Can we come up with a code that reliably decides when to remove the first comment we see above? The answer given by human might be "when all the entries about /home/alice, /home/bob, and /home/charlie get removed", but can we implement that in code without interpreting the natural language? I doubt it. We may even be better off to keep the comment to remind ourselves where to add the per-user stuff in the section when we hire a new user the next time.

With the new "--comment" thing might be able to add comments without using an editor, but the user would need to view the configuration file with a pager or an editor bypassing Git to make use of what was recorded in there. And it is likely that these comments will need to be edited or removed using an editor because "git config" command would not be usable for maintaining them. For that matter, if you made a typo in your "--comment" option from the command line, you would have to resort to your editor to fix that typo.

The above is an illustration of what I want to see the author, who comes up with this "wouldn't it be wonderful to teach git-config to add comment?" idea, thinks through. The first patch might be to just add a comment when a variable=value is added, but we want to see the vision of how the whole end-user experience should look like, in which this first small piece will fit.

Identify the whole life cycle of the result of using the feature, what the end-user experience of using the result of using the feature would look like, etc. In this case, "the result of using the feature" is the comments that are initially placed next to a newly defined variable-value pair, and at least to me, the end-user experience to manage the life cycle of these comments is not all that improved from the status quo, which is that you need your pager and editor to view them, edit them, typofix them, and remove them.

I am somewhat negative on the idea to make the initial addition of the comment vastly different from all other uses of the comment by introducing this "--comment" option. If we have a vision, if not design and/or code, to help managing other parts of the lifecycle, not just adding in the beginning, it might be a different story, but I cannot quite see it.

And the amount of the change required for that tiny bit of "improvement" (if we can call it, which is dubious) does not seem worth it.

Thanks.

* Junio C Hamano:

Thank you for your in-depth comment. This is the first time I have ever considered contributing to Git, so I have a lot to learn. My pull request [1] on GitGitGadget has been approved by Johannes Schindelin, by the way, and the PR is based on an issue Johannes created [2] after a brief discussion him and I had on Discord [3]. I have updated the subject line, as you suggested.

[1] https://github.com/gitgitgadget/git/pull/1681
[2] https://github.com/gitgitgadget/git/issues/1680
[3] https://discord.com/channels/1042895022950994071/1213052410281467906

I don't know if it is the polite thing to ask you to please refer to the information I linked, or if I should duplicate the information here? The short version is that I need to be able to distinguish between config entries made by automation (like scripts, or Ansible in my particular case) and those made by a human.

The result of running the above command looks as follows:

[safe]

	directory = /home/alice/somerepo.git #I changed this. --A. Script

I mentioned human-readable comments in the patch, and humans are indeed the intended audience. If a human finds changes made to a Git config file, and a comment states that the modification was e.g. made by automation, it adds beneficial information for said human. I can for example create a comment with a URL pointing to a website providing additional explanations.

Yes. I routinely use cat/vim to inspect/modify my Git config files. They are suitable for human consumption, after all. Also, comments can already be manually added in creative ways, and are ignored when Git reads config data. Comments being read only by humans is pretty much their whole point, in my opinion.

Your examples about difficulties removing comments hinge on there being multiline comments, as far as I can tell. My patch only supports single-line comments, and only as a suffix to newly added key-value pairs. This is a deliberate design choice.

I don't have a greater vision for comments. Their use is to provide information for humans, no more, no less. There is also no idea of a user experience beyond pager/editor in my mind. The patch addresses specific needs of mine, and Johannes suggested it as a new feature, and that's all the motivation behind it.

As I mentioned in my PR, it also does not seem elegant to me to modify so many files. Alas, C does not offer expanding function signatures by adding parameters with default values, like Python does. Adding a new function like, perhaps, git_config_set_in_file_gently_with_comment() could be a remedy?

-Ralph

That would have been a crucial piece of information to have in the proposed log message, as limiting ourselves to a comment that is tucked after the same line as the value, things can become somewhat simplified. We may not have to worry about deletion, even though the point about "we need to look at and typofix them with our viewers and editors" still stands.

By the way, you may or may not have noticed it, but my example deliberately had a multi-line comment:

    $ git config --global --comment 'the reason why I added ~alice/
    is because ...' --add safe.directory /home/alice/somerepo.git

How such a thing is handled also needs to be discussed in the proposed log message, and perhaps in the documentation as well.

Such design choices need to be described in the proposed log message to help future developers who will be updating this feature, once it gets in.

Thanks for writing quite a lot to answer _my_ questions, but these questions are samples of things that future developers would wonder and ask about when they want to fix bugs in, enhance, or otherwise modify the implementation of this "add comment" feature. They may even be working on adding other features to complement the "add comment" feature, by designing support for viewing or typofixing existing comments. When they do so, it would help them to know how this existing feature was expected to be used and how it would fit in a larger picture (which may not have yet existed back when the feature was invented). Answering these anticipated questions is one of the greatest things that a commit log message can do to help them.

Thanks.

* Junio C. Hamano:

Just as a brief interjection: I am sorry that my inexperience with Git's mailing-list based process caused me to leave out details which were discussed earlier, be it on GitHub or Discord. However, me not mentioning that I am aiming for single-line suffix comments only was due to simple forgetfulness, without an excuse behind it. My bad.

I think it best I flesh out the commit message before anything else, and then resubmit. That should bundle the necessary information.

-Ralph

Don't feel too bad. It is not about discord vs mailing list vs github "reviews on the web". It is about the end-product, the commit log messages, what future developers will see in their "git log" output.

Yup. Please do that.

Communicating with reviewers and other contributors is not the primary goal of the review process. It is done to help us reach a better end-product, the commit log messages that will help our future developers and future selves.

Thanks.

log message, as limiting ourselves to a comment that is

simplified. We may not have to worry about deletion, even

viewers and editors" still stands.

had a multi-line comment:

message, and perhaps in the documentation as well.

help future developers who will be updating this feature,

questions are samples of things that future developers would

modify the implementation of this "add comment"

the "add comment" feature, by designing support for

them to know how this existing feature was expected to be

existed back when the feature was invented). Answering these

message can do to help them.

I am concerned about the compatibility of this series with the community. While comments are permitted in .gitconfig files, I am not 100% sure that all stakeholders, particularly those who parse .gitconfig files in their own scripts outside of git - sure, it is their own responsibility, but this might be unexpected. I worry that this might unintentionally introduce incompatibilities into repository configurations. Is there broad consensus to do this?

--Randall

* [email protected]:

Comments are nothing new, and humans have added far crazier comments to their Git config in the past. The patch ensures that a '#' precedes the comments added using git-config, which is not guaranteed to happen when Joe Random User manually edits config files.

I think that anybody incapable of reliably dealing with comments in config files would already have fallen flat on his/her nose, regardless of how those comments were made.

Do you have an example?

-Ralph

their Git config in the past. The patch ensures that a '#'

happen when Joe Random User manually edits config files.

files would already have fallen flat on his/her nose,

No example. This is a comment on "potential" changes to data that scripts around git for automation purposes might use. My purpose is just to highlight, for the purpose of reviewing the change, that there may be unintended impacts, that's all. It may be useful to include comments in the change notices and documentation pages that using this capability may impact scripting. When a user manually puts in a comment, any breakages in their scripts are 100% their issue. With git config moving comments around, responsibility shifts to git - a.k.a., unintended consequences. I am not asking that this change not happen - it is a good thing, but ensuring that we communicate that this may cause breakages if external programs/scripts read .gitconfig would be helpful. This also would need to be coordinated with the libification efforts at some point.

* [email protected]:

I see. Have you perhaps spotted a flaw in the patch which might cause problems? If so, I'd like to address it.

It is important to note that my implementation does not engage in "moving comments around" at all. It merely supports adding comment suffixes to config lines *while they are created by git-config*. No after-the-fact comment manipulation is done. There are no multiline comments involved either, which I should have mentioned from the get-go. The limited scope of my proposal is deliberate, and aimed at avoiding possible problems based on the design alone.

-Ralph

From: Ralph Seichter <[email protected]>

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

* Motivation:

The use case which triggered my submitting this patch is 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 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 does not justifiy the possible problems they pose when it comes to removing ML comments later.

* Target audience:

Regarding the intended consumers of the comments made:
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.

---
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 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

* Motivation: The use case which triggered my submitting this patch is 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 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 does not justifiy the possible problems they pose when it comes to removing ML comments later.

* Target audience: Regarding the intended consumers of the comments
made: 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.

Changes since v1:

* Rewrite commit message to address reviewers' questions.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1681%2Frseichter%2Fissue-1680-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1681/rseichter/issue-1680-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1681

Range-diff vs v1:

1: d07cbb4bbf7 ! 1: 1e6ccc81685 Allow git-config to append a comment
@@ Metadata
Author: Ralph Seichter <[email protected]>

## Commit message ##
- Allow git-config to append a comment
+ config: add --comment option to add a comment

Introduce the ability to append comments to modifications made using git-config. Example usage:

- git config --comment "I changed this. --A. Script" \
- --add safe.directory /home/alice/somerepo.git
+ 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
+
+ * Motivation:
+
+ The use case which triggered my submitting this patch is
+ 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.
+ prepended to the provided comment string, and that the
+ 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
+ does not justifiy the possible problems they pose when
+ it comes to removing ML comments later.
+
+ * Target audience:
+
+ Regarding the intended consumers of the comments made:
+ 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.

 Documentation/git-config.txt | 10 +++++++---
 builtin/config.c             | 21 ++++++++++++++-------
 builtin/gc.c                 |  4 ++--
 builtin/submodule--helper.c  |  2 +-
 builtin/worktree.c           |  4 ++--
 config.c                     | 21 +++++++++++++--------
 config.h                     |  4 ++--
 sequencer.c                  | 28 ++++++++++++++--------------
 submodule-config.c           |  2 +-
 submodule.c                  |  2 +-
 t/t1300-config.sh            |  9 +++++++--
 worktree.c                   |  4 ++--
 12 files changed, 66 insertions(+), 45 deletions(-)

index dff39093b5e..ee8cd251b24 100644
@@ -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,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.
+
 --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

index b55bfae7d66..2aab3c0baf3 100644
@@ -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,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))) {
+		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 +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);
 		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 +898,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 +907,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 +915,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 +943,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();

index cb80ced6cb5..342907f7bdb 100644
@@ -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);
 

index fda50f2af1e..e4e18adb575 100644
@@ -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);
 

index 9c76b62b02d..a20cc8820e5 100644
@@ -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);
 

index 3cfeb3d8bd9..a22594eabd9 100644
@@ -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,10 @@ 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)
+		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 +3134,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 +3157,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 +3199,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 +3250,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 +3404,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 +3449,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 +3472,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;
 }

index 5dba984f770..a85a8271696 100644
@@ -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:

index f49a871ac06..4c91ca5a844 100644
@@ -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;
 }

index 54130f6a385..11428b4adad 100644
@@ -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);

index 213da79f661..86630932d09 100644
@@ -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);
 

index 31c38786870..daddaedd23c 100755
@@ -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
+'
 
 test_expect_success 'non-match result' 'test_cmp expect .git/config'
 

index b02a05a74a3..cf5eea8c931 100644
@@ -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;
 }

base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d

gitgitgadget

For readability, you probably would want to have a SP before the given string, i.e.,

variable = "value" # message comes here

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.

It is unclear what happens when a user gives comment that already has the comment introducer, e.g., --comment="# this is comment".

"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".

"justify"

The above two lines say the same thing and are unnecessary, especially before a sentence that begins with "They are aimed at".

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.

	if (comment &&
	    !(actions & (...)) {

    int git_config_set_in_file_gently(const char *filename, const char *key, const char *value);

	--comment="# the user already has the pound"
	--comment="this is being ${LF} naughty to break configuration"

Let me interject... Perhaps also a tab character before the "# comment", instead of a space character. That would result in even better readability.

instead of a space character. That would result in even better

Does adding a tab following data change the parse semantics of .gitconfig? My naïve understanding is that .gitconfig follows a basic rule of leading tab within a section, followed by text. Is there a formal syntax description of what valid input is? The value does not need to be quoted, so what does the following actually resolve to:

(TAB)variable = value(TAB)# comment.

Does variable mean value or value(TAB)? Obviously TABS should be correctly be interpreted as whitespace to be ignored. However, what about:

(TAB)variable = value(TAB)s(TAB) # comment.

Does that mean value(TAB)s, value(TAB)s(TAB), value s, value s(TAB), values?

The definition according to git-config is

"The syntax is fairly flexible and permissive; whitespaces are mostly ignored. The # and ; characters begin comments to the end of line, blank lines are ignored."

"Mostly" does not make me comfortable that this is formally allowed or disallowed or ignored. I would suggest that this change needs to formalize the grammar on that documentation page for clarity.

It should mean "value(TAB)s", according to git-config(1).

I believe these two quotations from git-config(1) should make it more clear:

A line that defines a value can be continued to the next line by
ending
it with a \; the backslash and the end-of-line are stripped. Leading whitespaces after name =, the remainder of the line after the first comment character # or ;, and trailing whitespaces of the line are discarded unless they are enclosed in double quotes. Internal whitespaces within the value are retained verbatim.

The following escape sequences (beside \" and \\) are recognized: \n for newline character (NL), \t for horizontal tabulation (HT, TAB)
and
\b for backspace (BS). Other char escape sequences (including octal escape sequences) are invalid.

To me, all that indicates that trailing tab characters are stripped, but...

... I do agree that it should be clarified further in the man page.

Hmm, after having a look at config.c and doing some testing, git-config(1) seems to be wrong when it says that "internal whitespaces within the value are retained verbatim".

Here's an example... I placed the following into ~/.gitconfig:

[test]
blah = huh uh

There's a tab character between "huh" and "uh". Though, running

  git config --get test.blah

produces

huh uh

which contains a space character, not a tab. That's expected according to config.c, but not according to git-config(1).

Should we correct the wording in git-config(1) accordingly?

After some testing, I can confirm that any tabs (or spaces, or mixes) between the value and the "#" character do get ignored.

Depends on your screen width ;-)

If you were trying to tell me that SP or no SP is merely a personal preference with the comment, I think you succeeded in doing so.

Thanks.

Ah, screens are pretty wide these days. :)

Huh, that wasn't my intention. IMHO, a space character between "#" and the actual comment is pretty much mandatory.

Ah, OK, you were talking about the gap after the value before the "#" that introduces the comment, but I somehow mistook it as a comment about the whitespace after '#'.

The gap after the value, I do not have a strong opinion either way between SP and HT, except that I agree there should be something there for readability.

Given that other places where we do insert comments, like in the log message editor during "git commit -e", we always give a single space after the comment character, I tend to agree that a space after '#' is pretty much mandatory. It is a non starter to tell users that they should add their own SP at the beginning if they want to use such a common style, i.e.

git commit --comment=' here is my message' ;# BAD

With a simple rule like "Unless your message begins with '#', the message is prepended by '# ' (pound, followed by a SP), but when your message begins with '#', the string is used as is", those who want to use their own style can use whatever style they want, e.g.

git commit --comment='#I do not want SP there' git commit --comment='#^II want a HT there instead'

and that would be a much more preferrable design, i.e. making the common things easy, while leaving unusual things possible.

Thanks.

Yes, that's what I was talking about. I'm sorry if the way I wrote it initially wasn't clear enough.

I'd vote for a space character after "#", because that's pretty much the de facto standard. I don't remember seeing tabs used there.

I'd agree with that. Requiring the users to include a leading space would make things inconistent.

Agreed. That would be nice.

* Dragan Simic:

I'd rather not open /that/ can of worms. Tabs versus spaces, tab size, and so forth, are too much matters of personal taste for me to want to spend any time discussing.

-Ralph

I see, but please note that everyone should be prepared to spend some time discussing even seemingly unrelated things when contributing to a major open-source project.

I mean, perhaps the whole thing with the tab characters may not be the right example, but I just wanted to point out that the more major an open-source project is, the more discussion is often required.

* Dragan Simic:

Oh, I have no qualms discussing things, but over the last 40+ years, nothing good has ever come from debating the pros and cons of tabs and spaces. At least that's my personal experience.

-Ralph

There are pros and cons to both spaces and tabs, so there's hardly anything that can be concluded about either option being better or worse than the other. They're both good and bad at the same time.

However, I think there should be some way to allow the users to choose the kind of spacing between the automatically added comments and their respective values. Yes, readability may be subjective, but I think that the users should be allowed some kind of choice.

As to the whitespace _before_ the '#', we can just pick one and leave the choice to users' editor, which they have to resort to anyway for even trivial things like fixing typos. I am fine to defer the choice between HT and SP to Ralph as the person in the driver seat,

As to the whitespace _after_ the '#', I would say we have obligation to the users to be consistent with other codepaths that we already write our own comments. "# " is the only sensible choice of the default there, and we can allow other styles as I outlined in a separate message if we wanted to.

I'd vote for tabs before hashes (i.e. "name(SP)=(SP)value(HT)#(SP)comment"), because that's what I've seen used in numerous codebases. Thus, that should introduce some kind of additional consistency.

I'd agree with that, as I already noted in an earlier reply.

My take would be that all whitespace is ignored, but if you want a tab or other character in a value, be explicit about it:

variable = "value\ts" # comment

where all whitespace and comments are dropped from the parse to be functionally equivalent to

variable=value(TAB)s

when processing. Implicit quoting arbitrary whitespace can be interpreted in an ambiguous way. That's basically what the C parser does.

--Randall

Oh, I fully agree, but I'm afraid that would be a breaking change at this point. Perhaps numerous configuration values in the field already use the current (mis)behavior of the value parser in config.c.

I think the only right thing to do at this point is to document it properly and correctly in the git-config(1) manual page. A bit later I'll prepare and send a patch that does it.

Preface: I am not replying to every part of Junio's email this time around. However, that does not imply I am ignoring them, I simply do not currently have access to the source code.

* Junio C Hamano:

If the user wants a whitespace after the #, they can add it in the comment using quoting, e.g. --comment " message comes here". I don't think it is necessary to enforce the extra SP, because it is not syntactically required. Besides, readability is quite subjective.

Unclear? ;-) I wrote "a # character is always prepended to the provided comment string", and that is what happens. The current implementation is meant to be safe, not fancy.

That's GitGitGadget's doing. Unfortunately, it does not want to let me remove my pull request description on the GitHub website. I already tried to fiddle with it, but no success yet; GitHub keeps restoring my latest description text.

I don't know enough about the timespans involved to be sure, but that sounds like it could take a long time?

As I wrote before, the # is prepended unconditionally. LF is a more interesting question. I haven't yet tried actively introducing a linefeed. Does strbuf_addf() have any related filtering logic?

-Ralph

Perhaps that should be documented, so the users know what to expect and how to ensure extra spacing, if they desire so.

Perhaps that should also be documented, to avoid the "##" sequences from occurring unexpectedly.

* Dragan Simic:

Yup, better to be explicit than implicit. By the way: When it comes to my proposed patch, I am having small difficulties finding a balance between documenting things thoroughly and being accused of producing walls of text. ;-)

-Ralph

* Ralph Seichter:

I have now tried several times to inject a LF into a comment string, and did not manage to break the resulting config file. For example,

  ./git config --comment "foo\012bar" --add section.key value

leads to this entry:

[section]

      key = value #foo\012bar

Variable expansion with LF="\012" and --comment "foo${LF}bar" did not introduce a linefeed either. Have you guys perhaps managed to create an invalid config file?

-Ralph

You must use a literal line feed, e.g.:

    LF='
    '

(a single quoted newline) or, in a shell that supports this syntax:

    LF=$'\012'

(note $ and single quote: double quotes here do not work)
to get the line-feed into the shell variable. You can also capture the output of a program that emits the line-feed, but these are the direct assignment methods.

For instance:

    $ LF=$'\012'
    $ echo "foo${LF}bar"
    foo
    bar
    $

Lacking such capabilities, it's easy enough to use the printf shell command to produce special characters as output (which you can then capture, but various shells may also have Special Rules about such captured output, so the direct assignment method is better, in my opinion).

Chris

* Chris Torek:

Of course, silly me. Easily done in a shell script. I was too focused on trying it in an interactive shell. Thanks, Chris.

Do you perhaps know of a function in the Git code base which could be used to sanitise strings? It is quite a lot of code to sift through if one is unfamiliar with it, so I'll gladly take hints.

-Ralph

;-)

I think I've already given you that in my first message that mentioned multi-line input. In the test suite, we already have that $LF defined so your tests can use it without defining it yourself.

Don't silently butcher end-user input; instead do something like this to make it clear that you do not support it.

	if (strchr(comment_message, '\n'))
		die(_("multi-line comment not supported: '%s'"),
			comment_message);

From: Ralph Seichter <[email protected]>

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

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.

The implementation ensures that a # character is unconditionally prepended to the provided comment string, and that 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.

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.

---
config: add --comment option to add a comment

Changes since v2:

* Address reviewers' remarks.

Changes since v1:

* Rewrite commit message to address reviewers' questions.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1681%2Frseichter%2Fissue-1680-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1681/rseichter/issue-1680-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1681

Range-diff vs v2:

1: 1e6ccc81685 ! 1: 99cc839a115 config: add --comment option to add a comment
@@ Commit message
[safe]
directory = /home/alice/repo.git #changed via script

- * Motivation:
+ 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.

- The use case which triggered my submitting this patch is
- 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.
+ The implementation ensures that a # character is unconditionally
+ prepended to the provided comment string, and that 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.

- * Design considerations and implementation details:
-
- The implementation ensures that a # character is always
- prepended to the provided comment string, and that the
- 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
- does not justifiy the possible problems they pose when
- it comes to removing ML comments later.
-
- * Target audience:
-
- Regarding the intended consumers of the comments made:
- 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.
+ 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.

@@ Documentation/git-config.txt: 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.
++--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
@@ builtin/config.c: 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();
++ 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 */
@@ config.c: static ssize_t write_pair(int fd, const char *key, const char *value,
break;
}
- strbuf_addf(&sb, "%s\n", quote);
-+ if (comment)
-+ strbuf_addf(&sb, "%s #%s\n", quote, comment);
-+ else
++
++ 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);
@@ t/t1300-config.sh: test_expect_success 'replace with non-match (actually matchin
- 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'

 Documentation/git-config.txt | 11 ++++++++---
 builtin/config.c             | 22 +++++++++++++++-------
 builtin/gc.c                 |  4 ++--
 builtin/submodule--helper.c  |  2 +-
 builtin/worktree.c           |  4 ++--
 config.c                     | 25 +++++++++++++++++--------
 config.h                     |  4 ++--
 sequencer.c                  | 28 ++++++++++++++--------------
 submodule-config.c           |  2 +-
 submodule.c                  |  2 +-
 t/t1300-config.sh            | 15 +++++++++++++--
 worktree.c                   |  4 ++--
 12 files changed, 78 insertions(+), 45 deletions(-)

index dff39093b5e..e608d5ffef2 100644
@@ -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

index b55bfae7d66..c54e9941a5f 100644
@@ -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();

index cb80ced6cb5..342907f7bdb 100644
@@ -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);
 

index fda50f2af1e..e4e18adb575 100644
@@ -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);
 

index 9c76b62b02d..a20cc8820e5 100644
@@ -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);
 

index 3cfeb3d8bd9..2f3f6d123c6 100644
@@ -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;
 }

index 5dba984f770..a85a8271696 100644
@@ -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:

index f49a871ac06..4c91ca5a844 100644
@@ -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;
 }

index 54130f6a385..11428b4adad 100644
@@ -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);

index 213da79f661..86630932d09 100644
@@ -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);
 

index 31c38786870..ac7b02e6b07 100755
@@ -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'
 

index b02a05a74a3..cf5eea8c931 100644
@@ -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;
 }

base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d

gitgitgadget

Thanks for updating. The proposed commit log message reads much smoother.

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.

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.

These will become redundant if we rewrite the earlier paragraph like illustrated above.

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

	--comment <message>::
		Append a comment _<message>_ after the value on the
		same line. ...

	strbuf_addf(&sb, "%s%s\n", quote, comment);

 Documentation/git-config.txt | 15 ++++++++-------
 builtin/config.c             | 15 +++++++++++----
 config.c                     | 20 ++++++++++++++------
 t/t1300-config.sh            |  9 +++++----
 4 files changed, 38 insertions(+), 21 deletions(-)

index e608d5ffef..af374ee2e0 100644
@@ -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

index c54e9941a5..e859a659f4 100644
@@ -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);
 

index 2f3f6d123c..15019cb9a5 100644
@@ -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)

index ac7b02e6b0..d5dfb45877 100755
@@ -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

Extending the previous step, this allows the whitespace placed after the value before the "# comment message" to be tweaked by tweaking the preprocessing rule to:

* 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 '#', a Space is prepended.

* Otherwise, " # " (Space, '#', Space) is prefixed.

* A string with LF in it cannot be used as a comment string.

Unlike the previous step, which unconditionally added a space after the value before writing the "# comment string", because the above preprocessing already gives a whitespace before the '#', the resulting string is written immediately after copying the value.

And the sanity checking rule becomes

* comment string after the above massaging that comes into git_config_set_multivar_in_file_gently() must

- begin with zero or more whitespace characters followed by '#'.
- not have a LF in it.

I personally think this is over-engineered, but since I thought things through anyway, here it is in the patch form. The logic to tweak end-user supplied comment string is encapsulated in a new helper function, git_config_prepare_comment_string(), so if new front-end callers would want to use the same massaging rules, it is easily reused.

Unfortunately I do not think of a way to tweak the preprocessing rules further to optionally allow having no blank after the value, i.e. to produce

[section]
variable = value#comment

(which is a valid way to say section.variable=value, by the way) without sacrificing the ergonomics for the more usual case, so this time I really stop here.

---

* This is [3/1] as the one attached to my review of the single patch is [2/1].

 Documentation/git-config.txt | 12 +++++--
 builtin/config.c             |  7 +---
 config.c                     | 69 ++++++++++++++++++++++++++++++------
 config.h                     |  2 ++
 t/t1300-config.sh            |  6 ++++
 5 files changed, 76 insertions(+), 20 deletions(-)

index af374ee2e0..e4f2e07926 100644
@@ -89,9 +89,15 @@ OPTIONS
 
 --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).
+
+	If _<message>_ begins with one or more whitespaces followed
+	by "#", it is used as-is.  If it begins with "#", a space is
+	prepended before it is used.  Otherwise, a string " # " (a
+	space followed by a hash followed by a space) is prepended
+	to it.  And the resulting string is placed immediately after
+	the value defined for the variable.  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

index e859a659f4..0015620dde 100644
@@ -841,12 +841,7 @@ 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);
-	}
+	comment = git_config_prepare_comment_string(comment);
 
 	if (actions & PAGING_ACTIONS)
 		setup_auto_pager("config", 1);

index a85a827169..f4966e3749 100644
@@ -338,6 +338,8 @@ 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 *, const char *, unsigned);
 
+const char *git_config_prepare_comment_string(const char *);
+
 /**
  * takes four parameters:
  *

index d5dfb45877..cc050b3c20 100755
@@ -74,6 +74,8 @@ cat > expect << EOF
 	penguin = gentoo # Pygoscelis papua
 	disposition = peckish # find fish
 	foo = bar #abc
+	spsp = value # and comment
+	htsp = value	# and comment
 [Sections]
 	WhatEver = Second
 EOF

@@ -82,6 +84,10 @@ 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="and comment" section.spsp value &&
+	git config --comment="	# and comment" section.htsp value &&
+
 	test_cmp expect .git/config
 '
 

2.44.0-248-g4f9b731bde

If I am not mistaken, this topic, originating at

https://lore.kernel.org/git/[email protected]/

is expecting responses from you. Can you unblock it to let us move forward?

Thanks.

* Junio C. Hamano:

I don't see any obvious way to alter the pull request's state, or where a response of mine might be inserted to move things along. I also have not found anything related in the GitGitGadget docs. Thus, I'll try to issue another /submit command in the hope that this might affect the process.

-Ralph

The easiest would have been for you to say "Yup, the two additional patches queued on top of mine seem to make it better. Let me review them in detail", followed by "Yeah, they looked good to me", and after that we can just merge the topic with three patches down to 'next' and later to 'master'.

Of course, if you do not agree with the two follow-up patches, you can point out issues in them and argue why they are bad idea. That would take more cycles, of course.

* Junio C. Hamano:

Would it? Well, you wrote the following two weeks ago, among other things:

This unilateral decision of yours, and the following prolonged debate about spaces/tabs (which I clearly stated I consider a waste of time) left me with the impression that what I think doesn't matter much anyway. Also, it seems to me that this whole subject has already been blown far out of proportion. If this exercise leads to the feature I was proposing, you guys can do whatever, and I won't slow things down by expressing my opinions, or by continuing to formulate my own commit messages. So much time has already been spent.

-Ralph

Hello Ralph,

Well, is getting patches accepted to git hard and often challenging? Absolutely. Do additional patches and discussions sprout all over the place? They obviously do. But, does a high-profile open-source project deserve such an approach? Without doubt.

In fact, pretty much every good software project should employ such an approach, but that unfortunately isn't always the case.

That's how I see it.

You can call it unilateral or whatever you like, but there are project principles, e.g., "try to keep things consistent", and "making unusual things possible is fine, but it shouldn't make the default thing harder", and they are not negotiable.

When it comes to quality of the end product, it is true that your considering it a waste of time does not matter. It is effort needed to polish your topic to an acceptable level. Either we do so with the help of reviewer input, or alternatively we could drop the topic.

In any case, I'll keep the three patches separate and mark the topic for 'next'.

Thanks.

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.

"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.