[PATCH] Allow git-config to append a comment

[PATCH] Allow git-config to append a comment

[Ralph Seichter via GitGitGadget]

From: Ralph Seichter <github@seichter.de>

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.

Signed-off-by: Ralph Seichter <github@seichter.de>
---
    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(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index dff39093b5e..ee8cd251b24 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,9 +9,9 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [--type=<type>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]]
-'git config' [<file-option>] [--type=<type>] --add <name> <value>
-'git config' [<file-option>] [--type=<type>] [--fixed-value] --replace-all <name> <value> [<value-pattern>]
+'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]]
+'git config' [<file-option>] [--type=<type>] [--comment=<value>] --add <name> <value>
+'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] --replace-all <name> <value> [<value-pattern>]
 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get <name> [<value-pattern>]
 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all <name> [<value-pattern>]
 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp <name-regex> [<value-pattern>]
@@ -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
diff --git a/builtin/config.c b/builtin/config.c
index b55bfae7d66..2aab3c0baf3 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -44,6 +44,7 @@ static struct config_options config_options;
 static int show_origin;
 static int show_scope;
 static int fixed_value;
+static const char *comment;
 
 #define ACTION_GET (1<<0)
 #define ACTION_GET_ALL (1<<1)
@@ -173,6 +174,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")),
 	OPT_BOOL(0, "show-scope", &show_scope, N_("show scope of config (worktree, local, global, system, command)")),
 	OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")),
+	OPT_STRING(0, "comment", &comment, N_("value"), N_("human-readable comment string (# will be prepended automatically)")),
 	OPT_END(),
 };
 
@@ -797,6 +799,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();
diff --git a/builtin/gc.c b/builtin/gc.c
index cb80ced6cb5..342907f7bdb 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1553,7 +1553,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
 			die(_("$HOME not set"));
 		rc = git_config_set_multivar_in_file_gently(
 			config_file, "maintenance.repo", maintpath,
-			CONFIG_REGEX_NONE, 0);
+			CONFIG_REGEX_NONE, NULL, 0);
 		free(global_config_file);
 
 		if (rc)
@@ -1620,7 +1620,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
 		if (!config_file)
 			die(_("$HOME not set"));
 		rc = git_config_set_multivar_in_file_gently(
-			config_file, key, NULL, maintpath,
+			config_file, key, NULL, maintpath, NULL,
 			CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);
 		free(global_config_file);
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fda50f2af1e..e4e18adb575 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1283,7 +1283,7 @@ static void sync_submodule(const char *path, const char *prefix,
 	submodule_to_gitdir(&sb, path);
 	strbuf_addstr(&sb, "/config");
 
-	if (git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url))
+	if (git_config_set_in_file_gently(sb.buf, remote_key, NULL, sub_origin_url))
 		die(_("failed to update remote for submodule '%s'"),
 		      path);
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9c76b62b02d..a20cc8820e5 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -365,12 +365,12 @@ static void copy_filtered_worktree_config(const char *worktree_git_dir)
 		if (!git_configset_get_bool(&cs, "core.bare", &bare) &&
 			bare &&
 			git_config_set_multivar_in_file_gently(
-				to_file, "core.bare", NULL, "true", 0))
+				to_file, "core.bare", NULL, "true", NULL, 0))
 			error(_("failed to unset '%s' in '%s'"),
 				"core.bare", to_file);
 		if (!git_configset_get(&cs, "core.worktree") &&
 			git_config_set_in_file_gently(to_file,
-							"core.worktree", NULL))
+							"core.worktree", NULL, NULL))
 			error(_("failed to unset '%s' in '%s'"),
 				"core.worktree", to_file);
 
diff --git a/config.c b/config.c
index 3cfeb3d8bd9..a22594eabd9 100644
--- a/config.c
+++ b/config.c
@@ -3001,6 +3001,7 @@ static ssize_t write_section(int fd, const char *key,
 }
 
 static ssize_t write_pair(int fd, const char *key, const char *value,
+			  const char *comment,
 			  const struct config_store_data *store)
 {
 	int i;
@@ -3041,7 +3042,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;
 }
diff --git a/config.h b/config.h
index 5dba984f770..a85a8271696 100644
--- a/config.h
+++ b/config.h
@@ -290,7 +290,7 @@ int git_config_pathname(const char **, const char *, const char *);
 
 int git_config_expiry_date(timestamp_t *, const char *, const char *);
 int git_config_color(char *, const char *, const char *);
-int git_config_set_in_file_gently(const char *, const char *, const char *);
+int git_config_set_in_file_gently(const char *, const char *, const char *, const char *);
 
 /**
  * write config values to a specific config file, takes a key/value pair as
@@ -336,7 +336,7 @@ int git_config_parse_key(const char *, char **, size_t *);
 int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);
 void git_config_set_multivar(const char *, const char *, const char *, unsigned);
 int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);
-int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
+int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, const char *, unsigned);
 
 /**
  * takes four parameters:
diff --git a/sequencer.c b/sequencer.c
index f49a871ac06..4c91ca5a844 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3460,54 +3460,54 @@ static int save_opts(struct replay_opts *opts)
 
 	if (opts->no_commit)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.no-commit", "true");
+					"options.no-commit", NULL, "true");
 	if (opts->edit >= 0)
-		res |= git_config_set_in_file_gently(opts_file, "options.edit",
+		res |= git_config_set_in_file_gently(opts_file, "options.edit", NULL,
 						     opts->edit ? "true" : "false");
 	if (opts->allow_empty)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.allow-empty", "true");
+					"options.allow-empty", NULL, "true");
 	if (opts->allow_empty_message)
 		res |= git_config_set_in_file_gently(opts_file,
-				"options.allow-empty-message", "true");
+				"options.allow-empty-message", NULL, "true");
 	if (opts->keep_redundant_commits)
 		res |= git_config_set_in_file_gently(opts_file,
-				"options.keep-redundant-commits", "true");
+				"options.keep-redundant-commits", NULL, "true");
 	if (opts->signoff)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.signoff", "true");
+					"options.signoff", NULL, "true");
 	if (opts->record_origin)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.record-origin", "true");
+					"options.record-origin", NULL, "true");
 	if (opts->allow_ff)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.allow-ff", "true");
+					"options.allow-ff", NULL, "true");
 	if (opts->mainline) {
 		struct strbuf buf = STRBUF_INIT;
 		strbuf_addf(&buf, "%d", opts->mainline);
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.mainline", buf.buf);
+					"options.mainline", NULL, buf.buf);
 		strbuf_release(&buf);
 	}
 	if (opts->strategy)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.strategy", opts->strategy);
+					"options.strategy", NULL, opts->strategy);
 	if (opts->gpg_sign)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.gpg-sign", opts->gpg_sign);
+					"options.gpg-sign", NULL, opts->gpg_sign);
 	for (size_t i = 0; i < opts->xopts.nr; i++)
 		res |= git_config_set_multivar_in_file_gently(opts_file,
 				"options.strategy-option",
-				opts->xopts.v[i], "^$", 0);
+				opts->xopts.v[i], "^$", NULL, 0);
 	if (opts->allow_rerere_auto)
 		res |= git_config_set_in_file_gently(opts_file,
-				"options.allow-rerere-auto",
+				"options.allow-rerere-auto", NULL,
 				opts->allow_rerere_auto == RERERE_AUTOUPDATE ?
 				"true" : "false");
 
 	if (opts->explicit_cleanup)
 		res |= git_config_set_in_file_gently(opts_file,
-				"options.default-msg-cleanup",
+				"options.default-msg-cleanup", NULL,
 				describe_cleanup_mode(opts->default_msg_cleanup));
 	return res;
 }
diff --git a/submodule-config.c b/submodule-config.c
index 54130f6a385..11428b4adad 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -978,7 +978,7 @@ int config_set_in_gitmodules_file_gently(const char *key, const char *value)
 {
 	int ret;
 
-	ret = git_config_set_in_file_gently(GITMODULES_FILE, key, value);
+	ret = git_config_set_in_file_gently(GITMODULES_FILE, key, NULL, value);
 	if (ret < 0)
 		/* Maybe the user already did that, don't error out here */
 		warning(_("Could not update .gitmodules entry %s"), key);
diff --git a/submodule.c b/submodule.c
index 213da79f661..86630932d09 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2052,7 +2052,7 @@ void submodule_unset_core_worktree(const struct submodule *sub)
 	submodule_name_to_gitdir(&config_path, the_repository, sub->name);
 	strbuf_addstr(&config_path, "/config");
 
-	if (git_config_set_in_file_gently(config_path.buf, "core.worktree", NULL))
+	if (git_config_set_in_file_gently(config_path.buf, "core.worktree", NULL, NULL))
 		warning(_("Could not unset core.worktree setting in submodule '%s'"),
 			  sub->path);
 
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 31c38786870..daddaedd23c 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -69,13 +69,18 @@ test_expect_success 'replace with non-match (actually matching)' '
 
 cat > expect << EOF
 [section]
-	penguin = very blue
 	Movie = BadPhysics
 	UPPERCASE = true
-	penguin = kingpin
+	penguin = gentoo #Pygoscelis papua
+	disposition = peckish #find fish
 [Sections]
 	WhatEver = Second
 EOF
+test_expect_success 'append comments' '
+	git config --replace-all --comment="Pygoscelis papua" section.penguin gentoo &&
+	git config --comment="find fish" section.disposition peckish &&
+	test_cmp expect .git/config
+'
 
 test_expect_success 'non-match result' 'test_cmp expect .git/config'
 
diff --git a/worktree.c b/worktree.c
index b02a05a74a3..cf5eea8c931 100644
--- a/worktree.c
+++ b/worktree.c
@@ -807,9 +807,9 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
 static int move_config_setting(const char *key, const char *value,
 			       const char *from_file, const char *to_file)
 {
-	if (git_config_set_in_file_gently(to_file, key, value))
+	if (git_config_set_in_file_gently(to_file, key, NULL, value))
 		return error(_("unable to set %s in '%s'"), key, to_file);
-	if (git_config_set_in_file_gently(from_file, key, NULL))
+	if (git_config_set_in_file_gently(from_file, key, NULL, NULL))
 		return error(_("unable to unset %s in '%s'"), key, from_file);
 	return 0;
 }

base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
-- 
gitgitgadget

Re: [PATCH] Allow git-config to append a comment

[Junio C Hamano]

"Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ralph Seichter <github@seichter.de>
> Subject: Re: [PATCH] Allow git-config to append a comment

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

> 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

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.

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

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.

Re: [PATCH] Allow git-config to append a comment

[Ralph Seichter]

* Junio C Hamano:

 > Make sure your title will mix well in "git shortlog --no-merges"
 > output from recent commits by other contributors.

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.

 >> git config --comment "I changed this. --A. Script" \
 >>   --add safe.directory /home/alice/somerepo.git
 >
 > 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?

The result of running the above command looks as follows:

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

 > Why are you adding "# comment" to your config file? Who reads these
 > comments, with what tool, and for what purpose?

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.

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

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.

 > Can we come up with a code that reliably decides when to remove the
 > first comment we see above?

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.

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

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.

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

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

Re: [PATCH] Allow git-config to append a comment

[Junio C Hamano]

Ralph Seichter <github@seichter.de> writes:

>> 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?
>
> The result of running the above command looks as follows:
>
>   [safe]
> 	directory = /home/alice/somerepo.git #I changed this. --A. Script

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.

> ... My patch only supports
> single-line comments, and only as a suffix to newly added key-value
> pairs. This is a deliberate design choice.

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.

Re: [PATCH] Allow git-config to append a comment

[Ralph Seichter]

* Junio C. Hamano:

>> My patch only supports single-line comments, and only as a suffix to
>> newly added key-value pairs. This is a deliberate design choice.
>
> 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.

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

Re: [PATCH] Allow git-config to append a comment

[Junio C Hamano]

Ralph Seichter <ralph@seichter.de> writes:

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

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.

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

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.

RE: [PATCH] Allow git-config to append a comment

[rsbecker]

On Thursday, March 7, 2024 7:12 AM, Junio C Hamano wrote:
>Ralph Seichter <github@seichter.de> writes:
>
>>> 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?
>>
>> The result of running the above command looks as follows:
>>
>>   [safe]
>> 	directory = /home/alice/somerepo.git #I changed this. --A. Script
>
>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.
>
>> ... My patch only supports
>> single-line comments, and only as a suffix to newly added key-value
>> pairs. This is a deliberate design choice.
>
>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.

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

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

[Ralph Seichter via GitGitGadget]

From: Ralph Seichter <github@seichter.de>

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.

Signed-off-by: Ralph Seichter <github@seichter.de>
---
    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 <github@seichter.de>
      
       ## 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.
      
          Signed-off-by: Ralph Seichter <github@seichter.de>
      


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

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index dff39093b5e..ee8cd251b24 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,9 +9,9 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [--type=<type>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]]
-'git config' [<file-option>] [--type=<type>] --add <name> <value>
-'git config' [<file-option>] [--type=<type>] [--fixed-value] --replace-all <name> <value> [<value-pattern>]
+'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]]
+'git config' [<file-option>] [--type=<type>] [--comment=<value>] --add <name> <value>
+'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] --replace-all <name> <value> [<value-pattern>]
 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get <name> [<value-pattern>]
 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all <name> [<value-pattern>]
 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp <name-regex> [<value-pattern>]
@@ -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
diff --git a/builtin/config.c b/builtin/config.c
index b55bfae7d66..2aab3c0baf3 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -44,6 +44,7 @@ static struct config_options config_options;
 static int show_origin;
 static int show_scope;
 static int fixed_value;
+static const char *comment;
 
 #define ACTION_GET (1<<0)
 #define ACTION_GET_ALL (1<<1)
@@ -173,6 +174,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")),
 	OPT_BOOL(0, "show-scope", &show_scope, N_("show scope of config (worktree, local, global, system, command)")),
 	OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")),
+	OPT_STRING(0, "comment", &comment, N_("value"), N_("human-readable comment string (# will be prepended automatically)")),
 	OPT_END(),
 };
 
@@ -797,6 +799,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();
diff --git a/builtin/gc.c b/builtin/gc.c
index cb80ced6cb5..342907f7bdb 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1553,7 +1553,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
 			die(_("$HOME not set"));
 		rc = git_config_set_multivar_in_file_gently(
 			config_file, "maintenance.repo", maintpath,
-			CONFIG_REGEX_NONE, 0);
+			CONFIG_REGEX_NONE, NULL, 0);
 		free(global_config_file);
 
 		if (rc)
@@ -1620,7 +1620,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
 		if (!config_file)
 			die(_("$HOME not set"));
 		rc = git_config_set_multivar_in_file_gently(
-			config_file, key, NULL, maintpath,
+			config_file, key, NULL, maintpath, NULL,
 			CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);
 		free(global_config_file);
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fda50f2af1e..e4e18adb575 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1283,7 +1283,7 @@ static void sync_submodule(const char *path, const char *prefix,
 	submodule_to_gitdir(&sb, path);
 	strbuf_addstr(&sb, "/config");
 
-	if (git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url))
+	if (git_config_set_in_file_gently(sb.buf, remote_key, NULL, sub_origin_url))
 		die(_("failed to update remote for submodule '%s'"),
 		      path);
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9c76b62b02d..a20cc8820e5 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -365,12 +365,12 @@ static void copy_filtered_worktree_config(const char *worktree_git_dir)
 		if (!git_configset_get_bool(&cs, "core.bare", &bare) &&
 			bare &&
 			git_config_set_multivar_in_file_gently(
-				to_file, "core.bare", NULL, "true", 0))
+				to_file, "core.bare", NULL, "true", NULL, 0))
 			error(_("failed to unset '%s' in '%s'"),
 				"core.bare", to_file);
 		if (!git_configset_get(&cs, "core.worktree") &&
 			git_config_set_in_file_gently(to_file,
-							"core.worktree", NULL))
+							"core.worktree", NULL, NULL))
 			error(_("failed to unset '%s' in '%s'"),
 				"core.worktree", to_file);
 
diff --git a/config.c b/config.c
index 3cfeb3d8bd9..a22594eabd9 100644
--- a/config.c
+++ b/config.c
@@ -3001,6 +3001,7 @@ static ssize_t write_section(int fd, const char *key,
 }
 
 static ssize_t write_pair(int fd, const char *key, const char *value,
+			  const char *comment,
 			  const struct config_store_data *store)
 {
 	int i;
@@ -3041,7 +3042,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;
 }
diff --git a/config.h b/config.h
index 5dba984f770..a85a8271696 100644
--- a/config.h
+++ b/config.h
@@ -290,7 +290,7 @@ int git_config_pathname(const char **, const char *, const char *);
 
 int git_config_expiry_date(timestamp_t *, const char *, const char *);
 int git_config_color(char *, const char *, const char *);
-int git_config_set_in_file_gently(const char *, const char *, const char *);
+int git_config_set_in_file_gently(const char *, const char *, const char *, const char *);
 
 /**
  * write config values to a specific config file, takes a key/value pair as
@@ -336,7 +336,7 @@ int git_config_parse_key(const char *, char **, size_t *);
 int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);
 void git_config_set_multivar(const char *, const char *, const char *, unsigned);
 int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);
-int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
+int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, const char *, unsigned);
 
 /**
  * takes four parameters:
diff --git a/sequencer.c b/sequencer.c
index f49a871ac06..4c91ca5a844 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3460,54 +3460,54 @@ static int save_opts(struct replay_opts *opts)
 
 	if (opts->no_commit)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.no-commit", "true");
+					"options.no-commit", NULL, "true");
 	if (opts->edit >= 0)
-		res |= git_config_set_in_file_gently(opts_file, "options.edit",
+		res |= git_config_set_in_file_gently(opts_file, "options.edit", NULL,
 						     opts->edit ? "true" : "false");
 	if (opts->allow_empty)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.allow-empty", "true");
+					"options.allow-empty", NULL, "true");
 	if (opts->allow_empty_message)
 		res |= git_config_set_in_file_gently(opts_file,
-				"options.allow-empty-message", "true");
+				"options.allow-empty-message", NULL, "true");
 	if (opts->keep_redundant_commits)
 		res |= git_config_set_in_file_gently(opts_file,
-				"options.keep-redundant-commits", "true");
+				"options.keep-redundant-commits", NULL, "true");
 	if (opts->signoff)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.signoff", "true");
+					"options.signoff", NULL, "true");
 	if (opts->record_origin)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.record-origin", "true");
+					"options.record-origin", NULL, "true");
 	if (opts->allow_ff)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.allow-ff", "true");
+					"options.allow-ff", NULL, "true");
 	if (opts->mainline) {
 		struct strbuf buf = STRBUF_INIT;
 		strbuf_addf(&buf, "%d", opts->mainline);
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.mainline", buf.buf);
+					"options.mainline", NULL, buf.buf);
 		strbuf_release(&buf);
 	}
 	if (opts->strategy)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.strategy", opts->strategy);
+					"options.strategy", NULL, opts->strategy);
 	if (opts->gpg_sign)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.gpg-sign", opts->gpg_sign);
+					"options.gpg-sign", NULL, opts->gpg_sign);
 	for (size_t i = 0; i < opts->xopts.nr; i++)
 		res |= git_config_set_multivar_in_file_gently(opts_file,
 				"options.strategy-option",
-				opts->xopts.v[i], "^$", 0);
+				opts->xopts.v[i], "^$", NULL, 0);
 	if (opts->allow_rerere_auto)
 		res |= git_config_set_in_file_gently(opts_file,
-				"options.allow-rerere-auto",
+				"options.allow-rerere-auto", NULL,
 				opts->allow_rerere_auto == RERERE_AUTOUPDATE ?
 				"true" : "false");
 
 	if (opts->explicit_cleanup)
 		res |= git_config_set_in_file_gently(opts_file,
-				"options.default-msg-cleanup",
+				"options.default-msg-cleanup", NULL,
 				describe_cleanup_mode(opts->default_msg_cleanup));
 	return res;
 }
diff --git a/submodule-config.c b/submodule-config.c
index 54130f6a385..11428b4adad 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -978,7 +978,7 @@ int config_set_in_gitmodules_file_gently(const char *key, const char *value)
 {
 	int ret;
 
-	ret = git_config_set_in_file_gently(GITMODULES_FILE, key, value);
+	ret = git_config_set_in_file_gently(GITMODULES_FILE, key, NULL, value);
 	if (ret < 0)
 		/* Maybe the user already did that, don't error out here */
 		warning(_("Could not update .gitmodules entry %s"), key);
diff --git a/submodule.c b/submodule.c
index 213da79f661..86630932d09 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2052,7 +2052,7 @@ void submodule_unset_core_worktree(const struct submodule *sub)
 	submodule_name_to_gitdir(&config_path, the_repository, sub->name);
 	strbuf_addstr(&config_path, "/config");
 
-	if (git_config_set_in_file_gently(config_path.buf, "core.worktree", NULL))
+	if (git_config_set_in_file_gently(config_path.buf, "core.worktree", NULL, NULL))
 		warning(_("Could not unset core.worktree setting in submodule '%s'"),
 			  sub->path);
 
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 31c38786870..daddaedd23c 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -69,13 +69,18 @@ test_expect_success 'replace with non-match (actually matching)' '
 
 cat > expect << EOF
 [section]
-	penguin = very blue
 	Movie = BadPhysics
 	UPPERCASE = true
-	penguin = kingpin
+	penguin = gentoo #Pygoscelis papua
+	disposition = peckish #find fish
 [Sections]
 	WhatEver = Second
 EOF
+test_expect_success 'append comments' '
+	git config --replace-all --comment="Pygoscelis papua" section.penguin gentoo &&
+	git config --comment="find fish" section.disposition peckish &&
+	test_cmp expect .git/config
+'
 
 test_expect_success 'non-match result' 'test_cmp expect .git/config'
 
diff --git a/worktree.c b/worktree.c
index b02a05a74a3..cf5eea8c931 100644
--- a/worktree.c
+++ b/worktree.c
@@ -807,9 +807,9 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
 static int move_config_setting(const char *key, const char *value,
 			       const char *from_file, const char *to_file)
 {
-	if (git_config_set_in_file_gently(to_file, key, value))
+	if (git_config_set_in_file_gently(to_file, key, NULL, value))
 		return error(_("unable to set %s in '%s'"), key, to_file);
-	if (git_config_set_in_file_gently(from_file, key, NULL))
+	if (git_config_set_in_file_gently(from_file, key, NULL, NULL))
 		return error(_("unable to unset %s in '%s'"), key, from_file);
 	return 0;
 }

base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
-- 
gitgitgadget

RE: [PATCH] Allow git-config to append a comment

[Ralph Seichter]

* rsbecker@nexbridge.com:

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

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.

> I worry that this might unintentionally introduce incompatibilities
> into repository configurations.

Do you have an example?

-Ralph

RE: [PATCH] Allow git-config to append a comment

[rsbecker]

On Thursday, March 7, 2024 10:26 AM, Ralph Seichter wrote:
>* rsbecker@nexbridge.com:
>
>> 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.
>
>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.
>
>> I worry that this might unintentionally introduce incompatibilities
>> into repository configurations.
>
>Do you have an example?

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.

RE: [PATCH] Allow git-config to append a comment

[Ralph Seichter]

* rsbecker@nexbridge.com:

> My purpose is just to highlight, for the purpose of reviewing the
> change, that there may be unintended impacts, that's all.

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

> With git config moving comments around, responsibility shifts to git -
> a.k.a., unintended consequences.

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

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

[Junio C Hamano]

"Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ralph Seichter <github@seichter.de>
>
> Introduce the ability to append comments to modifications
> made using git-config. Example usage:
>
>   git config --comment "changed via script" \
>     --add safe.directory /home/alice/repo.git
>
> based on the proposed patch, the output produced is:
>
>   [safe]
>     directory = /home/alice/repo.git #changed via script

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

	variable = "value" # message comes here

> * Motivation:
>
> The use case which triggered my submitting this patch is

These two lines should not be needed.  It is customary to just state
that the users need to be able to do X and adding feature Y is one
way to allow that, without such preamble.

> my need to distinguish between config entries made using
> automation and entries made by a human. Automation can
> add comments containing a URL pointing to explanations
> for the change made, avoiding questions from users as to
> why their config file was changed by a third party.
>
> * Design considerations and implementation details:
>
> The implementation ensures that a # character is always
> prepended to the provided comment string, and that the

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

> comment text is appended as a suffix to the changed key-
> value-pair in the same line of text. Multiline comments
> are deliberately not supported, because their usefulness

"not supported" can take many shapes, ranging from "producing a
random result and may corrupt the resulting configuration file"
and "the second and subsequent lines are silently ignored", to
"results in an error, stops the command before doing anything".

> does not justifiy the possible problems they pose when
> it comes to removing ML comments later.

"justify"

>
> * Target audience:
>
> Regarding the intended consumers of the comments made:

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

> They are aimed at humans who inspect or change their Git
> config using a pager or editor. Comments are not meant
> to be read or displayed by git-config at a later time.
>
> Signed-off-by: Ralph Seichter <github@seichter.de>
> ---
>     config: add --comment option to add a comment
>
>     config: add --comment option to add a comment
>      
>     Introduce the ability to append comments to modifications made using
> ...
>        displayed by git-config at a later time.

No need to repeat a wall of text below the three-dash line if they
do not give additional information on top of what is already in the
proposed commit log message.

> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index dff39093b5e..ee8cd251b24 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -9,9 +9,9 @@ git-config - Get and set repository or global options
>  SYNOPSIS
>  --------
>  [verse]
> -'git config' [<file-option>] [--type=<type>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]]
> -'git config' [<file-option>] [--type=<type>] --add <name> <value>
> -'git config' [<file-option>] [--type=<type>] [--fixed-value] --replace-all <name> <value> [<value-pattern>]
> +'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]]
> +'git config' [<file-option>] [--type=<type>] [--comment=<value>] --add <name> <value>
> +'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] --replace-all <name> <value> [<value-pattern>]
>  'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get <name> [<value-pattern>]
>  'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all <name> [<value-pattern>]
>  'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp <name-regex> [<value-pattern>]

There is Patrick's proposal to revamp the UI of this command,

  https://lore.kernel.org/git/cover.1709724089.git.ps@pks.im/

which may make the above simpler (e.g., there won't be two lines
that talks about "set" commands, with and without "--add" option,
for example).  This topic may want to at least wait for the
conclusion of that design discussion, and possibly its
implementation.

> @@ -87,6 +87,10 @@ OPTIONS
>  	values.  This is the same as providing '^$' as the `value-pattern`
>  	in `--replace-all`.
>  
> +--comment::
> +	Append a comment to new or modified lines. A '#' character
> +	will be automatically prepended to the value.

Other options that take mandatory parameter are are described with
their parameter on the item heading line.  It may help to somehow
mention that the "appending" is done at the end on the same line.
Perhaps something along this line.

	--comment <message>::
		Append the _<message>_ at the end of the new or
		modified lines with the value.  A comment introducer
		` # ` is prepended to _<message>_ if it does not
		already have one.  The command errors out if given
		a _<message>_ that spans multiple lines.


> @@ -797,6 +799,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  		usage_builtin_config();
>  	}
>  
> +	if (comment && !(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) {

This overly long line is both visually annoying and harder to
maintain.  If you can design a solution that makes it easier to read
a future patch that adds support of the comment option to more
actions (or removes it from an action), that would be great.
Otherwise, do at least:

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

> +		error(_("--comment is only applicable to add/set/replace operations"));
> +		usage_builtin_config();
> +	}
> +

If I were doing this, I'd probably add this new block after the
fixed-value thing, as it is bad manners to add new code _before_
existing ones, as if your new invention were somehow more important
than all the others, when the order does not matter.  If there is a
valid technical reason (e.g., the new code modifies the state of the
program that affects the beahviour of the later and existing parts
of the code), the above comment of course does not apply.

>  	/* check usage of --fixed-value */
>  	if (fixed_value) {
>  		int allowed_usage = 0;


> @@ -880,7 +887,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
>  		check_write();
>  		check_argc(argc, 2, 2);
>  		value = normalize_value(argv[0], argv[1], &default_kvi);
> -		ret = git_config_set_in_file_gently(given_config_source.file, argv[0], value);
> +		ret = git_config_set_in_file_gently(given_config_source.file, argv[0], comment, value);

It is somewhat annoying that we have to see so much code churn to
add this new parameter X-<.  I notice that we are sending "comment"
to the underlying machinery without doing ANY sanity checking (like,
"ah, we got a message without the '#' prefix, so we should add '# '
in front of it before asking the API to add that comment string to
the value line").  We may also want to have a code that says:

    Yikes, this message has LF in it, but the underlying machinery
    does not check for it and end up corrupting the configuration
    file by creating

	[section]
		var = value #comment
	that spans on two lines

    when given

	LF='
	'
        git config --comment="comment${LF}that spans..." section.var value

or something.  The underlying machinery should be updated to die()
when given such a message instead of silently corrupting the
resulting file, but the front-end that receives the end-user input
should check for obviously problematic payload before bothering the
API with it.

> -	strbuf_addf(&sb, "%s\n", quote);
> +	if (comment)
> +		strbuf_addf(&sb, "%s #%s\n", quote, comment);
> +	else
> +		strbuf_addf(&sb, "%s\n", quote);

> diff --git a/config.h b/config.h
> index 5dba984f770..a85a8271696 100644
> --- a/config.h
> +++ b/config.h
> @@ -290,7 +290,7 @@ int git_config_pathname(const char **, const char *, const char *);
>  
>  int git_config_expiry_date(timestamp_t *, const char *, const char *);
>  int git_config_color(char *, const char *, const char *);
> -int git_config_set_in_file_gently(const char *, const char *, const char *);

The original was probably OK without naming these parameters of the
same type, as it is (arguably) obvious to have the filename first,
because that is what differenciates the "in-file" variant.  And then
key followed by value, because that is the usual "assignment" order
people would naturally expect.

But it was already on the borderline.  The idea is that we do not
have to (but still can) name the parameters in our function
declarations when it is obvious from their types what they are.
Addition of this comment thing will push us way over the line.

The same comment applies to many of the function declarations
touched by this patch.

If I were doing this patch, I'd have at least one clean-up patch
that updates this line to read:

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

And then write a separate patch to add the "--comment" feature.

I however have some suspicion that we might move away from "listing
many random parameters" style to "having only the essential
parameters, together with a single pointer to a structure that holds
the optional bits" style, especially when the UI revamp Patrick
proposed comes.  In the case of config API, <key, value> is the
essential bit in the write direction, and value-pattern restriction
and the bit that says key is an regexp would be in "the optional
bits" group.  This <comment> thing will also be in the latter class.

> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index 31c38786870..daddaedd23c 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -69,13 +69,18 @@ test_expect_success 'replace with non-match (actually matching)' '
>  
>  cat > expect << EOF
>  [section]
> -	penguin = very blue
>  	Movie = BadPhysics
>  	UPPERCASE = true
> -	penguin = kingpin
> +	penguin = gentoo #Pygoscelis papua
> +	disposition = peckish #find fish
>  [Sections]
>  	WhatEver = Second
>  EOF
> +test_expect_success 'append comments' '
> +	git config --replace-all --comment="Pygoscelis papua" section.penguin gentoo &&
> +	git config --comment="find fish" section.disposition peckish &&
> +	test_cmp expect .git/config
> +'

Add test for at least two more cases and show what should happen.

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

Thanks.

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

[Dragan Simic]

On 2024-03-11 13:55, Junio C Hamano wrote:
> "Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Ralph Seichter <github@seichter.de>
>> 
>> Introduce the ability to append comments to modifications
>> made using git-config. Example usage:
>> 
>>   git config --comment "changed via script" \
>>     --add safe.directory /home/alice/repo.git
>> 
>> based on the proposed patch, the output produced is:
>> 
>>   [safe]
>>     directory = /home/alice/repo.git #changed via script
> 
> For readability, you probably would want to have a SP before the
> given string, i.e.,
> 
> 	variable = "value" # message comes here

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

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

[rsbecker]

On Monday, March 11, 2024 12:17 PM, Dragan Simic wrote:
>Subject: Re: [PATCH v2] config: add --comment option to add a comment
>
>On 2024-03-11 13:55, Junio C Hamano wrote:
>> "Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> From: Ralph Seichter <github@seichter.de>
>>>
>>> Introduce the ability to append comments to modifications made using
>>> git-config. Example usage:
>>>
>>>   git config --comment "changed via script" \
>>>     --add safe.directory /home/alice/repo.git
>>>
>>> based on the proposed patch, the output produced is:
>>>
>>>   [safe]
>>>     directory = /home/alice/repo.git #changed via script
>>
>> For readability, you probably would want to have a SP before the given
>> string, i.e.,
>>
>> 	variable = "value" # message comes here
>
>Let me interject...  Perhaps also a tab character before the "# comment",
instead of a space character.  That would result in even better
>readability.

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.

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

[Dragan Simic]

On 2024-03-11 17:48, rsbecker@nexbridge.com wrote:
> On Monday, March 11, 2024 12:17 PM, Dragan Simic wrote:
>> Subject: Re: [PATCH v2] config: add --comment option to add a comment
>> 
>> On 2024-03-11 13:55, Junio C Hamano wrote:
>>> "Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>> 
>>>> From: Ralph Seichter <github@seichter.de>
>>>> 
>>>> Introduce the ability to append comments to modifications made using
>>>> git-config. Example usage:
>>>> 
>>>>   git config --comment "changed via script" \
>>>>     --add safe.directory /home/alice/repo.git
>>>> 
>>>> based on the proposed patch, the output produced is:
>>>> 
>>>>   [safe]
>>>>     directory = /home/alice/repo.git #changed via script
>>> 
>>> For readability, you probably would want to have a SP before the 
>>> given
>>> string, i.e.,
>>> 
>>> 	variable = "value" # message comes here
>> 
>> Let me interject...  Perhaps also a tab character before the "# 
>> comment",
> instead of a space character.  That would result in even better
>> readability.
> 
> 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?

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

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

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

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

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

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> On 2024-03-11 13:55, Junio C Hamano wrote:
>> "Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> 
>>> From: Ralph Seichter <github@seichter.de>
>>> Introduce the ability to append comments to modifications
>>> made using git-config. Example usage:
>>>   git config --comment "changed via script" \
>>>     --add safe.directory /home/alice/repo.git
>>> based on the proposed patch, the output produced is:
>>>   [safe]
>>>     directory = /home/alice/repo.git #changed via script
>> For readability, you probably would want to have a SP before the
>> given string, i.e.,
>> 	variable = "value" # message comes here
>
> Let me interject...  Perhaps also a tab character before the "#
> comment",
> instead of a space character.  That would result in even better
> readability.

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.

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

[Dragan Simic]

On 2024-03-11 18:29, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
>> On 2024-03-11 13:55, Junio C Hamano wrote:
>>> "Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>> 
>>>> From: Ralph Seichter <github@seichter.de>
>>>> Introduce the ability to append comments to modifications
>>>> made using git-config. Example usage:
>>>>   git config --comment "changed via script" \
>>>>     --add safe.directory /home/alice/repo.git
>>>> based on the proposed patch, the output produced is:
>>>>   [safe]
>>>>     directory = /home/alice/repo.git #changed via script
>>> For readability, you probably would want to have a SP before the
>>> given string, i.e.,
>>> 	variable = "value" # message comes here
>> 
>> Let me interject...  Perhaps also a tab character before the "#
>> comment",
>> instead of a space character.  That would result in even better
>> readability.
> 
> Depends on your screen width ;-)

Ah, screens are pretty wide these days. :)

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

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

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

[Dragan Simic]

On 2024-03-11 18:00, Dragan Simic wrote:
> On 2024-03-11 17:48, rsbecker@nexbridge.com wrote:
>> On Monday, March 11, 2024 12:17 PM, Dragan Simic wrote:
>>> Subject: Re: [PATCH v2] config: add --comment option to add a comment
>>> 
>>> On 2024-03-11 13:55, Junio C Hamano wrote:
>>>> "Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>>> 
>>>>> From: Ralph Seichter <github@seichter.de>
>>>>> 
>>>>> Introduce the ability to append comments to modifications made 
>>>>> using
>>>>> git-config. Example usage:
>>>>> 
>>>>>   git config --comment "changed via script" \
>>>>>     --add safe.directory /home/alice/repo.git
>>>>> 
>>>>> based on the proposed patch, the output produced is:
>>>>> 
>>>>>   [safe]
>>>>>     directory = /home/alice/repo.git #changed via script
>>>> 
>>>> For readability, you probably would want to have a SP before the 
>>>> given
>>>> string, i.e.,
>>>> 
>>>> 	variable = "value" # message comes here
>>> 
>>> Let me interject...  Perhaps also a tab character before the "# 
>>> comment",
>> instead of a space character.  That would result in even better
>>> readability.
>> 
>> 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?
> 
> It should mean "value(TAB)s", according to git-config(1).

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?

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

After some testing, I can confirm that any tabs (or spaces, or mixes) 
between
the value and the "#" character do get 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.
> 
> ... I do agree that it should be clarified further in the man page.

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

[Ralph Seichter]

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:

 > For readability, you probably would want to have a SP before the
 > given string, i.e.,
 >
 > 	variable = "value" # message comes here

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.

 >> The implementation ensures that a # character is always
 >> prepended to the provided comment string, and that the
 >
 > It is unclear what happens when a user gives comment that already
 > has the comment introducer, e.g., --comment="# this is comment".

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.

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

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.

 > This topic may want to at least wait for the conclusion of that
 > design discussion, and possibly its implementation.

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

 > I notice that we are sending "comment" to the underlying machinery
 > without doing ANY sanity checking (like, "ah, we got a message
 > without the '#' prefix, so we should add '# ' in front of it before
 > asking the API to add that comment string to the value line").

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

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

[Ralph Seichter]

* Dragan Simic:

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

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

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

[Dragan Simic]

On 2024-03-11 19:23, Ralph Seichter wrote:
> * Dragan Simic:
> 
>> Let me interject... Perhaps also a tab character before the "#
>> comment", instead of a space character. That would result in even
>> better readability.
> 
> 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.

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.

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

[Dragan Simic]

On 2024-03-11 19:16, Ralph Seichter wrote:
> * Junio C Hamano:
> 
>> For readability, you probably would want to have a SP before the
>> given string, i.e.,
>> 
>> 	variable = "value" # message comes here
> 
> 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.

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

>>> The implementation ensures that a # character is always
>>> prepended to the provided comment string, and that the
>> 
>> It is unclear what happens when a user gives comment that already
>> has the comment introducer, e.g., --comment="# this is comment".
> 
> 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.

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

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

[Ralph Seichter]

* Dragan Simic:

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

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

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

[Dragan Simic]

On 2024-03-11 19:57, Ralph Seichter wrote:
> * Dragan Simic:
> 
>> 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.
> 
> 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.

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.

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

[Ralph Seichter]

* Dragan Simic:

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

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

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

[rsbecker]

On Monday, March 11, 2024 2:57 PM, Ralph Seichter wrote:
>Subject: Re: [PATCH v2] config: add --comment option to add a comment
>
>* Dragan Simic:
>
> > 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.
>
>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.

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

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

>>> Let me interject...  Perhaps also a tab character before the "#
>>> comment",
>>> instead of a space character.  That would result in even better
>>> readability.
>> Depends on your screen width ;-)
>
> Ah, screens are pretty wide these days. :)
>
>> 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.
>
> 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.

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

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

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

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

[Dragan Simic]

On 2024-03-11 20:54, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>>>> Let me interject...  Perhaps also a tab character before the "#
>>>> comment",
>>>> instead of a space character.  That would result in even better
>>>> readability.
>>> Depends on your screen width ;-)
>> 
>> Ah, screens are pretty wide these days. :)
>> 
>>> 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.
>> 
>> 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 '#'.

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

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

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.

> 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

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

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

Agreed.  That would be nice.

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

[Dragan Simic]

On 2024-03-11 20:17, rsbecker@nexbridge.com wrote:
> 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.

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.

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

[Dragan Simic]

On 2024-03-11 22:31, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> 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,

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.

> 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 agree with that, as I already noted in an earlier reply.

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

[Ralph Seichter]

* Ralph Seichter:

 > LF is a more interesting question. I haven't yet tried actively
 > introducing a linefeed. Does strbuf_addf() have any related
 > filtering logic?

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

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

[Chris Torek]

On Mon, Mar 11, 2024 at 11:22 PM Ralph Seichter <github@seichter.de> wrote:
> I have now tried several times to inject a LF into a comment string ...
> Variable expansion with LF="\012" ...

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

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

[Ralph Seichter]

* Chris Torek:

> You must use a literal line feed, e.g.:
>
> LF='
> '

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

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

[Junio C Hamano]

Ralph Seichter <github@seichter.de> writes:

> * Chris Torek:
>
>> You must use a literal line feed, e.g.:
>>
>> LF='
>> '
>
> Of course, silly me. Easily done in a shell script. I was too focused on
> trying it in an interactive shell. Thanks, Chris.

;-) 

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.

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

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

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

[Ralph Seichter via GitGitGadget]

From: Ralph Seichter <github@seichter.de>

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.

Signed-off-by: Ralph Seichter <github@seichter.de>
---
    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.
      
          Signed-off-by: Ralph Seichter <github@seichter.de>
      
     @@ 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(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index dff39093b5e..e608d5ffef2 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,9 +9,9 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [--type=<type>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]]
-'git config' [<file-option>] [--type=<type>] --add <name> <value>
-'git config' [<file-option>] [--type=<type>] [--fixed-value] --replace-all <name> <value> [<value-pattern>]
+'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]]
+'git config' [<file-option>] [--type=<type>] [--comment=<value>] --add <name> <value>
+'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] --replace-all <name> <value> [<value-pattern>]
 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get <name> [<value-pattern>]
 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all <name> [<value-pattern>]
 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp <name-regex> [<value-pattern>]
@@ -87,6 +87,11 @@ OPTIONS
 	values.  This is the same as providing '^$' as the `value-pattern`
 	in `--replace-all`.
 
+--comment <value>::
+	Append a comment to new or modified lines. A '#' character will be
+	unconditionally prepended to the value. The value must not contain
+	linefeed characters (no multi-line comments are permitted).
+
 --get::
 	Get the value for a given key (optionally filtered by a regex
 	matching the value). Returns error code 1 if the key was not
diff --git a/builtin/config.c b/builtin/config.c
index b55bfae7d66..c54e9941a5f 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -44,6 +44,7 @@ static struct config_options config_options;
 static int show_origin;
 static int show_scope;
 static int fixed_value;
+static const char *comment;
 
 #define ACTION_GET (1<<0)
 #define ACTION_GET_ALL (1<<1)
@@ -173,6 +174,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")),
 	OPT_BOOL(0, "show-scope", &show_scope, N_("show scope of config (worktree, local, global, system, command)")),
 	OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")),
+	OPT_STRING(0, "comment", &comment, N_("value"), N_("human-readable comment string (# will be prepended automatically)")),
 	OPT_END(),
 };
 
@@ -797,6 +799,12 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		usage_builtin_config();
 	}
 
+	if (comment &&
+		!(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) {
+			error(_("--comment is only applicable to add/set/replace operations"));
+			usage_builtin_config();
+	}
+
 	/* check usage of --fixed-value */
 	if (fixed_value) {
 		int allowed_usage = 0;
@@ -880,7 +888,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		check_write();
 		check_argc(argc, 2, 2);
 		value = normalize_value(argv[0], argv[1], &default_kvi);
-		ret = git_config_set_in_file_gently(given_config_source.file, argv[0], value);
+		ret = git_config_set_in_file_gently(given_config_source.file, argv[0], comment, value);
 		if (ret == CONFIG_NOTHING_SET)
 			error(_("cannot overwrite multiple values with a single value\n"
 			"       Use a regexp, --add or --replace-all to change %s."), argv[0]);
@@ -891,7 +899,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		value = normalize_value(argv[0], argv[1], &default_kvi);
 		ret = git_config_set_multivar_in_file_gently(given_config_source.file,
 							     argv[0], value, argv[2],
-							     flags);
+							     comment, flags);
 	}
 	else if (actions == ACTION_ADD) {
 		check_write();
@@ -900,7 +908,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		ret = git_config_set_multivar_in_file_gently(given_config_source.file,
 							     argv[0], value,
 							     CONFIG_REGEX_NONE,
-							     flags);
+							     comment, flags);
 	}
 	else if (actions == ACTION_REPLACE_ALL) {
 		check_write();
@@ -908,7 +916,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		value = normalize_value(argv[0], argv[1], &default_kvi);
 		ret = git_config_set_multivar_in_file_gently(given_config_source.file,
 							     argv[0], value, argv[2],
-							     flags | CONFIG_FLAGS_MULTI_REPLACE);
+							     comment, flags | CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_GET) {
 		check_argc(argc, 1, 2);
@@ -936,17 +944,17 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		if (argc == 2)
 			return git_config_set_multivar_in_file_gently(given_config_source.file,
 								      argv[0], NULL, argv[1],
-								      flags);
+								      NULL, flags);
 		else
 			return git_config_set_in_file_gently(given_config_source.file,
-							     argv[0], NULL);
+							     argv[0], NULL, NULL);
 	}
 	else if (actions == ACTION_UNSET_ALL) {
 		check_write();
 		check_argc(argc, 1, 2);
 		return git_config_set_multivar_in_file_gently(given_config_source.file,
 							      argv[0], NULL, argv[1],
-							      flags | CONFIG_FLAGS_MULTI_REPLACE);
+							      NULL, flags | CONFIG_FLAGS_MULTI_REPLACE);
 	}
 	else if (actions == ACTION_RENAME_SECTION) {
 		check_write();
diff --git a/builtin/gc.c b/builtin/gc.c
index cb80ced6cb5..342907f7bdb 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1553,7 +1553,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)
 			die(_("$HOME not set"));
 		rc = git_config_set_multivar_in_file_gently(
 			config_file, "maintenance.repo", maintpath,
-			CONFIG_REGEX_NONE, 0);
+			CONFIG_REGEX_NONE, NULL, 0);
 		free(global_config_file);
 
 		if (rc)
@@ -1620,7 +1620,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
 		if (!config_file)
 			die(_("$HOME not set"));
 		rc = git_config_set_multivar_in_file_gently(
-			config_file, key, NULL, maintpath,
+			config_file, key, NULL, maintpath, NULL,
 			CONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);
 		free(global_config_file);
 
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index fda50f2af1e..e4e18adb575 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1283,7 +1283,7 @@ static void sync_submodule(const char *path, const char *prefix,
 	submodule_to_gitdir(&sb, path);
 	strbuf_addstr(&sb, "/config");
 
-	if (git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url))
+	if (git_config_set_in_file_gently(sb.buf, remote_key, NULL, sub_origin_url))
 		die(_("failed to update remote for submodule '%s'"),
 		      path);
 
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9c76b62b02d..a20cc8820e5 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -365,12 +365,12 @@ static void copy_filtered_worktree_config(const char *worktree_git_dir)
 		if (!git_configset_get_bool(&cs, "core.bare", &bare) &&
 			bare &&
 			git_config_set_multivar_in_file_gently(
-				to_file, "core.bare", NULL, "true", 0))
+				to_file, "core.bare", NULL, "true", NULL, 0))
 			error(_("failed to unset '%s' in '%s'"),
 				"core.bare", to_file);
 		if (!git_configset_get(&cs, "core.worktree") &&
 			git_config_set_in_file_gently(to_file,
-							"core.worktree", NULL))
+							"core.worktree", NULL, NULL))
 			error(_("failed to unset '%s' in '%s'"),
 				"core.worktree", to_file);
 
diff --git a/config.c b/config.c
index 3cfeb3d8bd9..2f3f6d123c6 100644
--- a/config.c
+++ b/config.c
@@ -3001,6 +3001,7 @@ static ssize_t write_section(int fd, const char *key,
 }
 
 static ssize_t write_pair(int fd, const char *key, const char *value,
+			  const char *comment,
 			  const struct config_store_data *store)
 {
 	int i;
@@ -3041,7 +3042,14 @@ static ssize_t write_pair(int fd, const char *key, const char *value,
 			strbuf_addch(&sb, value[i]);
 			break;
 		}
-	strbuf_addf(&sb, "%s\n", quote);
+
+	if (comment) {
+		if (strchr(comment, '\n'))
+			die(_("multi-line comments are not permitted: '%s'"), comment);
+		else
+			strbuf_addf(&sb, "%s #%s\n", quote, comment);
+	} else
+		strbuf_addf(&sb, "%s\n", quote);
 
 	ret = write_in_full(fd, sb.buf, sb.len);
 	strbuf_release(&sb);
@@ -3130,9 +3138,9 @@ static void maybe_remove_section(struct config_store_data *store,
 }
 
 int git_config_set_in_file_gently(const char *config_filename,
-				  const char *key, const char *value)
+				  const char *key, const char *comment, const char *value)
 {
-	return git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, 0);
+	return git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, comment, 0);
 }
 
 void git_config_set_in_file(const char *config_filename,
@@ -3153,7 +3161,7 @@ int repo_config_set_worktree_gently(struct repository *r,
 	if (r->repository_format_worktree_config) {
 		char *file = repo_git_path(r, "config.worktree");
 		int ret = git_config_set_multivar_in_file_gently(
-					file, key, value, NULL, 0);
+					file, key, value, NULL, NULL, 0);
 		free(file);
 		return ret;
 	}
@@ -3195,6 +3203,7 @@ void git_config_set(const char *key, const char *value)
 int git_config_set_multivar_in_file_gently(const char *config_filename,
 					   const char *key, const char *value,
 					   const char *value_pattern,
+					   const char *comment,
 					   unsigned flags)
 {
 	int fd = -1, in_fd = -1;
@@ -3245,7 +3254,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 		free(store.key);
 		store.key = xstrdup(key);
 		if (write_section(fd, key, &store) < 0 ||
-		    write_pair(fd, key, value, &store) < 0)
+		    write_pair(fd, key, value, comment, &store) < 0)
 			goto write_err_out;
 	} else {
 		struct stat st;
@@ -3399,7 +3408,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 				if (write_section(fd, key, &store) < 0)
 					goto write_err_out;
 			}
-			if (write_pair(fd, key, value, &store) < 0)
+			if (write_pair(fd, key, value, comment, &store) < 0)
 				goto write_err_out;
 		}
 
@@ -3444,7 +3453,7 @@ void git_config_set_multivar_in_file(const char *config_filename,
 				     const char *value_pattern, unsigned flags)
 {
 	if (!git_config_set_multivar_in_file_gently(config_filename, key, value,
-						    value_pattern, flags))
+						    value_pattern, NULL, flags))
 		return;
 	if (value)
 		die(_("could not set '%s' to '%s'"), key, value);
@@ -3467,7 +3476,7 @@ int repo_config_set_multivar_gently(struct repository *r, const char *key,
 	int res = git_config_set_multivar_in_file_gently(file,
 							 key, value,
 							 value_pattern,
-							 flags);
+							 NULL, flags);
 	free(file);
 	return res;
 }
diff --git a/config.h b/config.h
index 5dba984f770..a85a8271696 100644
--- a/config.h
+++ b/config.h
@@ -290,7 +290,7 @@ int git_config_pathname(const char **, const char *, const char *);
 
 int git_config_expiry_date(timestamp_t *, const char *, const char *);
 int git_config_color(char *, const char *, const char *);
-int git_config_set_in_file_gently(const char *, const char *, const char *);
+int git_config_set_in_file_gently(const char *, const char *, const char *, const char *);
 
 /**
  * write config values to a specific config file, takes a key/value pair as
@@ -336,7 +336,7 @@ int git_config_parse_key(const char *, char **, size_t *);
 int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);
 void git_config_set_multivar(const char *, const char *, const char *, unsigned);
 int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);
-int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);
+int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, const char *, unsigned);
 
 /**
  * takes four parameters:
diff --git a/sequencer.c b/sequencer.c
index f49a871ac06..4c91ca5a844 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3460,54 +3460,54 @@ static int save_opts(struct replay_opts *opts)
 
 	if (opts->no_commit)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.no-commit", "true");
+					"options.no-commit", NULL, "true");
 	if (opts->edit >= 0)
-		res |= git_config_set_in_file_gently(opts_file, "options.edit",
+		res |= git_config_set_in_file_gently(opts_file, "options.edit", NULL,
 						     opts->edit ? "true" : "false");
 	if (opts->allow_empty)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.allow-empty", "true");
+					"options.allow-empty", NULL, "true");
 	if (opts->allow_empty_message)
 		res |= git_config_set_in_file_gently(opts_file,
-				"options.allow-empty-message", "true");
+				"options.allow-empty-message", NULL, "true");
 	if (opts->keep_redundant_commits)
 		res |= git_config_set_in_file_gently(opts_file,
-				"options.keep-redundant-commits", "true");
+				"options.keep-redundant-commits", NULL, "true");
 	if (opts->signoff)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.signoff", "true");
+					"options.signoff", NULL, "true");
 	if (opts->record_origin)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.record-origin", "true");
+					"options.record-origin", NULL, "true");
 	if (opts->allow_ff)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.allow-ff", "true");
+					"options.allow-ff", NULL, "true");
 	if (opts->mainline) {
 		struct strbuf buf = STRBUF_INIT;
 		strbuf_addf(&buf, "%d", opts->mainline);
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.mainline", buf.buf);
+					"options.mainline", NULL, buf.buf);
 		strbuf_release(&buf);
 	}
 	if (opts->strategy)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.strategy", opts->strategy);
+					"options.strategy", NULL, opts->strategy);
 	if (opts->gpg_sign)
 		res |= git_config_set_in_file_gently(opts_file,
-					"options.gpg-sign", opts->gpg_sign);
+					"options.gpg-sign", NULL, opts->gpg_sign);
 	for (size_t i = 0; i < opts->xopts.nr; i++)
 		res |= git_config_set_multivar_in_file_gently(opts_file,
 				"options.strategy-option",
-				opts->xopts.v[i], "^$", 0);
+				opts->xopts.v[i], "^$", NULL, 0);
 	if (opts->allow_rerere_auto)
 		res |= git_config_set_in_file_gently(opts_file,
-				"options.allow-rerere-auto",
+				"options.allow-rerere-auto", NULL,
 				opts->allow_rerere_auto == RERERE_AUTOUPDATE ?
 				"true" : "false");
 
 	if (opts->explicit_cleanup)
 		res |= git_config_set_in_file_gently(opts_file,
-				"options.default-msg-cleanup",
+				"options.default-msg-cleanup", NULL,
 				describe_cleanup_mode(opts->default_msg_cleanup));
 	return res;
 }
diff --git a/submodule-config.c b/submodule-config.c
index 54130f6a385..11428b4adad 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -978,7 +978,7 @@ int config_set_in_gitmodules_file_gently(const char *key, const char *value)
 {
 	int ret;
 
-	ret = git_config_set_in_file_gently(GITMODULES_FILE, key, value);
+	ret = git_config_set_in_file_gently(GITMODULES_FILE, key, NULL, value);
 	if (ret < 0)
 		/* Maybe the user already did that, don't error out here */
 		warning(_("Could not update .gitmodules entry %s"), key);
diff --git a/submodule.c b/submodule.c
index 213da79f661..86630932d09 100644
--- a/submodule.c
+++ b/submodule.c
@@ -2052,7 +2052,7 @@ void submodule_unset_core_worktree(const struct submodule *sub)
 	submodule_name_to_gitdir(&config_path, the_repository, sub->name);
 	strbuf_addstr(&config_path, "/config");
 
-	if (git_config_set_in_file_gently(config_path.buf, "core.worktree", NULL))
+	if (git_config_set_in_file_gently(config_path.buf, "core.worktree", NULL, NULL))
 		warning(_("Could not unset core.worktree setting in submodule '%s'"),
 			  sub->path);
 
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index 31c38786870..ac7b02e6b07 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -69,13 +69,24 @@ test_expect_success 'replace with non-match (actually matching)' '
 
 cat > expect << EOF
 [section]
-	penguin = very blue
 	Movie = BadPhysics
 	UPPERCASE = true
-	penguin = kingpin
+	penguin = gentoo #Pygoscelis papua
+	disposition = peckish #find fish
+	foo = bar ## abc
 [Sections]
 	WhatEver = Second
 EOF
+test_expect_success 'append comments' '
+	git config --replace-all --comment="Pygoscelis papua" section.penguin gentoo &&
+	git config --comment="find fish" section.disposition peckish &&
+	git config --comment="# abc" section.foo bar &&
+	test_cmp expect .git/config
+'
+
+test_expect_success 'Prohibited LF in comment' '
+	test_must_fail git config --comment="a${LF}b" section.k v
+'
 
 test_expect_success 'non-match result' 'test_cmp expect .git/config'
 
diff --git a/worktree.c b/worktree.c
index b02a05a74a3..cf5eea8c931 100644
--- a/worktree.c
+++ b/worktree.c
@@ -807,9 +807,9 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,
 static int move_config_setting(const char *key, const char *value,
 			       const char *from_file, const char *to_file)
 {
-	if (git_config_set_in_file_gently(to_file, key, value))
+	if (git_config_set_in_file_gently(to_file, key, NULL, value))
 		return error(_("unable to set %s in '%s'"), key, to_file);
-	if (git_config_set_in_file_gently(from_file, key, NULL))
+	if (git_config_set_in_file_gently(from_file, key, NULL, NULL))
 		return error(_("unable to unset %s in '%s'"), key, from_file);
 	return 0;
 }

base-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d
-- 
gitgitgadget

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

[Junio C Hamano]

"Ralph Seichter via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Ralph Seichter <github@seichter.de>

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

> Users need to be able to distinguish between config entries made
> using automation and entries made by a human. Automation can add
> comments containing a URL pointing to explanations for the change
> made, avoiding questions from users as to why their config file
> was changed by a third party.

While I can understand what you want to say, primarily because we
were both on this topic for some time by now, a casual reader of the
above would say:

    Sure, but a human also would add comment to clarify what
    motivated the setting to be added. The fact that one entry has
    comment does not help distinguishing between automation and
    human input all that much.

More importantly, the reason why users would want to mark entries is
not limited to telling automation vs human.  So, it seems to me that
the paragraph needs a bit more work.  Here is my attempt.

    While the subcommands (including "git config") of "git" all can
    skip comment strings in the configuration files, and users do
    write comments in the configuration files with editors, there is
    no easy way that can be used by scripts and automation to leave
    comments when they add/modify configuration entries.

    Introduce a "--comment" option that can be used with the "git
    config" command when it is used for writing, so that each
    affected entry can be annotated with a comment that comes after
    the "variable = value" on the same line.  The option argument is
    deliberately limited to a single-line comment to match the line
    oriented nature of the configuration file, and "git config" will
    error out early when fed a multi-line string.  This way, removal
    of the variable will also remove the comment given to it by this
    mechanism.

> The implementation ensures that a # character is unconditionally
> prepended to the provided comment string, and that

I thought we already discussed this and unconditional "#comment" has
already been declared a non starter.  I'll follow this review
response with a few patches that are designed to apply on top to
show a more ergonomic way to do this.

> the comment
> text is appended as a suffix to the changed key-value-pair in the
> same line of text. Multi-line comments (i.e. comments containing
> linefeed) are rejected as errors, causing Git to exit without
> making changes.

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

> Comments are aimed at humans who inspect or change their Git
> config using a pager or editor. Comments are not meant to be
> read or displayed by git-config at a later time.

As we sold why comments are useful and lack of mechanical way to add
one hurts earlier in the proposed log message, if we rewrite the
earlier paragraph like illustrated above, this would become
redundant.  I also do not think we need to limit future direction
with the last sentence.  I do not foresee a reason to forbid other
developers from adding an option to the "get" subcommand, e.g.

	$ git config --get --with-comments vari.able
	vari.able = value # added on Fri Mar 15 15:02:37 2024

> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index dff39093b5e..e608d5ffef2 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -9,9 +9,9 @@ git-config - Get and set repository or global options
>  SYNOPSIS
>  --------
>  [verse]
> -'git config' [<file-option>] [--type=<type>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]]
> -'git config' [<file-option>] [--type=<type>] --add <name> <value>
> -'git config' [<file-option>] [--type=<type>] [--fixed-value] --replace-all <name> <value> [<value-pattern>]
> +'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]]
> +'git config' [<file-option>] [--type=<type>] [--comment=<value>] --add <name> <value>
> +'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] --replace-all <name> <value> [<value-pattern>]
>  'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get <name> [<value-pattern>]
>  'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all <name> [<value-pattern>]
>  'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp <name-regex> [<value-pattern>]

"--comment=<value>" will become awkward when we write about how the
comment message is placed after the value in the description.
You'll see, in the patch I promised to follow this review with,
I've used --comment=<message> instead to avoid that issue.

> @@ -87,6 +87,11 @@ OPTIONS
>  	values.  This is the same as providing '^$' as the `value-pattern`
>  	in `--replace-all`.
>  
> +--comment <value>::
> +	Append a comment to new or modified lines. A '#' character will be
> +	unconditionally prepended to the value. The value must not contain
> +	linefeed characters (no multi-line comments are permitted).

About the "unconditional" part I won't repeat.  As to the formatting
style, the recent trend is to write placeholder emphasized and
inside angle bracket, e.g.

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

This illustration is only to show the formatting, and not what the
description says.

>  [section]
> -	penguin = very blue
>  	Movie = BadPhysics
>  	UPPERCASE = true
> -	penguin = kingpin
> +	penguin = gentoo #Pygoscelis papua
> +	disposition = peckish #find fish
> +	foo = bar ## abc

The first one, "very blue", will be gone, which I found a bit
surprising, but it is OK.  This is because "--replace-all" used
first will eat all the "penguin" and leaves only a single one.

>  [Sections]
>  	WhatEver = Second
>  EOF
> +test_expect_success 'append comments' '
> +	git config --replace-all --comment="Pygoscelis papua" section.penguin gentoo &&
> +	git config --comment="find fish" section.disposition peckish &&
> +	git config --comment="# abc" section.foo bar &&
> +	test_cmp expect .git/config
> +'

Notice that none of these would produce the most natural (at least
in the context of Git command set)

	variable = value # comment

You would have to say --comment=" comment" which is rather
unfortunate.

-------- >8 --------------- >8 --------------- >8 --------------- >8 --------
From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 15 Mar 2024 12:43:58 -0700
Subject: [PATCH] config: fix --comment formatting

When git adds comments itself (like "rebase -i" todo list and
"commit -e" log message editor), it always gives a comment
introducer "#" followed by a Space before the message, except for
the recently introduced "git config --comment", where the users are
forced to say " this is my comment" if they want to add their
comment in this usual format; otherwise their comment string will
end up without a space after the "#".

Make it more ergonomic, while keeping it possible to also use this
unusual style, by massaging the comment string at the UI layer with
a set of simple rules:

 * If the given comment string begins with '#', it is passed intact.
 * Otherwise, "# " is prefixed.
 * A string with LF in it cannot be used as a comment string.

Right now there is only one "front-end" that accepts end-user
comment string and calls the underlying machinery to add or modify
configuration file with comments, but to make sure that the future
callers perform similar massaging as they see fit, add a sanity
check logic in git_config_set_multivar_in_file_gently(), which is
the single choke point in the codepaths that consumes the comment
string.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

Even with this fix, the code unconditionally places a single Space
character after the value before the '#' comment introducer.  If we
wanted to allow it to be replaced with other kind of whitespaces,
we could tweak the massaging logic further.

The updated tweaking rule may become

 * If the given comment string begins with one or more whitespace
   characters followed by '#', it is passed intact.
 * If the given comment string begins with '#', then a SP is
   prefixed.
 * Otherwise, " # " (Space, '#', Space) is prefixed.
 * A string with LF in it cannot be used as a comment string.

And the sanity checking rule may become

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

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

Finally the code to add the massaged comment will simply become:

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

I personally think that is over-engineered, so this commit stops
short of doing any of that.  If we were to do so, the tweaking logic
must be encapsulated into a helper function, so that the second and
subsequent "front-end" can reuse it.

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

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e608d5ffef..af374ee2e0 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -9,9 +9,9 @@ git-config - Get and set repository or global options
 SYNOPSIS
 --------
 [verse]
-'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]]
-'git config' [<file-option>] [--type=<type>] [--comment=<value>] --add <name> <value>
-'git config' [<file-option>] [--type=<type>] [--comment=<value>] [--fixed-value] --replace-all <name> <value> [<value-pattern>]
+'git config' [<file-option>] [--type=<type>] [--comment=<message>] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] <name> [<value> [<value-pattern>]]
+'git config' [<file-option>] [--type=<type>] [--comment=<message>] --add <name> <value>
+'git config' [<file-option>] [--type=<type>] [--comment=<message>] [--fixed-value] --replace-all <name> <value> [<value-pattern>]
 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get <name> [<value-pattern>]
 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all <name> [<value-pattern>]
 'git config' [<file-option>] [--type=<type>] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp <name-regex> [<value-pattern>]
@@ -87,10 +87,11 @@ OPTIONS
 	values.  This is the same as providing '^$' as the `value-pattern`
 	in `--replace-all`.
 
---comment <value>::
-	Append a comment to new or modified lines. A '#' character will be
-	unconditionally prepended to the value. The value must not contain
-	linefeed characters (no multi-line comments are permitted).
+--comment <message>::
+	Append a comment at the end of new or modified lines.
+	Unless _<message>_ begins with "#", a string "# " (hash
+	followed by a space) is prepended to it. The _<message>_ must not
+	contain linefeed characters (no multi-line comments are permitted).
 
 --get::
 	Get the value for a given key (optionally filtered by a regex
diff --git a/builtin/config.c b/builtin/config.c
index c54e9941a5..e859a659f4 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -174,7 +174,7 @@ static struct option builtin_config_options[] = {
 	OPT_BOOL(0, "show-origin", &show_origin, N_("show origin of config (file, standard input, blob, command line)")),
 	OPT_BOOL(0, "show-scope", &show_scope, N_("show scope of config (worktree, local, global, system, command)")),
 	OPT_STRING(0, "default", &default_value, N_("value"), N_("with --get, use default value when missing entry")),
-	OPT_STRING(0, "comment", &comment, N_("value"), N_("human-readable comment string (# will be prepended automatically)")),
+	OPT_STRING(0, "comment", &comment, N_("value"), N_("human-readable comment string (# will be prepended as needed)")),
 	OPT_END(),
 };
 
@@ -800,9 +800,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 	}
 
 	if (comment &&
-		!(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) {
-			error(_("--comment is only applicable to add/set/replace operations"));
-			usage_builtin_config();
+	    !(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) {
+		error(_("--comment is only applicable to add/set/replace operations"));
+		usage_builtin_config();
 	}
 
 	/* check usage of --fixed-value */
@@ -841,6 +841,13 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		flags |= CONFIG_FLAGS_FIXED_VALUE;
 	}
 
+	if (comment) {
+		if (strchr(comment, '\n'))
+			die(_("no multi-line comment allowed: '%s'"), comment);
+		if (comment[0] != '#')
+			comment = xstrfmt("# %s", comment);
+	}
+
 	if (actions & PAGING_ACTIONS)
 		setup_auto_pager("config", 1);
 
diff --git a/config.c b/config.c
index 2f3f6d123c..15019cb9a5 100644
--- a/config.c
+++ b/config.c
@@ -3043,12 +3043,9 @@ static ssize_t write_pair(int fd, const char *key, const char *value,
 			break;
 		}
 
-	if (comment) {
-		if (strchr(comment, '\n'))
-			die(_("multi-line comments are not permitted: '%s'"), comment);
-		else
-			strbuf_addf(&sb, "%s #%s\n", quote, comment);
-	} else
+	if (comment)
+		strbuf_addf(&sb, "%s %s\n", quote, comment);
+	else
 		strbuf_addf(&sb, "%s\n", quote);
 
 	ret = write_in_full(fd, sb.buf, sb.len);
@@ -3214,6 +3211,17 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 	size_t contents_sz;
 	struct config_store_data store = CONFIG_STORE_INIT;
 
+	if (comment) {
+		/*
+		 * The front-end must have massaged the comment string
+		 * properly before calling us.
+		 */
+		if (strchr(comment, '\n'))
+			BUG("multi-line comments are not permitted: '%s'", comment);
+		if (comment[0] != '#')
+			BUG("comment should begin with '#': '%s'", comment);
+	}
+
 	/* parse-key returns negative; flip the sign to feed exit(3) */
 	ret = 0 - git_config_parse_key(key, &store.key, &store.baselen);
 	if (ret)
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index ac7b02e6b0..d5dfb45877 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -71,16 +71,17 @@ cat > expect << EOF
 [section]
 	Movie = BadPhysics
 	UPPERCASE = true
-	penguin = gentoo #Pygoscelis papua
-	disposition = peckish #find fish
-	foo = bar ## abc
+	penguin = gentoo # Pygoscelis papua
+	disposition = peckish # find fish
+	foo = bar #abc
 [Sections]
 	WhatEver = Second
 EOF
+
 test_expect_success 'append comments' '
 	git config --replace-all --comment="Pygoscelis papua" section.penguin gentoo &&
 	git config --comment="find fish" section.disposition peckish &&
-	git config --comment="# abc" section.foo bar &&
+	git config --comment="#abc" section.foo bar &&
 	test_cmp expect .git/config
 '
 
-- 
2.44.0-248-g4f9b731bde

[PATCH 3/1] config: allow tweaking whitespace between value and comment

[Junio C Hamano]

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * 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(-)

diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index af374ee2e0..e4f2e07926 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -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
diff --git a/builtin/config.c b/builtin/config.c
index e859a659f4..0015620dde 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -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);
diff --git a/config.c b/config.c
index 15019cb9a5..f1d4263a68 100644
--- a/config.c
+++ b/config.c
@@ -3044,7 +3044,7 @@ static ssize_t write_pair(int fd, const char *key, const char *value,
 		}
 
 	if (comment)
-		strbuf_addf(&sb, "%s %s\n", quote, comment);
+		strbuf_addf(&sb, "%s%s\n", quote, comment);
 	else
 		strbuf_addf(&sb, "%s\n", quote);
 
@@ -3172,6 +3172,62 @@ void git_config_set(const char *key, const char *value)
 	trace2_cmd_set_config(key, value);
 }
 
+/*
+ * The ownership rule is that the caller will own the string
+ * if it receives a piece of memory different from what it passed
+ * as the parameter.
+ */
+const char *git_config_prepare_comment_string(const char *comment)
+{
+	size_t leading_blanks;
+
+	if (!comment)
+		return NULL;
+
+	if (strchr(comment, '\n'))
+		die(_("no multi-line comment allowed: '%s'"), comment);
+
+	/*
+	 * If it begins with one or more leading whitespace characters
+	 * followed by '#", the comment string is used as-is.
+	 *
+	 * If it begins with '#', a SP is inserted between the comment
+	 * and the value the comment is about.
+	 *
+	 * Otherwise, the value is followed by a SP followed by '#'
+	 * followed by SP and then the comment string comes.
+	 */
+
+	leading_blanks = strspn(comment, " \t");
+	if (leading_blanks && comment[leading_blanks] == '#')
+		; /* use it as-is */
+	else if (comment[0] == '#')
+		comment = xstrfmt(" %s", comment);
+	else
+		comment = xstrfmt(" # %s", comment);
+
+	return comment;
+}
+
+static void validate_comment_string(const char *comment)
+{
+	size_t leading_blanks;
+
+	if (!comment)
+		return;
+	/*
+	 * 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);
+
+	leading_blanks = strspn(comment, " \t");
+	if (!leading_blanks || comment[leading_blanks] != '#')
+		BUG("comment must begin with one or more SP followed by '#': '%s'",
+		    comment);
+}
+
 /*
  * If value==NULL, unset in (remove from) config,
  * if value_pattern!=NULL, disregard key/value pairs where value does not match.
@@ -3211,16 +3267,7 @@ 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);
-	}
+	validate_comment_string(comment);
 
 	/* parse-key returns negative; flip the sign to feed exit(3) */
 	ret = 0 - git_config_parse_key(key, &store.key, &store.baselen);
diff --git a/config.h b/config.h
index a85a827169..f4966e3749 100644
--- a/config.h
+++ b/config.h
@@ -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:
  *
diff --git a/t/t1300-config.sh b/t/t1300-config.sh
index d5dfb45877..cc050b3c20 100755
--- a/t/t1300-config.sh
+++ b/t/t1300-config.sh
@@ -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

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

[Eric Sunshine]

On Fri, Mar 15, 2024 at 6:19 PM Junio C Hamano <gitster@pobox.com> wrote:
> ---comment <value>::
> -       Append a comment to new or modified lines. A '#' character will be
> -       unconditionally prepended to the value. The value must not contain
> -       linefeed characters (no multi-line comments are permitted).
> +--comment <message>::
> +       Append a comment at the end of new or modified lines.
> +       Unless _<message>_ begins with "#", a string "# " (hash
> +       followed by a space) is prepended to it. The _<message>_ must not
> +       contain linefeed characters (no multi-line comments are permitted).

In an earlier review, you wrote about the potential difficulties and
issues surrounding comments other than the simple "inline" comments
tackled by this patch. Do we foresee a future in which git-config may
be extended to handle automation of comments other than inline? If so,
do we really want to lock ourselves into having the generic option
`--comment=<message>` be restricted to only inline comments? Or would
it be better to plan for the future by instead having some sort of
annotation which indicates that we are requesting (or dealing with)
only inline comments, such as `--inline-comment=<message>` or
`--comment=inline:<message>`?

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

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> tackled by this patch. Do we foresee a future in which git-config may
> be extended to handle automation of comments other than inline? If so,
> do we really want to lock ourselves into having the generic option
> `--comment=<message>` be restricted to only inline comments? Or would
> it be better to plan for the future by instead having some sort of
> annotation which indicates that we are requesting (or dealing with)
> only inline comments, such as `--inline-comment=<message>` or
> `--comment=inline:<message>`?

I do not see a need for anything, while we fail a request with a
multi-line message and say "don't feed me a multi-line message".

When we start supporting multi-line comment (which I do not think is
a good idea at all, by the way), the code can switch between

	[vari]
		able = value # single line comment
		# a comment with
		# more than one line
		able = value

depending on what is in <message>, so "inline:" prefix does not even
have to exist, I would imagine.  Of course you could even go fancier
and allow something like this with unnecessary tailing LF ...

	$ git config --comment='another single line comment
	'

... as a signal to turn a single line commet on a separate line.

	[vari]
		able = value # single line comment
		# a comment with
		# more than one line
		able = value
		# another single value comment
		able = value

There will most likely need where e.g., above or below, around the
"var = val" line to place the comment as well, so I do not see much
value in investing more brain cycles on what the "--comment" option
should look like, while we only support single-liners and explicitly
reject multi-line messages.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> There will most likely need where e.g., above or below, around the

"need" -> "need for adding a new option to specify".

In sort, we would need to have more than just "use this message"
anyway, and cramming everything into the single "--comment" option,
even if it were feasible, would not be a good idea.

> "var = val" line to place the comment as well, so I do not see much
> value in investing more brain cycles on what the "--comment" option
> should look like, while we only support single-liners and explicitly
> reject multi-line messages.

Re: [PATCH 3/1] config: allow tweaking whitespace between value and comment

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Extending the previous step, this allows ...

If I am not mistaken, this topic, originating at

https://lore.kernel.org/git/pull.1681.v3.git.1710280020508.gitgitgadget@gmail.com/

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

Thanks.

Re: [PATCH 3/1] config: allow tweaking whitespace between value and comment

[Ralph Seichter]

* Junio C. Hamano:

> Can you unblock [this topic] to let us move forward?

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

Re: [PATCH 3/1] config: allow tweaking whitespace between value and comment

[Junio C Hamano]

Ralph Seichter <github@seichter.de> writes:

>> Can you unblock [this topic] to let us move forward?
>
> 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.

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.

Re: [PATCH 3/1] config: allow tweaking whitespace between value and comment

[Ralph Seichter]

* Junio C. Hamano:

> The easiest would have been for you to say "Yup, the two additional
> patches queued on top of mine seem to make it better. [...]

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

>> I thought we already discussed this and unconditional "#comment" has
>> already been declared a non starter.

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

Re: [PATCH 3/1] config: allow tweaking whitespace between value and comment

[Dragan Simic]

Hello Ralph,

On 2024-03-27 01:27, Ralph Seichter wrote:
> * Junio C. Hamano:
> 
>> The easiest would have been for you to say "Yup, the two additional
>> patches queued on top of mine seem to make it better. [...]
> 
> Would it? Well, you wrote the following two weeks ago, among other
> things:
> 
>>> I thought we already discussed this and unconditional "#comment" has
>>> already been declared a non starter.
> 
> 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.

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.

Re: [PATCH 3/1] config: allow tweaking whitespace between value and comment

[Junio C Hamano]

Ralph Seichter <github@seichter.de> writes:

>>> I thought we already discussed this and unconditional "#comment" has
>>> already been declared a non starter.
>
> 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.

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.