{"thread":{"id":"61043","subject":"[PATCH] Allow git-config to append a comment","startedAt":"2024-03-04T06:00:21Z","lastAt":"2024-03-27T17:27:29Z","messageCount":47,"participants":["Ralph Seichter via GitGitGadget","Junio C Hamano","Ralph Seichter","rsbecker@nexbridge.com","Dragan Simic","Chris Torek","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"489832","messageId":"pull.1681.git.1709532018372.gitgitgadget@gmail.com","threadId":"61043","inReplyTo":null,"subject":"[PATCH] Allow git-config to append a comment","fromName":"Ralph Seichter via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-04T06:00:17Z","receivedAt":"2024-03-04T06:00:21Z","isPatch":true,"sender":{"key":"github@seichter.de","avatar":"https://avatars.githubusercontent.com/u/30873939?v=4"},"body":"From: Ralph Seichter \n\nIntroduce the ability to append comments to modifications\nmade using git-config. Example usage:\n\n git config --comment \"I changed this. --A. Script\" \\\n --add safe.directory /home/alice/somerepo.git\n\nThe implementation ensures that a # character is always\nprepended to the provided comment string.\n\nSigned-off-by: Ralph Seichter \n---\n Allow git-config to append a comment\n \n Introduce the ability to append comments to modifications made using\n git-config. Example usage:\n \n git config --comment \"I changed this. --A. Script\" \\\n --add safe.directory /home/alice/somerepo.git\n \n \n The implementation ensures that a # character is always prepended to the\n provided comment string.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1681%2Frseichter%2Fissue-1680-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1681/rseichter/issue-1680-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1681\n\n Documentation/git-config.txt | 10 +++++++---\n builtin/config.c | 21 ++++++++++++++-------\n builtin/gc.c | 4 ++--\n builtin/submodule--helper.c | 2 +-\n builtin/worktree.c | 4 ++--\n config.c | 21 +++++++++++++--------\n config.h | 4 ++--\n sequencer.c | 28 ++++++++++++++--------------\n submodule-config.c | 2 +-\n submodule.c | 2 +-\n t/t1300-config.sh | 9 +++++++--\n worktree.c | 4 ++--\n 12 files changed, 66 insertions(+), 45 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex dff39093b5e..ee8cd251b24 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -9,9 +9,9 @@ git-config - Get and set repository or global options\n SYNOPSIS\n --------\n [verse]\n-'git config' [] [--type=] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] [ []]\n-'git config' [] [--type=] --add \n-'git config' [] [--type=] [--fixed-value] --replace-all []\n+'git config' [] [--type=] [--comment=] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] [ []]\n+'git config' [] [--type=] [--comment=] --add \n+'git config' [] [--type=] [--comment=] [--fixed-value] --replace-all []\n 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get []\n 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all []\n 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp []\n@@ -87,6 +87,10 @@ OPTIONS\n \tvalues. This is the same as providing '^$' as the `value-pattern`\n \tin `--replace-all`.\n \n+--comment::\n+\tAppend a comment to new or modified lines. A '#' character\n+\twill be automatically prepended to the value.\n+\n --get::\n \tGet the value for a given key (optionally filtered by a regex\n \tmatching the value). Returns error code 1 if the key was not\ndiff --git a/builtin/config.c b/builtin/config.c\nindex b55bfae7d66..2aab3c0baf3 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -44,6 +44,7 @@ static struct config_options config_options;\n static int show_origin;\n static int show_scope;\n static int fixed_value;\n+static const char *comment;\n \n #define ACTION_GET (1<<0)\n #define ACTION_GET_ALL (1<<1)\n@@ -173,6 +174,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BOOL(0, \"show-origin\", &show_origin, N_(\"show origin of config (file, standard input, blob, command line)\")),\n \tOPT_BOOL(0, \"show-scope\", &show_scope, N_(\"show scope of config (worktree, local, global, system, command)\")),\n \tOPT_STRING(0, \"default\", &default_value, N_(\"value\"), N_(\"with --get, use default value when missing entry\")),\n+\tOPT_STRING(0, \"comment\", &comment, N_(\"value\"), N_(\"human-readable comment string (# will be prepended automatically)\")),\n \tOPT_END(),\n };\n \n@@ -797,6 +799,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tusage_builtin_config();\n \t}\n \n+\tif (comment && !(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) {\n+\t\terror(_(\"--comment is only applicable to add/set/replace operations\"));\n+\t\tusage_builtin_config();\n+\t}\n+\n \t/* check usage of --fixed-value */\n \tif (fixed_value) {\n \t\tint allowed_usage = 0;\n@@ -880,7 +887,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1], &default_kvi);\n-\t\tret = git_config_set_in_file_gently(given_config_source.file, argv[0], value);\n+\t\tret = git_config_set_in_file_gently(given_config_source.file, argv[0], comment, value);\n \t\tif (ret == CONFIG_NOTHING_SET)\n \t\t\terror(_(\"cannot overwrite multiple values with a single value\\n\"\n \t\t\t\" Use a regexp, --add or --replace-all to change %s.\"), argv[0]);\n@@ -891,7 +898,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tvalue = normalize_value(argv[0], argv[1], &default_kvi);\n \t\tret = git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], value, argv[2],\n-\t\t\t\t\t\t\t flags);\n+\t\t\t\t\t\t\t comment, flags);\n \t}\n \telse if (actions == ACTION_ADD) {\n \t\tcheck_write();\n@@ -900,7 +907,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tret = git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], value,\n \t\t\t\t\t\t\t CONFIG_REGEX_NONE,\n-\t\t\t\t\t\t\t flags);\n+\t\t\t\t\t\t\t comment, flags);\n \t}\n \telse if (actions == ACTION_REPLACE_ALL) {\n \t\tcheck_write();\n@@ -908,7 +915,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tvalue = normalize_value(argv[0], argv[1], &default_kvi);\n \t\tret = git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], value, argv[2],\n-\t\t\t\t\t\t\t flags | CONFIG_FLAGS_MULTI_REPLACE);\n+\t\t\t\t\t\t\t comment, flags | CONFIG_FLAGS_MULTI_REPLACE);\n \t}\n \telse if (actions == ACTION_GET) {\n \t\tcheck_argc(argc, 1, 2);\n@@ -936,17 +943,17 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tif (argc == 2)\n \t\t\treturn git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t\t argv[0], NULL, argv[1],\n-\t\t\t\t\t\t\t\t flags);\n+\t\t\t\t\t\t\t\t NULL, flags);\n \t\telse\n \t\t\treturn git_config_set_in_file_gently(given_config_source.file,\n-\t\t\t\t\t\t\t argv[0], NULL);\n+\t\t\t\t\t\t\t argv[0], NULL, NULL);\n \t}\n \telse if (actions == ACTION_UNSET_ALL) {\n \t\tcheck_write();\n \t\tcheck_argc(argc, 1, 2);\n \t\treturn git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], NULL, argv[1],\n-\t\t\t\t\t\t\t flags | CONFIG_FLAGS_MULTI_REPLACE);\n+\t\t\t\t\t\t\t NULL, flags | CONFIG_FLAGS_MULTI_REPLACE);\n \t}\n \telse if (actions == ACTION_RENAME_SECTION) {\n \t\tcheck_write();\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex cb80ced6cb5..342907f7bdb 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1553,7 +1553,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"$HOME not set\"));\n \t\trc = git_config_set_multivar_in_file_gently(\n \t\t\tconfig_file, \"maintenance.repo\", maintpath,\n-\t\t\tCONFIG_REGEX_NONE, 0);\n+\t\t\tCONFIG_REGEX_NONE, NULL, 0);\n \t\tfree(global_config_file);\n \n \t\tif (rc)\n@@ -1620,7 +1620,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \t\tif (!config_file)\n \t\t\tdie(_(\"$HOME not set\"));\n \t\trc = git_config_set_multivar_in_file_gently(\n-\t\t\tconfig_file, key, NULL, maintpath,\n+\t\t\tconfig_file, key, NULL, maintpath, NULL,\n \t\t\tCONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);\n \t\tfree(global_config_file);\n \ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex fda50f2af1e..e4e18adb575 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1283,7 +1283,7 @@ static void sync_submodule(const char *path, const char *prefix,\n \tsubmodule_to_gitdir(&sb, path);\n \tstrbuf_addstr(&sb, \"/config\");\n \n-\tif (git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url))\n+\tif (git_config_set_in_file_gently(sb.buf, remote_key, NULL, sub_origin_url))\n \t\tdie(_(\"failed to update remote for submodule '%s'\"),\n \t\t path);\n \ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 9c76b62b02d..a20cc8820e5 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -365,12 +365,12 @@ static void copy_filtered_worktree_config(const char *worktree_git_dir)\n \t\tif (!git_configset_get_bool(&cs, \"core.bare\", &bare) &&\n \t\t\tbare &&\n \t\t\tgit_config_set_multivar_in_file_gently(\n-\t\t\t\tto_file, \"core.bare\", NULL, \"true\", 0))\n+\t\t\t\tto_file, \"core.bare\", NULL, \"true\", NULL, 0))\n \t\t\terror(_(\"failed to unset '%s' in '%s'\"),\n \t\t\t\t\"core.bare\", to_file);\n \t\tif (!git_configset_get(&cs, \"core.worktree\") &&\n \t\t\tgit_config_set_in_file_gently(to_file,\n-\t\t\t\t\t\t\t\"core.worktree\", NULL))\n+\t\t\t\t\t\t\t\"core.worktree\", NULL, NULL))\n \t\t\terror(_(\"failed to unset '%s' in '%s'\"),\n \t\t\t\t\"core.worktree\", to_file);\n \ndiff --git a/config.c b/config.c\nindex 3cfeb3d8bd9..a22594eabd9 100644\n--- a/config.c\n+++ b/config.c\n@@ -3001,6 +3001,7 @@ static ssize_t write_section(int fd, const char *key,\n }\n \n static ssize_t write_pair(int fd, const char *key, const char *value,\n+\t\t\t const char *comment,\n \t\t\t const struct config_store_data *store)\n {\n \tint i;\n@@ -3041,7 +3042,10 @@ static ssize_t write_pair(int fd, const char *key, const char *value,\n \t\t\tstrbuf_addch(&sb, value[i]);\n \t\t\tbreak;\n \t\t}\n-\tstrbuf_addf(&sb, \"%s\\n\", quote);\n+\tif (comment)\n+\t\tstrbuf_addf(&sb, \"%s #%s\\n\", quote, comment);\n+\telse\n+\t\tstrbuf_addf(&sb, \"%s\\n\", quote);\n \n \tret = write_in_full(fd, sb.buf, sb.len);\n \tstrbuf_release(&sb);\n@@ -3130,9 +3134,9 @@ static void maybe_remove_section(struct config_store_data *store,\n }\n \n int git_config_set_in_file_gently(const char *config_filename,\n-\t\t\t\t const char *key, const char *value)\n+\t\t\t\t const char *key, const char *comment, const char *value)\n {\n-\treturn git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, 0);\n+\treturn git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, comment, 0);\n }\n \n void git_config_set_in_file(const char *config_filename,\n@@ -3153,7 +3157,7 @@ int repo_config_set_worktree_gently(struct repository *r,\n \tif (r->repository_format_worktree_config) {\n \t\tchar *file = repo_git_path(r, \"config.worktree\");\n \t\tint ret = git_config_set_multivar_in_file_gently(\n-\t\t\t\t\tfile, key, value, NULL, 0);\n+\t\t\t\t\tfile, key, value, NULL, NULL, 0);\n \t\tfree(file);\n \t\treturn ret;\n \t}\n@@ -3195,6 +3199,7 @@ void git_config_set(const char *key, const char *value)\n int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\t\t\t const char *key, const char *value,\n \t\t\t\t\t const char *value_pattern,\n+\t\t\t\t\t const char *comment,\n \t\t\t\t\t unsigned flags)\n {\n \tint fd = -1, in_fd = -1;\n@@ -3245,7 +3250,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\tfree(store.key);\n \t\tstore.key = xstrdup(key);\n \t\tif (write_section(fd, key, &store) < 0 ||\n-\t\t write_pair(fd, key, value, &store) < 0)\n+\t\t write_pair(fd, key, value, comment, &store) < 0)\n \t\t\tgoto write_err_out;\n \t} else {\n \t\tstruct stat st;\n@@ -3399,7 +3404,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\t\tif (write_section(fd, key, &store) < 0)\n \t\t\t\t\tgoto write_err_out;\n \t\t\t}\n-\t\t\tif (write_pair(fd, key, value, &store) < 0)\n+\t\t\tif (write_pair(fd, key, value, comment, &store) < 0)\n \t\t\t\tgoto write_err_out;\n \t\t}\n \n@@ -3444,7 +3449,7 @@ void git_config_set_multivar_in_file(const char *config_filename,\n \t\t\t\t const char *value_pattern, unsigned flags)\n {\n \tif (!git_config_set_multivar_in_file_gently(config_filename, key, value,\n-\t\t\t\t\t\t value_pattern, flags))\n+\t\t\t\t\t\t value_pattern, NULL, flags))\n \t\treturn;\n \tif (value)\n \t\tdie(_(\"could not set '%s' to '%s'\"), key, value);\n@@ -3467,7 +3472,7 @@ int repo_config_set_multivar_gently(struct repository *r, const char *key,\n \tint res = git_config_set_multivar_in_file_gently(file,\n \t\t\t\t\t\t\t key, value,\n \t\t\t\t\t\t\t value_pattern,\n-\t\t\t\t\t\t\t flags);\n+\t\t\t\t\t\t\t NULL, flags);\n \tfree(file);\n \treturn res;\n }\ndiff --git a/config.h b/config.h\nindex 5dba984f770..a85a8271696 100644\n--- a/config.h\n+++ b/config.h\n@@ -290,7 +290,7 @@ int git_config_pathname(const char **, const char *, const char *);\n \n int git_config_expiry_date(timestamp_t *, const char *, const char *);\n int git_config_color(char *, const char *, const char *);\n-int git_config_set_in_file_gently(const char *, const char *, const char *);\n+int git_config_set_in_file_gently(const char *, const char *, const char *, const char *);\n \n /**\n * write config values to a specific config file, takes a key/value pair as\n@@ -336,7 +336,7 @@ int git_config_parse_key(const char *, char **, size_t *);\n int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);\n void git_config_set_multivar(const char *, const char *, const char *, unsigned);\n int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);\n-int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);\n+int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, const char *, unsigned);\n \n /**\n * takes four parameters:\ndiff --git a/sequencer.c b/sequencer.c\nindex f49a871ac06..4c91ca5a844 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3460,54 +3460,54 @@ static int save_opts(struct replay_opts *opts)\n \n \tif (opts->no_commit)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.no-commit\", \"true\");\n+\t\t\t\t\t\"options.no-commit\", NULL, \"true\");\n \tif (opts->edit >= 0)\n-\t\tres |= git_config_set_in_file_gently(opts_file, \"options.edit\",\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.edit\", NULL,\n \t\t\t\t\t\t opts->edit ? \"true\" : \"false\");\n \tif (opts->allow_empty)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.allow-empty\", \"true\");\n+\t\t\t\t\t\"options.allow-empty\", NULL, \"true\");\n \tif (opts->allow_empty_message)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\"options.allow-empty-message\", \"true\");\n+\t\t\t\t\"options.allow-empty-message\", NULL, \"true\");\n \tif (opts->keep_redundant_commits)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\"options.keep-redundant-commits\", \"true\");\n+\t\t\t\t\"options.keep-redundant-commits\", NULL, \"true\");\n \tif (opts->signoff)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.signoff\", \"true\");\n+\t\t\t\t\t\"options.signoff\", NULL, \"true\");\n \tif (opts->record_origin)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.record-origin\", \"true\");\n+\t\t\t\t\t\"options.record-origin\", NULL, \"true\");\n \tif (opts->allow_ff)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.allow-ff\", \"true\");\n+\t\t\t\t\t\"options.allow-ff\", NULL, \"true\");\n \tif (opts->mainline) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstrbuf_addf(&buf, \"%d\", opts->mainline);\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.mainline\", buf.buf);\n+\t\t\t\t\t\"options.mainline\", NULL, buf.buf);\n \t\tstrbuf_release(&buf);\n \t}\n \tif (opts->strategy)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.strategy\", opts->strategy);\n+\t\t\t\t\t\"options.strategy\", NULL, opts->strategy);\n \tif (opts->gpg_sign)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.gpg-sign\", opts->gpg_sign);\n+\t\t\t\t\t\"options.gpg-sign\", NULL, opts->gpg_sign);\n \tfor (size_t i = 0; i < opts->xopts.nr; i++)\n \t\tres |= git_config_set_multivar_in_file_gently(opts_file,\n \t\t\t\t\"options.strategy-option\",\n-\t\t\t\topts->xopts.v[i], \"^$\", 0);\n+\t\t\t\topts->xopts.v[i], \"^$\", NULL, 0);\n \tif (opts->allow_rerere_auto)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\"options.allow-rerere-auto\",\n+\t\t\t\t\"options.allow-rerere-auto\", NULL,\n \t\t\t\topts->allow_rerere_auto == RERERE_AUTOUPDATE ?\n \t\t\t\t\"true\" : \"false\");\n \n \tif (opts->explicit_cleanup)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\"options.default-msg-cleanup\",\n+\t\t\t\t\"options.default-msg-cleanup\", NULL,\n \t\t\t\tdescribe_cleanup_mode(opts->default_msg_cleanup));\n \treturn res;\n }\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 54130f6a385..11428b4adad 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -978,7 +978,7 @@ int config_set_in_gitmodules_file_gently(const char *key, const char *value)\n {\n \tint ret;\n \n-\tret = git_config_set_in_file_gently(GITMODULES_FILE, key, value);\n+\tret = git_config_set_in_file_gently(GITMODULES_FILE, key, NULL, value);\n \tif (ret < 0)\n \t\t/* Maybe the user already did that, don't error out here */\n \t\twarning(_(\"Could not update .gitmodules entry %s\"), key);\ndiff --git a/submodule.c b/submodule.c\nindex 213da79f661..86630932d09 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -2052,7 +2052,7 @@ void submodule_unset_core_worktree(const struct submodule *sub)\n \tsubmodule_name_to_gitdir(&config_path, the_repository, sub->name);\n \tstrbuf_addstr(&config_path, \"/config\");\n \n-\tif (git_config_set_in_file_gently(config_path.buf, \"core.worktree\", NULL))\n+\tif (git_config_set_in_file_gently(config_path.buf, \"core.worktree\", NULL, NULL))\n \t\twarning(_(\"Could not unset core.worktree setting in submodule '%s'\"),\n \t\t\t sub->path);\n \ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 31c38786870..daddaedd23c 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -69,13 +69,18 @@ test_expect_success 'replace with non-match (actually matching)' '\n \n cat > expect << EOF\n [section]\n-\tpenguin = very blue\n \tMovie = BadPhysics\n \tUPPERCASE = true\n-\tpenguin = kingpin\n+\tpenguin = gentoo #Pygoscelis papua\n+\tdisposition = peckish #find fish\n [Sections]\n \tWhatEver = Second\n EOF\n+test_expect_success 'append comments' '\n+\tgit config --replace-all --comment=\"Pygoscelis papua\" section.penguin gentoo &&\n+\tgit config --comment=\"find fish\" section.disposition peckish &&\n+\ttest_cmp expect .git/config\n+'\n \n test_expect_success 'non-match result' 'test_cmp expect .git/config'\n \ndiff --git a/worktree.c b/worktree.c\nindex b02a05a74a3..cf5eea8c931 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -807,9 +807,9 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,\n static int move_config_setting(const char *key, const char *value,\n \t\t\t const char *from_file, const char *to_file)\n {\n-\tif (git_config_set_in_file_gently(to_file, key, value))\n+\tif (git_config_set_in_file_gently(to_file, key, NULL, value))\n \t\treturn error(_(\"unable to set %s in '%s'\"), key, to_file);\n-\tif (git_config_set_in_file_gently(from_file, key, NULL))\n+\tif (git_config_set_in_file_gently(from_file, key, NULL, NULL))\n \t\treturn error(_(\"unable to unset %s in '%s'\"), key, from_file);\n \treturn 0;\n }\n\nbase-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d\n-- \ngitgitgadget\n"},{"id":"490090","messageId":"xmqqttljtkn4.fsf@gitster.g","threadId":"61043","inReplyTo":"pull.1681.git.1709532018372.gitgitgadget@gmail.com","subject":"Re: [PATCH] Allow git-config to append a comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-06T16:01:03Z","receivedAt":"2024-03-06T16:01:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ralph Seichter via GitGitGadget\" writes:\n\n> From: Ralph Seichter \n> Subject: Re: [PATCH] Allow git-config to append a comment\n\nThanks for a patch.\n\nMake sure your title will mix well in \"git shortlog --no-merges\"\noutput from recent commits by other contributors. See\nDocumentation/SubmittingPatches[[describe-changes]]. Perhaps\n\n Subject: [PATCH] config: add --comment option to add a comment\n\n> Introduce the ability to append comments to modifications\n> made using git-config. Example usage:\n>\n> git config --comment \"I changed this. --A. Script\" \\\n> --add safe.directory /home/alice/somerepo.git\n\nIf you are illustrating a sample input, please also explain what\noutput it produces. What do the resulting lines in the config file\nlook like after you run this command?\n\nI find the overall idea somewhat incomplete. Perhaps stepping back\na bit and thinking through the _whole_ use case of \"comment\" in the\nconfiguration files is in order, before thinking about an easier way\nto add one. Why are you adding \"# comment\" to your config file?\nWho reads these comments, with what tool, and for what purpose?\nWhat will they do after they learn something from what their former\nselves wrote as comments?\n\nLet's imagine we had this in our global configuration file in the\nbeginning:\n\n $ cat ~/.gitconfig\n\t...\n [safe]\n\tdirectory = /home/bob/otherrepo.git\n\t...\n\nand then we run this:\n\n $ git config --global --comment 'the reason why I added ~alice/\n is because ...' --add safe.directory /home/alice/somerepo.git\n $ git config --global --add safe.directory /home/charlie/somerepo.git\n\nwhich may modify the configuration file with the new entry with a\ncomment. The part about safe.directory might now read like so:\n\n [safe]\n\tdirectory = /home/bob/otherrepo.git\n # the reason why I added ~alice/\n # is because ...\n\tdirectory = /home/alice/somerepo.git\n\tdirectory = /home/charlie/somerepo.git\n\nNow how do we find out about this comment? \"git config -l\" would\nnot give us that. Are we expected to look at it in our editor or\npager?\n\n $ vi ~/.gitconfig\n $ less ~/.gitconfig\n\nWhy are we interested in looking at these comments in the first\nplace [*]?\n\n Side note: I do not ask \"why are we interested in leaving\n these comments in the file\"---it goes without saying that\n it is because we want to be able to later read them.\n\nPresumably, we are trying to remind ourselves why we added a\nparticular variable=value. By learning that, what are we going to\ndo next? Perhaps once the reason to mark /home/alice/somerepo.git\nas a safe.directory disappears, we would want to remove this entry?\n\nBut then, after running\n\n $ less ~/.gitconfig\n\nand then learning that we no longer need /home/alice/somerepo.git\nmarked as a safe.directory, how would we remove it from the\nconfiguration file, and what happens to the comment?\n\n $ git config --global --unset safe.directory /home/alice/somerepo.git\n\nmay remove the value, but it would not remove the comment, and\nyou'll be left with something like this:\n\n [safe]\n\tdirectory = /home/bob/otherrepo.git\n # the reason why I added ~alice/\n # is because ...\n\tdirectory = /home/charlie/somerepo.git\n\nThe remaining comment looks as if it talks about \"charlie\", but it\nsimply is stale.\n\nShould it have removed the comment, then? I actually do not think\nso. A comment before a particular variable definition may not be\nthe result of the use of this new \"config --comment\" option, but\nperhaps the user added it manually. Or even worse, the comment may\nnot even be about an individual entry. Imagine\n\n [section]\n\t# definitions for per-user stuff begin here\n\tvar = /home/alice\n\t# these two directories are actually charlie's\n\tvar = /home/bob\n\tvar = /home/charlie\n\n\t# definitions for per-project stuff\n\tvar = /project/a\n\tvar = /project/b\n\nCan we come up with a code that reliably decides when to remove the\nfirst comment we see above? The answer given by human might be\n\"when all the entries about /home/alice, /home/bob, and\n/home/charlie get removed\", but can we implement that in code\nwithout interpreting the natural language? I doubt it. We may even\nbe better off to keep the comment to remind ourselves where to add\nthe per-user stuff in the section when we hire a new user the next\ntime.\n\nWith the new \"--comment\" thing might be able to add comments without\nusing an editor, but the user would need to view the configuration\nfile with a pager or an editor bypassing Git to make use of what was\nrecorded in there. And it is likely that these comments will need\nto be edited or removed using an editor because \"git config\" command\nwould not be usable for maintaining them. For that matter, if you\nmade a typo in your \"--comment\" option from the command line, you\nwould have to resort to your editor to fix that typo.\n\nThe above is an illustration of what I want to see the author, who\ncomes up with this \"wouldn't it be wonderful to teach git-config to\nadd comment?\" idea, thinks through. The first patch might be to\njust add a comment when a variable=value is added, but we want to\nsee the vision of how the whole end-user experience should look\nlike, in which this first small piece will fit.\n\nIdentify the whole life cycle of the result of using the feature,\nwhat the end-user experience of using the result of using the\nfeature would look like, etc. In this case, \"the result of using\nthe feature\" is the comments that are initially placed next to a\nnewly defined variable-value pair, and at least to me, the end-user\nexperience to manage the life cycle of these comments is not all\nthat improved from the status quo, which is that you need your pager\nand editor to view them, edit them, typofix them, and remove them.\n\nI am somewhat negative on the idea to make the initial addition of\nthe comment vastly different from all other uses of the comment by\nintroducing this \"--comment\" option. If we have a vision, if not\ndesign and/or code, to help managing other parts of the lifecycle,\nnot just adding in the beginning, it might be a different story, but\nI cannot quite see it.\n\n> Documentation/git-config.txt | 10 +++++++---\n> builtin/config.c | 21 ++++++++++++++-------\n> builtin/gc.c | 4 ++--\n> builtin/submodule--helper.c | 2 +-\n> builtin/worktree.c | 4 ++--\n> config.c | 21 +++++++++++++--------\n> config.h | 4 ++--\n> sequencer.c | 28 ++++++++++++++--------------\n> submodule-config.c | 2 +-\n> submodule.c | 2 +-\n> t/t1300-config.sh | 9 +++++++--\n> worktree.c | 4 ++--\n> 12 files changed, 66 insertions(+), 45 deletions(-)\n\nAnd the amount of the change required for that tiny bit of\n\"improvement\" (if we can call it, which is dubious) does not seem\nworth it.\n\nThanks.\n"},{"id":"490104","messageId":"2560952c-4495-4a71-9497-aa40032e1d2b@seichter.de","threadId":"61043","inReplyTo":"xmqqttljtkn4.fsf@gitster.g","subject":"Re: [PATCH] Allow git-config to append a comment","fromName":"Ralph Seichter","fromEmail":"github@seichter.de","sentAt":"2024-03-06T17:24:56Z","receivedAt":"2024-03-06T17:25:08Z","isPatch":true,"sender":{"key":"github@seichter.de","avatar":"https://avatars.githubusercontent.com/u/30873939?v=4"},"body":"* Junio C Hamano:\n\n > Make sure your title will mix well in \"git shortlog --no-merges\"\n > output from recent commits by other contributors.\n\nThank you for your in-depth comment. This is the first time I have\never considered contributing to Git, so I have a lot to learn. My pull\nrequest [1] on GitGitGadget has been approved by Johannes Schindelin,\nby the way, and the PR is based on an issue Johannes created [2] after\na brief discussion him and I had on Discord [3]. I have updated the\nsubject line, as you suggested.\n\n [1] https://github.com/gitgitgadget/git/pull/1681\n [2] https://github.com/gitgitgadget/git/issues/1680\n [3] https://discord.com/channels/1042895022950994071/1213052410281467906\n\nI don't know if it is the polite thing to ask you to please refer to\nthe information I linked, or if I should duplicate the information\nhere? The short version is that I need to be able to distinguish\nbetween config entries made by automation (like scripts, or Ansible\nin my particular case) and those made by a human.\n\n >> git config --comment \"I changed this. --A. Script\" \\\n >> --add safe.directory /home/alice/somerepo.git\n >\n > If you are illustrating a sample input, please also explain what\n > output it produces. What do the resulting lines in the config file\n > look like after you run this command?\n\nThe result of running the above command looks as follows:\n\n [safe]\n\tdirectory = /home/alice/somerepo.git #I changed this. --A. Script\n\n > Why are you adding \"# comment\" to your config file? Who reads these\n > comments, with what tool, and for what purpose?\n\nI mentioned human-readable comments in the patch, and humans are\nindeed the intended audience. If a human finds changes made to a Git\nconfig file, and a comment states that the modification was e.g. made\nby automation, it adds beneficial information for said human. I can for\nexample create a comment with a URL pointing to a website providing\nadditional explanations.\n\n > Now how do we find out about this comment? \"git config -l\" would\n > not give us that. Are we expected to look at it in our editor or\n > pager?\n\nYes. I routinely use cat/vim to inspect/modify my Git config\nfiles. They are suitable for human consumption, after all. Also,\ncomments can already be manually added in creative ways, and are\nignored when Git reads config data. Comments being read only by\nhumans is pretty much their whole point, in my opinion.\n\n > Can we come up with a code that reliably decides when to remove the\n > first comment we see above?\n\nYour examples about difficulties removing comments hinge on there\nbeing multiline comments, as far as I can tell. My patch only supports\nsingle-line comments, and only as a suffix to newly added key-value\npairs. This is a deliberate design choice.\n\n > The above is an illustration of what I want to see the author, who\n > comes up with this \"wouldn't it be wonderful to teach git-config to\n > add comment?\" idea, thinks through. The first patch might be to\n > just add a comment when a variable=value is added, but we want to\n > see the vision of how the whole end-user experience should look\n > like, in which this first small piece will fit.\n\nI don't have a greater vision for comments. Their use is to provide\ninformation for humans, no more, no less. There is also no idea of a\nuser experience beyond pager/editor in my mind. The patch addresses\nspecific needs of mine, and Johannes suggested it as a new feature,\nand that's all the motivation behind it.\n\n > And the amount of the change required for that tiny bit of\n > \"improvement\" (if we can call it, which is dubious) does not seem\n > worth it.\n\nAs I mentioned in my PR, it also does not seem elegant to me to modify\nso many files. Alas, C does not offer expanding function signatures by\nadding parameters with default values, like Python does. Adding a new\nfunction like, perhaps, git_config_set_in_file_gently_with_comment()\ncould be a remedy?\n\n-Ralph\n"},{"id":"490171","messageId":"xmqqplw6nsuz.fsf@gitster.g","threadId":"61043","inReplyTo":"2560952c-4495-4a71-9497-aa40032e1d2b@seichter.de","subject":"Re: [PATCH] Allow git-config to append a comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-07T12:12:20Z","receivedAt":"2024-03-07T12:12:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ralph Seichter writes:\n\n>> If you are illustrating a sample input, please also explain what\n>> output it produces. What do the resulting lines in the config file\n>> look like after you run this command?\n>\n> The result of running the above command looks as follows:\n>\n> [safe]\n> \tdirectory = /home/alice/somerepo.git #I changed this. --A. Script\n\nThat would have been a crucial piece of information to have in the\nproposed log message, as limiting ourselves to a comment that is\ntucked after the same line as the value, things can become somewhat\nsimplified. We may not have to worry about deletion, even though the\npoint about \"we need to look at and typofix them with our viewers\nand editors\" still stands.\n\nBy the way, you may or may not have noticed it, but my example\ndeliberately had a multi-line comment:\n\n $ git config --global --comment 'the reason why I added ~alice/\n is because ...' --add safe.directory /home/alice/somerepo.git\n\nHow such a thing is handled also needs to be discussed in the\nproposed log message, and perhaps in the documentation as well.\n\n> ... My patch only supports\n> single-line comments, and only as a suffix to newly added key-value\n> pairs. This is a deliberate design choice.\n\nSuch design choices need to be described in the proposed log message\nto help future developers who will be updating this feature, once it\ngets in.\n\nThanks for writing quite a lot to answer _my_ questions, but these\nquestions are samples of things that future developers would wonder\nand ask about when they want to fix bugs in, enhance, or otherwise\nmodify the implementation of this \"add comment\" feature. They may\neven be working on adding other features to complement the \"add\ncomment\" feature, by designing support for viewing or typofixing\nexisting comments. When they do so, it would help them to know how\nthis existing feature was expected to be used and how it would fit\nin a larger picture (which may not have yet existed back when the\nfeature was invented). Answering these anticipated questions is one\nof the greatest things that a commit log message can do to help\nthem.\n\nThanks.\n\n\n"},{"id":"490173","messageId":"87plw6xld7.fsf@ra.horus-it.com","threadId":"61043","inReplyTo":"xmqqplw6nsuz.fsf@gitster.g","subject":"Re: [PATCH] Allow git-config to append a comment","fromName":"Ralph Seichter","fromEmail":"ralph@seichter.de","sentAt":"2024-03-07T12:44:04Z","receivedAt":"2024-03-07T12:44:07Z","isPatch":true,"sender":{"key":"ralph@seichter.de","avatar":null},"body":"* Junio C. Hamano:\n\n>> My patch only supports single-line comments, and only as a suffix to\n>> newly added key-value pairs. This is a deliberate design choice.\n>\n> Such design choices need to be described in the proposed log message\n> to help future developers who will be updating this feature, once it\n> gets in.\n\nJust as a brief interjection: I am sorry that my inexperience with Git's\nmailing-list based process caused me to leave out details which were\ndiscussed earlier, be it on GitHub or Discord. However, me not\nmentioning that I am aiming for single-line suffix comments only was due\nto simple forgetfulness, without an excuse behind it. My bad.\n\nI think it best I flesh out the commit message before anything else, and\nthen resubmit. That should bundle the necessary information.\n\n-Ralph\n"},{"id":"490181","messageId":"xmqqh6hinpd7.fsf@gitster.g","threadId":"61043","inReplyTo":"87plw6xld7.fsf@ra.horus-it.com","subject":"Re: [PATCH] Allow git-config to append a comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-07T13:27:48Z","receivedAt":"2024-03-07T13:27:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ralph Seichter writes:\n\n> Just as a brief interjection: I am sorry that my inexperience with Git's\n> mailing-list based process caused me to leave out details which were\n> discussed earlier, be it on GitHub or Discord. However, me not\n> mentioning that I am aiming for single-line suffix comments only was due\n> to simple forgetfulness, without an excuse behind it. My bad.\n\nDon't feel too bad. It is not about discord vs mailing list vs\ngithub \"reviews on the web\". It is about the end-product, the\ncommit log messages, what future developers will see in their \"git\nlog\" output.\n\n> I think it best I flesh out the commit message before anything else, and\n> then resubmit. That should bundle the necessary information.\n\nYup. Please do that.\n\nCommunicating with reviewers and other contributors is not the\nprimary goal of the review process. It is done to help us reach a\nbetter end-product, the commit log messages that will help our future\ndevelopers and future selves.\n\nThanks.\n"},{"id":"490183","messageId":"098101da7096$cd773d40$6865b7c0$@nexbridge.com","threadId":"61043","inReplyTo":"xmqqplw6nsuz.fsf@gitster.g","subject":"RE: [PATCH] Allow git-config to append a comment","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-03-07T13:53:09Z","receivedAt":"2024-03-07T13:53:40Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Thursday, March 7, 2024 7:12 AM, Junio C Hamano wrote:\n>Ralph Seichter writes:\n>\n>>> If you are illustrating a sample input, please also explain what\n>>> output it produces. What do the resulting lines in the config file\n>>> look like after you run this command?\n>>\n>> The result of running the above command looks as follows:\n>>\n>> [safe]\n>> \tdirectory = /home/alice/somerepo.git #I changed this. --A. Script\n>\n>That would have been a crucial piece of information to have in the proposed\nlog message, as limiting ourselves to a comment that is\n>tucked after the same line as the value, things can become somewhat\nsimplified. We may not have to worry about deletion, even\n>though the point about \"we need to look at and typofix them with our\nviewers and editors\" still stands.\n>\n>By the way, you may or may not have noticed it, but my example deliberately\nhad a multi-line comment:\n>\n> $ git config --global --comment 'the reason why I added ~alice/\n> is because ...' --add safe.directory /home/alice/somerepo.git\n>\n>How such a thing is handled also needs to be discussed in the proposed log\nmessage, and perhaps in the documentation as well.\n>\n>> ... My patch only supports\n>> single-line comments, and only as a suffix to newly added key-value\n>> pairs. This is a deliberate design choice.\n>\n>Such design choices need to be described in the proposed log message to\nhelp future developers who will be updating this feature,\n>once it gets in.\n>\n>Thanks for writing quite a lot to answer _my_ questions, but these\nquestions are samples of things that future developers would\n>wonder and ask about when they want to fix bugs in, enhance, or otherwise\nmodify the implementation of this \"add comment\"\n>feature. They may even be working on adding other features to complement\nthe \"add comment\" feature, by designing support for\n>viewing or typofixing existing comments. When they do so, it would help\nthem to know how this existing feature was expected to be\n>used and how it would fit in a larger picture (which may not have yet\nexisted back when the feature was invented). Answering these\n>anticipated questions is one of the greatest things that a commit log\nmessage can do to help them.\n>\n>Thanks.\n\nI am concerned about the compatibility of this series with the community.\nWhile comments are permitted in .gitconfig files, I am not 100% sure that\nall stakeholders, particularly those who parse .gitconfig files in their own\nscripts outside of git - sure, it is their own responsibility, but this\nmight be unexpected. I worry that this might unintentionally introduce\nincompatibilities into repository configurations. Is there broad consensus\nto do this?\n\n--Randall\n\n"},{"id":"490185","messageId":"pull.1681.v2.git.1709824540636.gitgitgadget@gmail.com","threadId":"61043","inReplyTo":"pull.1681.git.1709532018372.gitgitgadget@gmail.com","subject":"[PATCH v2] config: add --comment option to add a comment","fromName":"Ralph Seichter via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-07T15:15:39Z","receivedAt":"2024-03-07T15:15:44Z","isPatch":true,"sender":{"key":"github@seichter.de","avatar":"https://avatars.githubusercontent.com/u/30873939?v=4"},"body":"From: Ralph Seichter \n\nIntroduce the ability to append comments to modifications\nmade using git-config. Example usage:\n\n git config --comment \"changed via script\" \\\n --add safe.directory /home/alice/repo.git\n\nbased on the proposed patch, the output produced is:\n\n [safe]\n directory = /home/alice/repo.git #changed via script\n\n* Motivation:\n\nThe use case which triggered my submitting this patch is\nmy need to distinguish between config entries made using\nautomation and entries made by a human. Automation can\nadd comments containing a URL pointing to explanations\nfor the change made, avoiding questions from users as to\nwhy their config file was changed by a third party.\n\n* Design considerations and implementation details:\n\nThe implementation ensures that a # character is always\nprepended to the provided comment string, and that the\ncomment text is appended as a suffix to the changed key-\nvalue-pair in the same line of text. Multiline comments\nare deliberately not supported, because their usefulness\ndoes not justifiy the possible problems they pose when\nit comes to removing ML comments later.\n\n* Target audience:\n\nRegarding the intended consumers of the comments made:\nThey are aimed at humans who inspect or change their Git\nconfig using a pager or editor. Comments are not meant\nto be read or displayed by git-config at a later time.\n\nSigned-off-by: Ralph Seichter \n---\n config: add --comment option to add a comment\n \n config: add --comment option to add a comment\n \n Introduce the ability to append comments to modifications made using\n git-config. Example usage:\n \n git config --comment \"changed via script\" \\\n --add safe.directory /home/alice/repo.git\n \n \n based on the proposed patch, the output produced is:\n \n [safe]\n directory = /home/alice/repo.git #changed via script\n \n \n * Motivation: The use case which triggered my submitting this patch is\n my need to distinguish between config entries made using automation\n and entries made by a human. Automation can add comments containing a\n URL pointing to explanations for the change made, avoiding questions\n from users as to why their config file was changed by a third party.\n \n * Design considerations and implementation details: The implementation\n ensures that a # character is always prepended to the provided\n comment string, and that the comment text is appended as a suffix to\n the changed key- value-pair in the same line of text. Multiline\n comments are deliberately not supported, because their usefulness\n does not justifiy the possible problems they pose when it comes to\n removing ML comments later.\n \n * Target audience: Regarding the intended consumers of the comments\n made: They are aimed at humans who inspect or change their Git config\n using a pager or editor. Comments are not meant to be read or\n displayed by git-config at a later time.\n \n Changes since v1:\n \n * Rewrite commit message to address reviewers' questions.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1681%2Frseichter%2Fissue-1680-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1681/rseichter/issue-1680-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1681\n\nRange-diff vs v1:\n\n 1: d07cbb4bbf7 ! 1: 1e6ccc81685 Allow git-config to append a comment\n @@ Metadata\n Author: Ralph Seichter \n \n ## Commit message ##\n - Allow git-config to append a comment\n + config: add --comment option to add a comment\n \n Introduce the ability to append comments to modifications\n made using git-config. Example usage:\n \n - git config --comment \"I changed this. --A. Script\" \\\n - --add safe.directory /home/alice/somerepo.git\n + git config --comment \"changed via script\" \\\n + --add safe.directory /home/alice/repo.git\n +\n + based on the proposed patch, the output produced is:\n +\n + [safe]\n + directory = /home/alice/repo.git #changed via script\n +\n + * Motivation:\n +\n + The use case which triggered my submitting this patch is\n + my need to distinguish between config entries made using\n + automation and entries made by a human. Automation can\n + add comments containing a URL pointing to explanations\n + for the change made, avoiding questions from users as to\n + why their config file was changed by a third party.\n +\n + * Design considerations and implementation details:\n \n The implementation ensures that a # character is always\n - prepended to the provided comment string.\n + prepended to the provided comment string, and that the\n + comment text is appended as a suffix to the changed key-\n + value-pair in the same line of text. Multiline comments\n + are deliberately not supported, because their usefulness\n + does not justifiy the possible problems they pose when\n + it comes to removing ML comments later.\n +\n + * Target audience:\n +\n + Regarding the intended consumers of the comments made:\n + They are aimed at humans who inspect or change their Git\n + config using a pager or editor. Comments are not meant\n + to be read or displayed by git-config at a later time.\n \n Signed-off-by: Ralph Seichter \n \n\n\n Documentation/git-config.txt | 10 +++++++---\n builtin/config.c | 21 ++++++++++++++-------\n builtin/gc.c | 4 ++--\n builtin/submodule--helper.c | 2 +-\n builtin/worktree.c | 4 ++--\n config.c | 21 +++++++++++++--------\n config.h | 4 ++--\n sequencer.c | 28 ++++++++++++++--------------\n submodule-config.c | 2 +-\n submodule.c | 2 +-\n t/t1300-config.sh | 9 +++++++--\n worktree.c | 4 ++--\n 12 files changed, 66 insertions(+), 45 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex dff39093b5e..ee8cd251b24 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -9,9 +9,9 @@ git-config - Get and set repository or global options\n SYNOPSIS\n --------\n [verse]\n-'git config' [] [--type=] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] [ []]\n-'git config' [] [--type=] --add \n-'git config' [] [--type=] [--fixed-value] --replace-all []\n+'git config' [] [--type=] [--comment=] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] [ []]\n+'git config' [] [--type=] [--comment=] --add \n+'git config' [] [--type=] [--comment=] [--fixed-value] --replace-all []\n 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get []\n 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all []\n 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp []\n@@ -87,6 +87,10 @@ OPTIONS\n \tvalues. This is the same as providing '^$' as the `value-pattern`\n \tin `--replace-all`.\n \n+--comment::\n+\tAppend a comment to new or modified lines. A '#' character\n+\twill be automatically prepended to the value.\n+\n --get::\n \tGet the value for a given key (optionally filtered by a regex\n \tmatching the value). Returns error code 1 if the key was not\ndiff --git a/builtin/config.c b/builtin/config.c\nindex b55bfae7d66..2aab3c0baf3 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -44,6 +44,7 @@ static struct config_options config_options;\n static int show_origin;\n static int show_scope;\n static int fixed_value;\n+static const char *comment;\n \n #define ACTION_GET (1<<0)\n #define ACTION_GET_ALL (1<<1)\n@@ -173,6 +174,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BOOL(0, \"show-origin\", &show_origin, N_(\"show origin of config (file, standard input, blob, command line)\")),\n \tOPT_BOOL(0, \"show-scope\", &show_scope, N_(\"show scope of config (worktree, local, global, system, command)\")),\n \tOPT_STRING(0, \"default\", &default_value, N_(\"value\"), N_(\"with --get, use default value when missing entry\")),\n+\tOPT_STRING(0, \"comment\", &comment, N_(\"value\"), N_(\"human-readable comment string (# will be prepended automatically)\")),\n \tOPT_END(),\n };\n \n@@ -797,6 +799,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tusage_builtin_config();\n \t}\n \n+\tif (comment && !(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) {\n+\t\terror(_(\"--comment is only applicable to add/set/replace operations\"));\n+\t\tusage_builtin_config();\n+\t}\n+\n \t/* check usage of --fixed-value */\n \tif (fixed_value) {\n \t\tint allowed_usage = 0;\n@@ -880,7 +887,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1], &default_kvi);\n-\t\tret = git_config_set_in_file_gently(given_config_source.file, argv[0], value);\n+\t\tret = git_config_set_in_file_gently(given_config_source.file, argv[0], comment, value);\n \t\tif (ret == CONFIG_NOTHING_SET)\n \t\t\terror(_(\"cannot overwrite multiple values with a single value\\n\"\n \t\t\t\" Use a regexp, --add or --replace-all to change %s.\"), argv[0]);\n@@ -891,7 +898,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tvalue = normalize_value(argv[0], argv[1], &default_kvi);\n \t\tret = git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], value, argv[2],\n-\t\t\t\t\t\t\t flags);\n+\t\t\t\t\t\t\t comment, flags);\n \t}\n \telse if (actions == ACTION_ADD) {\n \t\tcheck_write();\n@@ -900,7 +907,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tret = git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], value,\n \t\t\t\t\t\t\t CONFIG_REGEX_NONE,\n-\t\t\t\t\t\t\t flags);\n+\t\t\t\t\t\t\t comment, flags);\n \t}\n \telse if (actions == ACTION_REPLACE_ALL) {\n \t\tcheck_write();\n@@ -908,7 +915,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tvalue = normalize_value(argv[0], argv[1], &default_kvi);\n \t\tret = git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], value, argv[2],\n-\t\t\t\t\t\t\t flags | CONFIG_FLAGS_MULTI_REPLACE);\n+\t\t\t\t\t\t\t comment, flags | CONFIG_FLAGS_MULTI_REPLACE);\n \t}\n \telse if (actions == ACTION_GET) {\n \t\tcheck_argc(argc, 1, 2);\n@@ -936,17 +943,17 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tif (argc == 2)\n \t\t\treturn git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t\t argv[0], NULL, argv[1],\n-\t\t\t\t\t\t\t\t flags);\n+\t\t\t\t\t\t\t\t NULL, flags);\n \t\telse\n \t\t\treturn git_config_set_in_file_gently(given_config_source.file,\n-\t\t\t\t\t\t\t argv[0], NULL);\n+\t\t\t\t\t\t\t argv[0], NULL, NULL);\n \t}\n \telse if (actions == ACTION_UNSET_ALL) {\n \t\tcheck_write();\n \t\tcheck_argc(argc, 1, 2);\n \t\treturn git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], NULL, argv[1],\n-\t\t\t\t\t\t\t flags | CONFIG_FLAGS_MULTI_REPLACE);\n+\t\t\t\t\t\t\t NULL, flags | CONFIG_FLAGS_MULTI_REPLACE);\n \t}\n \telse if (actions == ACTION_RENAME_SECTION) {\n \t\tcheck_write();\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex cb80ced6cb5..342907f7bdb 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1553,7 +1553,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"$HOME not set\"));\n \t\trc = git_config_set_multivar_in_file_gently(\n \t\t\tconfig_file, \"maintenance.repo\", maintpath,\n-\t\t\tCONFIG_REGEX_NONE, 0);\n+\t\t\tCONFIG_REGEX_NONE, NULL, 0);\n \t\tfree(global_config_file);\n \n \t\tif (rc)\n@@ -1620,7 +1620,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \t\tif (!config_file)\n \t\t\tdie(_(\"$HOME not set\"));\n \t\trc = git_config_set_multivar_in_file_gently(\n-\t\t\tconfig_file, key, NULL, maintpath,\n+\t\t\tconfig_file, key, NULL, maintpath, NULL,\n \t\t\tCONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);\n \t\tfree(global_config_file);\n \ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex fda50f2af1e..e4e18adb575 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1283,7 +1283,7 @@ static void sync_submodule(const char *path, const char *prefix,\n \tsubmodule_to_gitdir(&sb, path);\n \tstrbuf_addstr(&sb, \"/config\");\n \n-\tif (git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url))\n+\tif (git_config_set_in_file_gently(sb.buf, remote_key, NULL, sub_origin_url))\n \t\tdie(_(\"failed to update remote for submodule '%s'\"),\n \t\t path);\n \ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 9c76b62b02d..a20cc8820e5 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -365,12 +365,12 @@ static void copy_filtered_worktree_config(const char *worktree_git_dir)\n \t\tif (!git_configset_get_bool(&cs, \"core.bare\", &bare) &&\n \t\t\tbare &&\n \t\t\tgit_config_set_multivar_in_file_gently(\n-\t\t\t\tto_file, \"core.bare\", NULL, \"true\", 0))\n+\t\t\t\tto_file, \"core.bare\", NULL, \"true\", NULL, 0))\n \t\t\terror(_(\"failed to unset '%s' in '%s'\"),\n \t\t\t\t\"core.bare\", to_file);\n \t\tif (!git_configset_get(&cs, \"core.worktree\") &&\n \t\t\tgit_config_set_in_file_gently(to_file,\n-\t\t\t\t\t\t\t\"core.worktree\", NULL))\n+\t\t\t\t\t\t\t\"core.worktree\", NULL, NULL))\n \t\t\terror(_(\"failed to unset '%s' in '%s'\"),\n \t\t\t\t\"core.worktree\", to_file);\n \ndiff --git a/config.c b/config.c\nindex 3cfeb3d8bd9..a22594eabd9 100644\n--- a/config.c\n+++ b/config.c\n@@ -3001,6 +3001,7 @@ static ssize_t write_section(int fd, const char *key,\n }\n \n static ssize_t write_pair(int fd, const char *key, const char *value,\n+\t\t\t const char *comment,\n \t\t\t const struct config_store_data *store)\n {\n \tint i;\n@@ -3041,7 +3042,10 @@ static ssize_t write_pair(int fd, const char *key, const char *value,\n \t\t\tstrbuf_addch(&sb, value[i]);\n \t\t\tbreak;\n \t\t}\n-\tstrbuf_addf(&sb, \"%s\\n\", quote);\n+\tif (comment)\n+\t\tstrbuf_addf(&sb, \"%s #%s\\n\", quote, comment);\n+\telse\n+\t\tstrbuf_addf(&sb, \"%s\\n\", quote);\n \n \tret = write_in_full(fd, sb.buf, sb.len);\n \tstrbuf_release(&sb);\n@@ -3130,9 +3134,9 @@ static void maybe_remove_section(struct config_store_data *store,\n }\n \n int git_config_set_in_file_gently(const char *config_filename,\n-\t\t\t\t const char *key, const char *value)\n+\t\t\t\t const char *key, const char *comment, const char *value)\n {\n-\treturn git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, 0);\n+\treturn git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, comment, 0);\n }\n \n void git_config_set_in_file(const char *config_filename,\n@@ -3153,7 +3157,7 @@ int repo_config_set_worktree_gently(struct repository *r,\n \tif (r->repository_format_worktree_config) {\n \t\tchar *file = repo_git_path(r, \"config.worktree\");\n \t\tint ret = git_config_set_multivar_in_file_gently(\n-\t\t\t\t\tfile, key, value, NULL, 0);\n+\t\t\t\t\tfile, key, value, NULL, NULL, 0);\n \t\tfree(file);\n \t\treturn ret;\n \t}\n@@ -3195,6 +3199,7 @@ void git_config_set(const char *key, const char *value)\n int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\t\t\t const char *key, const char *value,\n \t\t\t\t\t const char *value_pattern,\n+\t\t\t\t\t const char *comment,\n \t\t\t\t\t unsigned flags)\n {\n \tint fd = -1, in_fd = -1;\n@@ -3245,7 +3250,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\tfree(store.key);\n \t\tstore.key = xstrdup(key);\n \t\tif (write_section(fd, key, &store) < 0 ||\n-\t\t write_pair(fd, key, value, &store) < 0)\n+\t\t write_pair(fd, key, value, comment, &store) < 0)\n \t\t\tgoto write_err_out;\n \t} else {\n \t\tstruct stat st;\n@@ -3399,7 +3404,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\t\tif (write_section(fd, key, &store) < 0)\n \t\t\t\t\tgoto write_err_out;\n \t\t\t}\n-\t\t\tif (write_pair(fd, key, value, &store) < 0)\n+\t\t\tif (write_pair(fd, key, value, comment, &store) < 0)\n \t\t\t\tgoto write_err_out;\n \t\t}\n \n@@ -3444,7 +3449,7 @@ void git_config_set_multivar_in_file(const char *config_filename,\n \t\t\t\t const char *value_pattern, unsigned flags)\n {\n \tif (!git_config_set_multivar_in_file_gently(config_filename, key, value,\n-\t\t\t\t\t\t value_pattern, flags))\n+\t\t\t\t\t\t value_pattern, NULL, flags))\n \t\treturn;\n \tif (value)\n \t\tdie(_(\"could not set '%s' to '%s'\"), key, value);\n@@ -3467,7 +3472,7 @@ int repo_config_set_multivar_gently(struct repository *r, const char *key,\n \tint res = git_config_set_multivar_in_file_gently(file,\n \t\t\t\t\t\t\t key, value,\n \t\t\t\t\t\t\t value_pattern,\n-\t\t\t\t\t\t\t flags);\n+\t\t\t\t\t\t\t NULL, flags);\n \tfree(file);\n \treturn res;\n }\ndiff --git a/config.h b/config.h\nindex 5dba984f770..a85a8271696 100644\n--- a/config.h\n+++ b/config.h\n@@ -290,7 +290,7 @@ int git_config_pathname(const char **, const char *, const char *);\n \n int git_config_expiry_date(timestamp_t *, const char *, const char *);\n int git_config_color(char *, const char *, const char *);\n-int git_config_set_in_file_gently(const char *, const char *, const char *);\n+int git_config_set_in_file_gently(const char *, const char *, const char *, const char *);\n \n /**\n * write config values to a specific config file, takes a key/value pair as\n@@ -336,7 +336,7 @@ int git_config_parse_key(const char *, char **, size_t *);\n int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);\n void git_config_set_multivar(const char *, const char *, const char *, unsigned);\n int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);\n-int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);\n+int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, const char *, unsigned);\n \n /**\n * takes four parameters:\ndiff --git a/sequencer.c b/sequencer.c\nindex f49a871ac06..4c91ca5a844 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3460,54 +3460,54 @@ static int save_opts(struct replay_opts *opts)\n \n \tif (opts->no_commit)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.no-commit\", \"true\");\n+\t\t\t\t\t\"options.no-commit\", NULL, \"true\");\n \tif (opts->edit >= 0)\n-\t\tres |= git_config_set_in_file_gently(opts_file, \"options.edit\",\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.edit\", NULL,\n \t\t\t\t\t\t opts->edit ? \"true\" : \"false\");\n \tif (opts->allow_empty)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.allow-empty\", \"true\");\n+\t\t\t\t\t\"options.allow-empty\", NULL, \"true\");\n \tif (opts->allow_empty_message)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\"options.allow-empty-message\", \"true\");\n+\t\t\t\t\"options.allow-empty-message\", NULL, \"true\");\n \tif (opts->keep_redundant_commits)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\"options.keep-redundant-commits\", \"true\");\n+\t\t\t\t\"options.keep-redundant-commits\", NULL, \"true\");\n \tif (opts->signoff)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.signoff\", \"true\");\n+\t\t\t\t\t\"options.signoff\", NULL, \"true\");\n \tif (opts->record_origin)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.record-origin\", \"true\");\n+\t\t\t\t\t\"options.record-origin\", NULL, \"true\");\n \tif (opts->allow_ff)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.allow-ff\", \"true\");\n+\t\t\t\t\t\"options.allow-ff\", NULL, \"true\");\n \tif (opts->mainline) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstrbuf_addf(&buf, \"%d\", opts->mainline);\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.mainline\", buf.buf);\n+\t\t\t\t\t\"options.mainline\", NULL, buf.buf);\n \t\tstrbuf_release(&buf);\n \t}\n \tif (opts->strategy)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.strategy\", opts->strategy);\n+\t\t\t\t\t\"options.strategy\", NULL, opts->strategy);\n \tif (opts->gpg_sign)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.gpg-sign\", opts->gpg_sign);\n+\t\t\t\t\t\"options.gpg-sign\", NULL, opts->gpg_sign);\n \tfor (size_t i = 0; i < opts->xopts.nr; i++)\n \t\tres |= git_config_set_multivar_in_file_gently(opts_file,\n \t\t\t\t\"options.strategy-option\",\n-\t\t\t\topts->xopts.v[i], \"^$\", 0);\n+\t\t\t\topts->xopts.v[i], \"^$\", NULL, 0);\n \tif (opts->allow_rerere_auto)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\"options.allow-rerere-auto\",\n+\t\t\t\t\"options.allow-rerere-auto\", NULL,\n \t\t\t\topts->allow_rerere_auto == RERERE_AUTOUPDATE ?\n \t\t\t\t\"true\" : \"false\");\n \n \tif (opts->explicit_cleanup)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\"options.default-msg-cleanup\",\n+\t\t\t\t\"options.default-msg-cleanup\", NULL,\n \t\t\t\tdescribe_cleanup_mode(opts->default_msg_cleanup));\n \treturn res;\n }\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 54130f6a385..11428b4adad 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -978,7 +978,7 @@ int config_set_in_gitmodules_file_gently(const char *key, const char *value)\n {\n \tint ret;\n \n-\tret = git_config_set_in_file_gently(GITMODULES_FILE, key, value);\n+\tret = git_config_set_in_file_gently(GITMODULES_FILE, key, NULL, value);\n \tif (ret < 0)\n \t\t/* Maybe the user already did that, don't error out here */\n \t\twarning(_(\"Could not update .gitmodules entry %s\"), key);\ndiff --git a/submodule.c b/submodule.c\nindex 213da79f661..86630932d09 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -2052,7 +2052,7 @@ void submodule_unset_core_worktree(const struct submodule *sub)\n \tsubmodule_name_to_gitdir(&config_path, the_repository, sub->name);\n \tstrbuf_addstr(&config_path, \"/config\");\n \n-\tif (git_config_set_in_file_gently(config_path.buf, \"core.worktree\", NULL))\n+\tif (git_config_set_in_file_gently(config_path.buf, \"core.worktree\", NULL, NULL))\n \t\twarning(_(\"Could not unset core.worktree setting in submodule '%s'\"),\n \t\t\t sub->path);\n \ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 31c38786870..daddaedd23c 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -69,13 +69,18 @@ test_expect_success 'replace with non-match (actually matching)' '\n \n cat > expect << EOF\n [section]\n-\tpenguin = very blue\n \tMovie = BadPhysics\n \tUPPERCASE = true\n-\tpenguin = kingpin\n+\tpenguin = gentoo #Pygoscelis papua\n+\tdisposition = peckish #find fish\n [Sections]\n \tWhatEver = Second\n EOF\n+test_expect_success 'append comments' '\n+\tgit config --replace-all --comment=\"Pygoscelis papua\" section.penguin gentoo &&\n+\tgit config --comment=\"find fish\" section.disposition peckish &&\n+\ttest_cmp expect .git/config\n+'\n \n test_expect_success 'non-match result' 'test_cmp expect .git/config'\n \ndiff --git a/worktree.c b/worktree.c\nindex b02a05a74a3..cf5eea8c931 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -807,9 +807,9 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,\n static int move_config_setting(const char *key, const char *value,\n \t\t\t const char *from_file, const char *to_file)\n {\n-\tif (git_config_set_in_file_gently(to_file, key, value))\n+\tif (git_config_set_in_file_gently(to_file, key, NULL, value))\n \t\treturn error(_(\"unable to set %s in '%s'\"), key, to_file);\n-\tif (git_config_set_in_file_gently(from_file, key, NULL))\n+\tif (git_config_set_in_file_gently(from_file, key, NULL, NULL))\n \t\treturn error(_(\"unable to unset %s in '%s'\"), key, from_file);\n \treturn 0;\n }\n\nbase-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d\n-- \ngitgitgadget\n"},{"id":"490191","messageId":"87h6higj1a.fsf@ra.horus-it.com","threadId":"61043","inReplyTo":"098101da7096$cd773d40$6865b7c0$@nexbridge.com","subject":"RE: [PATCH] Allow git-config to append a comment","fromName":"Ralph Seichter","fromEmail":"github@seichter.de","sentAt":"2024-03-07T15:26:25Z","receivedAt":"2024-03-07T15:26:27Z","isPatch":true,"sender":{"key":"github@seichter.de","avatar":"https://avatars.githubusercontent.com/u/30873939?v=4"},"body":"* rsbecker@nexbridge.com:\n\n> While comments are permitted in .gitconfig files, I am not 100% sure\n> that all stakeholders, particularly those who parse .gitconfig files\n> in their own scripts outside of git - sure, it is their own\n> responsibility, but this might be unexpected.\n\nComments are nothing new, and humans have added far crazier comments to\ntheir Git config in the past. The patch ensures that a '#' precedes the\ncomments added using git-config, which is not guaranteed to happen when\nJoe Random User manually edits config files.\n\nI think that anybody incapable of reliably dealing with comments in\nconfig files would already have fallen flat on his/her nose, regardless\nof how those comments were made.\n\n> I worry that this might unintentionally introduce incompatibilities\n> into repository configurations.\n\nDo you have an example?\n\n-Ralph\n"},{"id":"490193","messageId":"09c501da70a5$bd2f4f40$378dedc0$@nexbridge.com","threadId":"61043","inReplyTo":"87h6higj1a.fsf@ra.horus-it.com","subject":"RE: [PATCH] Allow git-config to append a comment","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-03-07T15:40:04Z","receivedAt":"2024-03-07T15:40:24Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Thursday, March 7, 2024 10:26 AM, Ralph Seichter wrote:\n>* rsbecker@nexbridge.com:\n>\n>> While comments are permitted in .gitconfig files, I am not 100% sure\n>> that all stakeholders, particularly those who parse .gitconfig files\n>> in their own scripts outside of git - sure, it is their own\n>> responsibility, but this might be unexpected.\n>\n>Comments are nothing new, and humans have added far crazier comments to\ntheir Git config in the past. The patch ensures that a '#'\n>precedes the comments added using git-config, which is not guaranteed to\nhappen when Joe Random User manually edits config files.\n>\n>I think that anybody incapable of reliably dealing with comments in config\nfiles would already have fallen flat on his/her nose,\n>regardless of how those comments were made.\n>\n>> I worry that this might unintentionally introduce incompatibilities\n>> into repository configurations.\n>\n>Do you have an example?\n\nNo example. This is a comment on \"potential\" changes to data that scripts\naround git for automation purposes might use. My purpose is just to\nhighlight, for the purpose of reviewing the change, that there may be\nunintended impacts, that's all. It may be useful to include comments in the\nchange notices and documentation pages that using this capability may impact\nscripting. When a user manually puts in a comment, any breakages in their\nscripts are 100% their issue. With git config moving comments around,\nresponsibility shifts to git - a.k.a., unintended consequences. I am not\nasking that this change not happen - it is a good thing, but ensuring that\nwe communicate that this may cause breakages if external programs/scripts\nread .gitconfig would be helpful. This also would need to be coordinated\nwith the libification efforts at some point.\n\n"},{"id":"490194","messageId":"87wmqevxur.fsf@ra.horus-it.com","threadId":"61043","inReplyTo":"09c501da70a5$bd2f4f40$378dedc0$@nexbridge.com","subject":"RE: [PATCH] Allow git-config to append a comment","fromName":"Ralph Seichter","fromEmail":"github@seichter.de","sentAt":"2024-03-07T15:57:16Z","receivedAt":"2024-03-07T15:57:18Z","isPatch":true,"sender":{"key":"github@seichter.de","avatar":"https://avatars.githubusercontent.com/u/30873939?v=4"},"body":"* rsbecker@nexbridge.com:\n\n> My purpose is just to highlight, for the purpose of reviewing the\n> change, that there may be unintended impacts, that's all.\n\nI see. Have you perhaps spotted a flaw in the patch which might cause\nproblems? If so, I'd like to address it.\n\n> With git config moving comments around, responsibility shifts to git -\n> a.k.a., unintended consequences.\n\nIt is important to note that my implementation does not engage in\n\"moving comments around\" at all. It merely supports adding comment\nsuffixes to config lines *while they are created by git-config*. No\nafter-the-fact comment manipulation is done. There are no multiline\ncomments involved either, which I should have mentioned from the get-go.\nThe limited scope of my proposal is deliberate, and aimed at avoiding\npossible problems based on the design alone.\n\n-Ralph\n"},{"id":"490355","messageId":"xmqqy1apudvv.fsf@gitster.g","threadId":"61043","inReplyTo":"pull.1681.v2.git.1709824540636.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-11T12:55:16Z","receivedAt":"2024-03-11T12:55:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ralph Seichter via GitGitGadget\" writes:\n\n> From: Ralph Seichter \n>\n> Introduce the ability to append comments to modifications\n> made using git-config. Example usage:\n>\n> git config --comment \"changed via script\" \\\n> --add safe.directory /home/alice/repo.git\n>\n> based on the proposed patch, the output produced is:\n>\n> [safe]\n> directory = /home/alice/repo.git #changed via script\n\nFor readability, you probably would want to have a SP before the\ngiven string, i.e.,\n\n\tvariable = \"value\" # message comes here\n\n> * Motivation:\n>\n> The use case which triggered my submitting this patch is\n\nThese two lines should not be needed. It is customary to just state\nthat the users need to be able to do X and adding feature Y is one\nway to allow that, without such preamble.\n\n> my need to distinguish between config entries made using\n> automation and entries made by a human. Automation can\n> add comments containing a URL pointing to explanations\n> for the change made, avoiding questions from users as to\n> why their config file was changed by a third party.\n>\n> * Design considerations and implementation details:\n>\n> The implementation ensures that a # character is always\n> prepended to the provided comment string, and that the\n\nIt is unclear what happens when a user gives comment that already\nhas the comment introducer, e.g., --comment=\"# this is comment\".\n\n> comment text is appended as a suffix to the changed key-\n> value-pair in the same line of text. Multiline comments\n> are deliberately not supported, because their usefulness\n\n\"not supported\" can take many shapes, ranging from \"producing a\nrandom result and may corrupt the resulting configuration file\"\nand \"the second and subsequent lines are silently ignored\", to\n\"results in an error, stops the command before doing anything\".\n\n> does not justifiy the possible problems they pose when\n> it comes to removing ML comments later.\n\n\"justify\"\n\n>\n> * Target audience:\n>\n> Regarding the intended consumers of the comments made:\n\nThe above two lines say the same thing and are unnecessary,\nespecially before a sentence that begins with \"They are aimed at\".\n\n> They are aimed at humans who inspect or change their Git\n> config using a pager or editor. Comments are not meant\n> to be read or displayed by git-config at a later time.\n>\n> Signed-off-by: Ralph Seichter \n> ---\n> config: add --comment option to add a comment\n>\n> config: add --comment option to add a comment\n> \n> Introduce the ability to append comments to modifications made using\n> ...\n> displayed by git-config at a later time.\n\nNo need to repeat a wall of text below the three-dash line if they\ndo not give additional information on top of what is already in the\nproposed commit log message.\n\n> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> index dff39093b5e..ee8cd251b24 100644\n> --- a/Documentation/git-config.txt\n> +++ b/Documentation/git-config.txt\n> @@ -9,9 +9,9 @@ git-config - Get and set repository or global options\n> SYNOPSIS\n> --------\n> [verse]\n> -'git config' [] [--type=] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] [ []]\n> -'git config' [] [--type=] --add \n> -'git config' [] [--type=] [--fixed-value] --replace-all []\n> +'git config' [] [--type=] [--comment=] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] [ []]\n> +'git config' [] [--type=] [--comment=] --add \n> +'git config' [] [--type=] [--comment=] [--fixed-value] --replace-all []\n> 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get []\n> 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all []\n> 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp []\n\nThere is Patrick's proposal to revamp the UI of this command,\n\n https://lore.kernel.org/git/cover.1709724089.git.ps@pks.im/\n\nwhich may make the above simpler (e.g., there won't be two lines\nthat talks about \"set\" commands, with and without \"--add\" option,\nfor example). This topic may want to at least wait for the\nconclusion of that design discussion, and possibly its\nimplementation.\n\n> @@ -87,6 +87,10 @@ OPTIONS\n> \tvalues. This is the same as providing '^$' as the `value-pattern`\n> \tin `--replace-all`.\n> \n> +--comment::\n> +\tAppend a comment to new or modified lines. A '#' character\n> +\twill be automatically prepended to the value.\n\nOther options that take mandatory parameter are are described with\ntheir parameter on the item heading line. It may help to somehow\nmention that the \"appending\" is done at the end on the same line.\nPerhaps something along this line.\n\n\t--comment ::\n\t\tAppend the __ at the end of the new or\n\t\tmodified lines with the value. A comment introducer\n\t\t` # ` is prepended to __ if it does not\n\t\talready have one. The command errors out if given\n\t\ta __ that spans multiple lines.\n\n\n> @@ -797,6 +799,11 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n> \t\tusage_builtin_config();\n> \t}\n> \n> +\tif (comment && !(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) {\n\nThis overly long line is both visually annoying and harder to\nmaintain. If you can design a solution that makes it easier to read\na future patch that adds support of the comment option to more\nactions (or removes it from an action), that would be great.\nOtherwise, do at least:\n\n\tif (comment &&\n\t !(actions & (...)) {\n\n> +\t\terror(_(\"--comment is only applicable to add/set/replace operations\"));\n> +\t\tusage_builtin_config();\n> +\t}\n> +\n\nIf I were doing this, I'd probably add this new block after the\nfixed-value thing, as it is bad manners to add new code _before_\nexisting ones, as if your new invention were somehow more important\nthan all the others, when the order does not matter. If there is a\nvalid technical reason (e.g., the new code modifies the state of the\nprogram that affects the beahviour of the later and existing parts\nof the code), the above comment of course does not apply.\n\n> \t/* check usage of --fixed-value */\n> \tif (fixed_value) {\n> \t\tint allowed_usage = 0;\n\n\n> @@ -880,7 +887,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n> \t\tcheck_write();\n> \t\tcheck_argc(argc, 2, 2);\n> \t\tvalue = normalize_value(argv[0], argv[1], &default_kvi);\n> -\t\tret = git_config_set_in_file_gently(given_config_source.file, argv[0], value);\n> +\t\tret = git_config_set_in_file_gently(given_config_source.file, argv[0], comment, value);\n\nIt is somewhat annoying that we have to see so much code churn to\nadd this new parameter X-<. I notice that we are sending \"comment\"\nto the underlying machinery without doing ANY sanity checking (like,\n\"ah, we got a message without the '#' prefix, so we should add '# '\nin front of it before asking the API to add that comment string to\nthe value line\"). We may also want to have a code that says:\n\n Yikes, this message has LF in it, but the underlying machinery\n does not check for it and end up corrupting the configuration\n file by creating\n\n\t[section]\n\t\tvar = value #comment\n\tthat spans on two lines\n\n when given\n\n\tLF='\n\t'\n git config --comment=\"comment${LF}that spans...\" section.var value\n\nor something. The underlying machinery should be updated to die()\nwhen given such a message instead of silently corrupting the\nresulting file, but the front-end that receives the end-user input\nshould check for obviously problematic payload before bothering the\nAPI with it.\n\n> -\tstrbuf_addf(&sb, \"%s\\n\", quote);\n> +\tif (comment)\n> +\t\tstrbuf_addf(&sb, \"%s #%s\\n\", quote, comment);\n> +\telse\n> +\t\tstrbuf_addf(&sb, \"%s\\n\", quote);\n\n> diff --git a/config.h b/config.h\n> index 5dba984f770..a85a8271696 100644\n> --- a/config.h\n> +++ b/config.h\n> @@ -290,7 +290,7 @@ int git_config_pathname(const char **, const char *, const char *);\n> \n> int git_config_expiry_date(timestamp_t *, const char *, const char *);\n> int git_config_color(char *, const char *, const char *);\n> -int git_config_set_in_file_gently(const char *, const char *, const char *);\n\nThe original was probably OK without naming these parameters of the\nsame type, as it is (arguably) obvious to have the filename first,\nbecause that is what differenciates the \"in-file\" variant. And then\nkey followed by value, because that is the usual \"assignment\" order\npeople would naturally expect.\n\nBut it was already on the borderline. The idea is that we do not\nhave to (but still can) name the parameters in our function\ndeclarations when it is obvious from their types what they are.\nAddition of this comment thing will push us way over the line.\n\nThe same comment applies to many of the function declarations\ntouched by this patch.\n\nIf I were doing this patch, I'd have at least one clean-up patch\nthat updates this line to read:\n\n int git_config_set_in_file_gently(const char *filename, const char *key, const char *value);\n\nAnd then write a separate patch to add the \"--comment\" feature.\n\nI however have some suspicion that we might move away from \"listing\nmany random parameters\" style to \"having only the essential\nparameters, together with a single pointer to a structure that holds\nthe optional bits\" style, especially when the UI revamp Patrick\nproposed comes. In the case of config API, is the\nessential bit in the write direction, and value-pattern restriction\nand the bit that says key is an regexp would be in \"the optional\nbits\" group. This thing will also be in the latter class.\n\n> diff --git a/t/t1300-config.sh b/t/t1300-config.sh\n> index 31c38786870..daddaedd23c 100755\n> --- a/t/t1300-config.sh\n> +++ b/t/t1300-config.sh\n> @@ -69,13 +69,18 @@ test_expect_success 'replace with non-match (actually matching)' '\n> \n> cat > expect << EOF\n> [section]\n> -\tpenguin = very blue\n> \tMovie = BadPhysics\n> \tUPPERCASE = true\n> -\tpenguin = kingpin\n> +\tpenguin = gentoo #Pygoscelis papua\n> +\tdisposition = peckish #find fish\n> [Sections]\n> \tWhatEver = Second\n> EOF\n> +test_expect_success 'append comments' '\n> +\tgit config --replace-all --comment=\"Pygoscelis papua\" section.penguin gentoo &&\n> +\tgit config --comment=\"find fish\" section.disposition peckish &&\n> +\ttest_cmp expect .git/config\n> +'\n\nAdd test for at least two more cases and show what should happen.\n\n\t--comment=\"# the user already has the pound\"\n\t--comment=\"this is being ${LF} naughty to break configuration\"\n\nThanks.\n"},{"id":"490362","messageId":"5eff951e815e2fdab3834c4aa4160ed8@manjaro.org","threadId":"61043","inReplyTo":"xmqqy1apudvv.fsf@gitster.g","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-11T16:17:13Z","receivedAt":"2024-03-11T16:17:17Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-11 13:55, Junio C Hamano wrote:\n> \"Ralph Seichter via GitGitGadget\" writes:\n> \n>> From: Ralph Seichter \n>> \n>> Introduce the ability to append comments to modifications\n>> made using git-config. Example usage:\n>> \n>> git config --comment \"changed via script\" \\\n>> --add safe.directory /home/alice/repo.git\n>> \n>> based on the proposed patch, the output produced is:\n>> \n>> [safe]\n>> directory = /home/alice/repo.git #changed via script\n> \n> For readability, you probably would want to have a SP before the\n> given string, i.e.,\n> \n> \tvariable = \"value\" # message comes here\n\nLet me interject... Perhaps also a tab character before the \"# \ncomment\",\ninstead of a space character. That would result in even better \nreadability.\n"},{"id":"490364","messageId":"0b8701da73d3$fa0f2080$ee2d6180$@nexbridge.com","threadId":"61043","inReplyTo":"5eff951e815e2fdab3834c4aa4160ed8@manjaro.org","subject":"RE: [PATCH v2] config: add --comment option to add a comment","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-03-11T16:48:37Z","receivedAt":"2024-03-11T16:49:15Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Monday, March 11, 2024 12:17 PM, Dragan Simic wrote:\n>Subject: Re: [PATCH v2] config: add --comment option to add a comment\n>\n>On 2024-03-11 13:55, Junio C Hamano wrote:\n>> \"Ralph Seichter via GitGitGadget\" writes:\n>>\n>>> From: Ralph Seichter \n>>>\n>>> Introduce the ability to append comments to modifications made using\n>>> git-config. Example usage:\n>>>\n>>> git config --comment \"changed via script\" \\\n>>> --add safe.directory /home/alice/repo.git\n>>>\n>>> based on the proposed patch, the output produced is:\n>>>\n>>> [safe]\n>>> directory = /home/alice/repo.git #changed via script\n>>\n>> For readability, you probably would want to have a SP before the given\n>> string, i.e.,\n>>\n>> \tvariable = \"value\" # message comes here\n>\n>Let me interject... Perhaps also a tab character before the \"# comment\",\ninstead of a space character. That would result in even better\n>readability.\n\nDoes adding a tab following data change the parse semantics of .gitconfig?\nMy naïve understanding is that .gitconfig follows a basic rule of leading\ntab within a section, followed by text. Is there a formal syntax description\nof what valid input is? The value does not need to be quoted, so what does\nthe following actually resolve to:\n\n(TAB)variable = value(TAB)# comment.\n\nDoes variable mean value or value(TAB)? Obviously TABS should be correctly\nbe interpreted as whitespace to be ignored. However, what about:\n\n(TAB)variable = value(TAB)s(TAB) # comment.\n\nDoes that mean value(TAB)s, value(TAB)s(TAB), value s, value s(TAB), values?\n\nThe definition according to git-config is\n\n\"The syntax is fairly flexible and permissive; whitespaces are mostly\nignored. The # and ; characters begin comments to the end of line, blank\nlines are ignored.\"\n\n\"Mostly\" does not make me comfortable that this is formally allowed or\ndisallowed or ignored. I would suggest that this change needs to formalize\nthe grammar on that documentation page for clarity.\n\n"},{"id":"490366","messageId":"e7fe2e229f0b5c76fdec1d789bfecd6e@manjaro.org","threadId":"61043","inReplyTo":"0b8701da73d3$fa0f2080$ee2d6180$@nexbridge.com","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-11T17:00:40Z","receivedAt":"2024-03-11T17:00:42Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-11 17:48, rsbecker@nexbridge.com wrote:\n> On Monday, March 11, 2024 12:17 PM, Dragan Simic wrote:\n>> Subject: Re: [PATCH v2] config: add --comment option to add a comment\n>> \n>> On 2024-03-11 13:55, Junio C Hamano wrote:\n>>> \"Ralph Seichter via GitGitGadget\" writes:\n>>> \n>>>> From: Ralph Seichter \n>>>> \n>>>> Introduce the ability to append comments to modifications made using\n>>>> git-config. Example usage:\n>>>> \n>>>> git config --comment \"changed via script\" \\\n>>>> --add safe.directory /home/alice/repo.git\n>>>> \n>>>> based on the proposed patch, the output produced is:\n>>>> \n>>>> [safe]\n>>>> directory = /home/alice/repo.git #changed via script\n>>> \n>>> For readability, you probably would want to have a SP before the \n>>> given\n>>> string, i.e.,\n>>> \n>>> \tvariable = \"value\" # message comes here\n>> \n>> Let me interject... Perhaps also a tab character before the \"# \n>> comment\",\n> instead of a space character. That would result in even better\n>> readability.\n> \n> Does adding a tab following data change the parse semantics of \n> .gitconfig?\n> My naïve understanding is that .gitconfig follows a basic rule of \n> leading\n> tab within a section, followed by text. Is there a formal syntax \n> description\n> of what valid input is? The value does not need to be quoted, so what \n> does\n> the following actually resolve to:\n> \n> (TAB)variable = value(TAB)# comment.\n> \n> Does variable mean value or value(TAB)? Obviously TABS should be \n> correctly\n> be interpreted as whitespace to be ignored. However, what about:\n> \n> (TAB)variable = value(TAB)s(TAB) # comment.\n> \n> Does that mean value(TAB)s, value(TAB)s(TAB), value s, value s(TAB), \n> values?\n\nIt should mean \"value(TAB)s\", according to git-config(1).\n\n> The definition according to git-config is\n> \n> \"The syntax is fairly flexible and permissive; whitespaces are mostly\n> ignored. The # and ; characters begin comments to the end of line, \n> blank\n> lines are ignored.\"\n\nI believe these two quotations from git-config(1) should make it more \nclear:\n\n A line that defines a value can be continued to the next line by \nending\n it with a \\; the backslash and the end-of-line are stripped. Leading\n whitespaces after name =, the remainder of the line after the first\n comment character # or ;, and trailing whitespaces of the line are\n discarded unless they are enclosed in double quotes. Internal\n whitespaces within the value are retained verbatim.\n\n The following escape sequences (beside \\\" and \\\\) are recognized: \\n\n for newline character (NL), \\t for horizontal tabulation (HT, TAB) \nand\n \\b for backspace (BS). Other char escape sequences (including octal\n escape sequences) are invalid.\n\nTo me, all that indicates that trailing tab characters are stripped, \nbut...\n\n> \"Mostly\" does not make me comfortable that this is formally allowed or\n> disallowed or ignored. I would suggest that this change needs to \n> formalize\n> the grammar on that documentation page for clarity.\n\n... I do agree that it should be clarified further in the man page.\n"},{"id":"490369","messageId":"xmqqr0ggsmmw.fsf@gitster.g","threadId":"61043","inReplyTo":"5eff951e815e2fdab3834c4aa4160ed8@manjaro.org","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-11T17:29:11Z","receivedAt":"2024-03-11T17:29:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n> On 2024-03-11 13:55, Junio C Hamano wrote:\n>> \"Ralph Seichter via GitGitGadget\" writes:\n>> \n>>> From: Ralph Seichter \n>>> Introduce the ability to append comments to modifications\n>>> made using git-config. Example usage:\n>>> git config --comment \"changed via script\" \\\n>>> --add safe.directory /home/alice/repo.git\n>>> based on the proposed patch, the output produced is:\n>>> [safe]\n>>> directory = /home/alice/repo.git #changed via script\n>> For readability, you probably would want to have a SP before the\n>> given string, i.e.,\n>> \tvariable = \"value\" # message comes here\n>\n> Let me interject... Perhaps also a tab character before the \"#\n> comment\",\n> instead of a space character. That would result in even better\n> readability.\n\nDepends on your screen width ;-)\n\nIf you were trying to tell me that SP or no SP is merely a personal\npreference with the comment, I think you succeeded in doing so.\n\nThanks.\n"},{"id":"490371","messageId":"dd818f1709834ca6cb9050fb4ad0f4ea@manjaro.org","threadId":"61043","inReplyTo":"xmqqr0ggsmmw.fsf@gitster.g","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-11T17:34:41Z","receivedAt":"2024-03-11T17:34:44Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-11 18:29, Junio C Hamano wrote:\n> Dragan Simic writes:\n>> On 2024-03-11 13:55, Junio C Hamano wrote:\n>>> \"Ralph Seichter via GitGitGadget\" writes:\n>>> \n>>>> From: Ralph Seichter \n>>>> Introduce the ability to append comments to modifications\n>>>> made using git-config. Example usage:\n>>>> git config --comment \"changed via script\" \\\n>>>> --add safe.directory /home/alice/repo.git\n>>>> based on the proposed patch, the output produced is:\n>>>> [safe]\n>>>> directory = /home/alice/repo.git #changed via script\n>>> For readability, you probably would want to have a SP before the\n>>> given string, i.e.,\n>>> \tvariable = \"value\" # message comes here\n>> \n>> Let me interject... Perhaps also a tab character before the \"#\n>> comment\",\n>> instead of a space character. That would result in even better\n>> readability.\n> \n> Depends on your screen width ;-)\n\nAh, screens are pretty wide these days. :)\n\n> If you were trying to tell me that SP or no SP is merely a personal\n> preference with the comment, I think you succeeded in doing so.\n\nHuh, that wasn't my intention. IMHO, a space character between \"#\"\nand the actual comment is pretty much mandatory.\n"},{"id":"490375","messageId":"0f3a39ab1eeb28201b474afb7fd92c89@manjaro.org","threadId":"61043","inReplyTo":"e7fe2e229f0b5c76fdec1d789bfecd6e@manjaro.org","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-11T17:52:21Z","receivedAt":"2024-03-11T17:52:24Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-11 18:00, Dragan Simic wrote:\n> On 2024-03-11 17:48, rsbecker@nexbridge.com wrote:\n>> On Monday, March 11, 2024 12:17 PM, Dragan Simic wrote:\n>>> Subject: Re: [PATCH v2] config: add --comment option to add a comment\n>>> \n>>> On 2024-03-11 13:55, Junio C Hamano wrote:\n>>>> \"Ralph Seichter via GitGitGadget\" writes:\n>>>> \n>>>>> From: Ralph Seichter \n>>>>> \n>>>>> Introduce the ability to append comments to modifications made \n>>>>> using\n>>>>> git-config. Example usage:\n>>>>> \n>>>>> git config --comment \"changed via script\" \\\n>>>>> --add safe.directory /home/alice/repo.git\n>>>>> \n>>>>> based on the proposed patch, the output produced is:\n>>>>> \n>>>>> [safe]\n>>>>> directory = /home/alice/repo.git #changed via script\n>>>> \n>>>> For readability, you probably would want to have a SP before the \n>>>> given\n>>>> string, i.e.,\n>>>> \n>>>> \tvariable = \"value\" # message comes here\n>>> \n>>> Let me interject... Perhaps also a tab character before the \"# \n>>> comment\",\n>> instead of a space character. That would result in even better\n>>> readability.\n>> \n>> Does adding a tab following data change the parse semantics of \n>> .gitconfig?\n>> My naïve understanding is that .gitconfig follows a basic rule of \n>> leading\n>> tab within a section, followed by text. Is there a formal syntax \n>> description\n>> of what valid input is? The value does not need to be quoted, so what \n>> does\n>> the following actually resolve to:\n>> \n>> (TAB)variable = value(TAB)# comment.\n>> \n>> Does variable mean value or value(TAB)? Obviously TABS should be \n>> correctly\n>> be interpreted as whitespace to be ignored. However, what about:\n>> \n>> (TAB)variable = value(TAB)s(TAB) # comment.\n>> \n>> Does that mean value(TAB)s, value(TAB)s(TAB), value s, value s(TAB), \n>> values?\n> \n> It should mean \"value(TAB)s\", according to git-config(1).\n\nHmm, after having a look at config.c and doing some testing, \ngit-config(1)\nseems to be wrong when it says that \"internal whitespaces within the \nvalue\nare retained verbatim\".\n\nHere's an example... I placed the following into ~/.gitconfig:\n\n [test]\n\t blah = huh\tuh\n\nThere's a tab character between \"huh\" and \"uh\". Though, running\n\n git config --get test.blah\n\nproduces\n\n huh uh\n\nwhich contains a space character, not a tab. That's expected according \nto\nconfig.c, but not according to git-config(1).\n\nShould we correct the wording in git-config(1) accordingly?\n\n>> The definition according to git-config is\n>> \n>> \"The syntax is fairly flexible and permissive; whitespaces are mostly\n>> ignored. The # and ; characters begin comments to the end of line, \n>> blank\n>> lines are ignored.\"\n> \n> I believe these two quotations from git-config(1) should make it more \n> clear:\n> \n> A line that defines a value can be continued to the next line by \n> ending\n> it with a \\; the backslash and the end-of-line are stripped. \n> Leading\n> whitespaces after name =, the remainder of the line after the first\n> comment character # or ;, and trailing whitespaces of the line are\n> discarded unless they are enclosed in double quotes. Internal\n> whitespaces within the value are retained verbatim.\n> \n> The following escape sequences (beside \\\" and \\\\) are recognized: \n> \\n\n> for newline character (NL), \\t for horizontal tabulation (HT, TAB) \n> and\n> \\b for backspace (BS). Other char escape sequences (including octal\n> escape sequences) are invalid.\n> \n> To me, all that indicates that trailing tab characters are stripped, \n> but...\n\nAfter some testing, I can confirm that any tabs (or spaces, or mixes) \nbetween\nthe value and the \"#\" character do get ignored.\n\n>> \"Mostly\" does not make me comfortable that this is formally allowed or\n>> disallowed or ignored. I would suggest that this change needs to \n>> formalize\n>> the grammar on that documentation page for clarity.\n> \n> ... I do agree that it should be clarified further in the man page.\n"},{"id":"490378","messageId":"9166fa83-4ed5-41fd-83f7-337fd524d0e2@seichter.de","threadId":"61043","inReplyTo":"xmqqy1apudvv.fsf@gitster.g","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Ralph Seichter","fromEmail":"github@seichter.de","sentAt":"2024-03-11T18:16:58Z","receivedAt":"2024-03-11T18:17:08Z","isPatch":true,"sender":{"key":"github@seichter.de","avatar":"https://avatars.githubusercontent.com/u/30873939?v=4"},"body":"Preface: I am not replying to every part of Junio's email this time\naround. However, that does not imply I am ignoring them, I simply do\nnot currently have access to the source code.\n\n\n* Junio C Hamano:\n\n > For readability, you probably would want to have a SP before the\n > given string, i.e.,\n >\n > \tvariable = \"value\" # message comes here\n\nIf the user wants a whitespace after the #, they can add it in the\ncomment using quoting, e.g. --comment \" message comes here\". I don't\nthink it is necessary to enforce the extra SP, because it is not\nsyntactically required. Besides, readability is quite subjective.\n\n >> The implementation ensures that a # character is always\n >> prepended to the provided comment string, and that the\n >\n > It is unclear what happens when a user gives comment that already\n > has the comment introducer, e.g., --comment=\"# this is comment\".\n\nUnclear? ;-) I wrote \"a # character is always prepended to the\nprovided comment string\", and that is what happens. The current\nimplementation is meant to be safe, not fancy.\n\n > No need to repeat a wall of text below the three-dash line if they\n > do not give additional information on top of what is already in the\n > proposed commit log message.\n\nThat's GitGitGadget's doing. Unfortunately, it does not want to let me\nremove my pull request description on the GitHub website. I already\ntried to fiddle with it, but no success yet; GitHub keeps restoring my\nlatest description text.\n\n > This topic may want to at least wait for the conclusion of that\n > design discussion, and possibly its implementation.\n\nI don't know enough about the timespans involved to be sure, but that\nsounds like it could take a long time?\n\n > I notice that we are sending \"comment\" to the underlying machinery\n > without doing ANY sanity checking (like, \"ah, we got a message\n > without the '#' prefix, so we should add '# ' in front of it before\n > asking the API to add that comment string to the value line\").\n\nAs I wrote before, the # is prepended unconditionally. LF is a more\ninteresting question. I haven't yet tried actively introducing a\nlinefeed. Does strbuf_addf() have any related filtering logic?\n\n-Ralph\n"},{"id":"490379","messageId":"ba6d65ca-8ae2-4d93-a5a5-e0b60768c302@seichter.de","threadId":"61043","inReplyTo":"5eff951e815e2fdab3834c4aa4160ed8@manjaro.org","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Ralph Seichter","fromEmail":"github@seichter.de","sentAt":"2024-03-11T18:23:11Z","receivedAt":"2024-03-11T18:23:14Z","isPatch":true,"sender":{"key":"github@seichter.de","avatar":"https://avatars.githubusercontent.com/u/30873939?v=4"},"body":"* Dragan Simic:\n\n > Let me interject... Perhaps also a tab character before the \"#\n > comment\", instead of a space character. That would result in even\n > better readability.\n\nI'd rather not open /that/ can of worms. Tabs versus spaces, tab size,\nand so forth, are too much matters of personal taste for me to want to\nspend any time discussing.\n\n-Ralph\n\n"},{"id":"490381","messageId":"265e5b2d352c34dfe0625904b3356000@manjaro.org","threadId":"61043","inReplyTo":"ba6d65ca-8ae2-4d93-a5a5-e0b60768c302@seichter.de","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-11T18:50:07Z","receivedAt":"2024-03-11T18:50:11Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-11 19:23, Ralph Seichter wrote:\n> * Dragan Simic:\n> \n>> Let me interject... Perhaps also a tab character before the \"#\n>> comment\", instead of a space character. That would result in even\n>> better readability.\n> \n> I'd rather not open /that/ can of worms. Tabs versus spaces, tab size,\n> and so forth, are too much matters of personal taste for me to want to\n> spend any time discussing.\n\nI see, but please note that everyone should be prepared to spend some\ntime discussing even seemingly unrelated things when contributing to\na major open-source project.\n\nI mean, perhaps the whole thing with the tab characters may not be\nthe right example, but I just wanted to point out that the more major\nan open-source project is, the more discussion is often required.\n"},{"id":"490382","messageId":"52cbdb9d1cbdb254dfdac467711becbf@manjaro.org","threadId":"61043","inReplyTo":"9166fa83-4ed5-41fd-83f7-337fd524d0e2@seichter.de","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-11T18:55:00Z","receivedAt":"2024-03-11T18:55:02Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-11 19:16, Ralph Seichter wrote:\n> * Junio C Hamano:\n> \n>> For readability, you probably would want to have a SP before the\n>> given string, i.e.,\n>> \n>> \tvariable = \"value\" # message comes here\n> \n> If the user wants a whitespace after the #, they can add it in the\n> comment using quoting, e.g. --comment \" message comes here\". I don't\n> think it is necessary to enforce the extra SP, because it is not\n> syntactically required. Besides, readability is quite subjective.\n\nPerhaps that should be documented, so the users know what to expect\nand how to ensure extra spacing, if they desire so.\n\n>>> The implementation ensures that a # character is always\n>>> prepended to the provided comment string, and that the\n>> \n>> It is unclear what happens when a user gives comment that already\n>> has the comment introducer, e.g., --comment=\"# this is comment\".\n> \n> Unclear? ;-) I wrote \"a # character is always prepended to the\n> provided comment string\", and that is what happens. The current\n> implementation is meant to be safe, not fancy.\n\nPerhaps that should also be documented, to avoid the \"##\" sequences\nfrom occurring unexpectedly.\n"},{"id":"490387","messageId":"277b3187-f793-4782-aa18-50bc15febe9a@seichter.de","threadId":"61043","inReplyTo":"265e5b2d352c34dfe0625904b3356000@manjaro.org","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Ralph Seichter","fromEmail":"github@seichter.de","sentAt":"2024-03-11T18:57:03Z","receivedAt":"2024-03-11T18:57:06Z","isPatch":true,"sender":{"key":"github@seichter.de","avatar":"https://avatars.githubusercontent.com/u/30873939?v=4"},"body":"* Dragan Simic:\n\n > I mean, perhaps the whole thing with the tab characters may not be\n > the right example, but I just wanted to point out that the more\n > major an open-source project is, the more discussion is often\n > required.\n\nOh, I have no qualms discussing things, but over the last 40+ years,\nnothing good has ever come from debating the pros and cons of tabs and\nspaces. At least that's my personal experience.\n\n-Ralph\n\n"},{"id":"490388","messageId":"349b8a67931a2fddfff35db393023bf0@manjaro.org","threadId":"61043","inReplyTo":"277b3187-f793-4782-aa18-50bc15febe9a@seichter.de","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-11T19:04:34Z","receivedAt":"2024-03-11T19:04:36Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-11 19:57, Ralph Seichter wrote:\n> * Dragan Simic:\n> \n>> I mean, perhaps the whole thing with the tab characters may not be\n>> the right example, but I just wanted to point out that the more\n>> major an open-source project is, the more discussion is often\n>> required.\n> \n> Oh, I have no qualms discussing things, but over the last 40+ years,\n> nothing good has ever come from debating the pros and cons of tabs and\n> spaces. At least that's my personal experience.\n\nThere are pros and cons to both spaces and tabs, so there's hardly\nanything that can be concluded about either option being better or\nworse than the other. They're both good and bad at the same time.\n\nHowever, I think there should be some way to allow the users to choose\nthe kind of spacing between the automatically added comments and their\nrespective values. Yes, readability may be subjective, but I think\nthat the users should be allowed some kind of choice.\n"},{"id":"490389","messageId":"e645729b-a605-43b7-83fa-18fdec282b42@seichter.de","threadId":"61043","inReplyTo":"52cbdb9d1cbdb254dfdac467711becbf@manjaro.org","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Ralph Seichter","fromEmail":"github@seichter.de","sentAt":"2024-03-11T19:04:42Z","receivedAt":"2024-03-11T19:04:44Z","isPatch":true,"sender":{"key":"github@seichter.de","avatar":"https://avatars.githubusercontent.com/u/30873939?v=4"},"body":"* Dragan Simic:\n\n > Perhaps that should be documented, so the users know what to expect\n > and how to ensure extra spacing, if they desire so.\n\nYup, better to be explicit than implicit. By the way: When it comes to\nmy proposed patch, I am having small difficulties finding a balance\nbetween documenting things thoroughly and being accused of producing\nwalls of text. ;-)\n\n-Ralph\n\n"},{"id":"490390","messageId":"0b9a01da73e8$b7cd8f80$2768ae80$@nexbridge.com","threadId":"61043","inReplyTo":"277b3187-f793-4782-aa18-50bc15febe9a@seichter.de","subject":"RE: [PATCH v2] config: add --comment option to add a comment","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-03-11T19:17:05Z","receivedAt":"2024-03-11T19:17:26Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Monday, March 11, 2024 2:57 PM, Ralph Seichter wrote:\n>Subject: Re: [PATCH v2] config: add --comment option to add a comment\n>\n>* Dragan Simic:\n>\n> > 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\n>more > major an open-source project is, the more discussion is often > required.\n>\n>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\n>tabs and spaces. At least that's my personal experience.\n\nMy take would be that all whitespace is ignored, but if you want a tab or other character in a value, be explicit about it:\n\n\tvariable = \"value\\ts\" # comment\n\nwhere all whitespace and comments are dropped from the parse to be functionally equivalent to\n\nvariable=value(TAB)s\n\nwhen processing. Implicit quoting arbitrary whitespace can be interpreted in an ambiguous way. That's basically what the C parser does.\n\n--Randall\n\n\n"},{"id":"490395","messageId":"xmqqzfv4pmrn.fsf@gitster.g","threadId":"61043","inReplyTo":"dd818f1709834ca6cb9050fb4ad0f4ea@manjaro.org","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-11T19:54:36Z","receivedAt":"2024-03-11T19:54:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n>>> Let me interject... Perhaps also a tab character before the \"#\n>>> comment\",\n>>> instead of a space character. That would result in even better\n>>> readability.\n>> Depends on your screen width ;-)\n>\n> Ah, screens are pretty wide these days. :)\n>\n>> If you were trying to tell me that SP or no SP is merely a personal\n>> preference with the comment, I think you succeeded in doing so.\n>\n> Huh, that wasn't my intention. IMHO, a space character between \"#\"\n> and the actual comment is pretty much mandatory.\n\nAh, OK, you were talking about the gap after the value before the\n\"#\" that introduces the comment, but I somehow mistook it as a\ncomment about the whitespace after '#'.\n\nThe gap after the value, I do not have a strong opinion either way\nbetween SP and HT, except that I agree there should be something\nthere for readability.\n\nGiven that other places where we do insert comments, like in the log\nmessage editor during \"git commit -e\", we always give a single space\nafter the comment character, I tend to agree that a space after '#'\nis pretty much mandatory. It is a non starter to tell users that\nthey should add their own SP at the beginning if they want to use\nsuch a common style, i.e.\n\n\tgit commit --comment=' here is my message' ;# BAD\n\nWith a simple rule like \"Unless your message begins with '#', the\nmessage is prepended by '# ' (pound, followed by a SP), but when\nyour message begins with '#', the string is used as is\", those who\nwant to use their own style can use whatever style they want, e.g.\n\n\tgit commit --comment='#I do not want SP there'\n\tgit commit --comment='#^II want a HT there instead'\n\nand that would be a much more preferrable design, i.e. making the\ncommon things easy, while leaving unusual things possible.\n\nThanks.\n\n"},{"id":"490399","messageId":"xmqq7ci8pi9d.fsf@gitster.g","threadId":"61043","inReplyTo":"349b8a67931a2fddfff35db393023bf0@manjaro.org","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-11T21:31:58Z","receivedAt":"2024-03-11T21:32:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dragan Simic writes:\n\n> However, I think there should be some way to allow the users to choose\n> the kind of spacing between the automatically added comments and their\n> respective values. Yes, readability may be subjective, but I think\n> that the users should be allowed some kind of choice.\n\nAs to the whitespace _before_ the '#', we can just pick one and\nleave the choice to users' editor, which they have to resort to\nanyway for even trivial things like fixing typos. I am fine to\ndefer the choice between HT and SP to Ralph as the person in the\ndriver seat,\n\nAs to the whitespace _after_ the '#', I would say we have obligation\nto the users to be consistent with other codepaths that we already\nwrite our own comments. \"# \" is the only sensible choice of the\ndefault there, and we can allow other styles as I outlined in a\nseparate message if we wanted to.\n"},{"id":"490440","messageId":"5413ccb588807965c5e54d579db1d741@manjaro.org","threadId":"61043","inReplyTo":"xmqqzfv4pmrn.fsf@gitster.g","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-12T02:25:37Z","receivedAt":"2024-03-12T02:25:40Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-11 20:54, Junio C Hamano wrote:\n> Dragan Simic writes:\n> \n>>>> Let me interject... Perhaps also a tab character before the \"#\n>>>> comment\",\n>>>> instead of a space character. That would result in even better\n>>>> readability.\n>>> Depends on your screen width ;-)\n>> \n>> Ah, screens are pretty wide these days. :)\n>> \n>>> If you were trying to tell me that SP or no SP is merely a personal\n>>> preference with the comment, I think you succeeded in doing so.\n>> \n>> Huh, that wasn't my intention. IMHO, a space character between \"#\"\n>> and the actual comment is pretty much mandatory.\n> \n> Ah, OK, you were talking about the gap after the value before the\n> \"#\" that introduces the comment, but I somehow mistook it as a\n> comment about the whitespace after '#'.\n\nYes, that's what I was talking about. I'm sorry if the way I wrote it\ninitially wasn't clear enough.\n\n> The gap after the value, I do not have a strong opinion either way\n> between SP and HT, except that I agree there should be something\n> there for readability.\n\nI'd vote for a space character after \"#\", because that's pretty much\nthe de facto standard. I don't remember seeing tabs used there.\n\n> Given that other places where we do insert comments, like in the log\n> message editor during \"git commit -e\", we always give a single space\n> after the comment character, I tend to agree that a space after '#'\n> is pretty much mandatory. It is a non starter to tell users that\n> they should add their own SP at the beginning if they want to use\n> such a common style, i.e.\n> \n> \tgit commit --comment=' here is my message' ;# BAD\n\nI'd agree with that. Requiring the users to include a leading space\nwould make things inconistent.\n\n> With a simple rule like \"Unless your message begins with '#', the\n> message is prepended by '# ' (pound, followed by a SP), but when\n> your message begins with '#', the string is used as is\", those who\n> want to use their own style can use whatever style they want, e.g.\n> \n> \tgit commit --comment='#I do not want SP there'\n> \tgit commit --comment='#^II want a HT there instead'\n> \n> and that would be a much more preferrable design, i.e. making the\n> common things easy, while leaving unusual things possible.\n\nAgreed. That would be nice.\n"},{"id":"490441","messageId":"41c7cfa73dc0b4e7345ba788c45beead@manjaro.org","threadId":"61043","inReplyTo":"0b9a01da73e8$b7cd8f80$2768ae80$@nexbridge.com","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-12T02:27:05Z","receivedAt":"2024-03-12T02:27:07Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-11 20:17, rsbecker@nexbridge.com wrote:\n> My take would be that all whitespace is ignored, but if you want a tab\n> or other character in a value, be explicit about it:\n> \n> \tvariable = \"value\\ts\" # comment\n> \n> where all whitespace and comments are dropped from the parse to be\n> functionally equivalent to\n> \n> variable=value(TAB)s\n> \n> when processing. Implicit quoting arbitrary whitespace can be\n> interpreted in an ambiguous way. That's basically what the C parser\n> does.\n\nOh, I fully agree, but I'm afraid that would be a breaking change at \nthis\npoint. Perhaps numerous configuration values in the field already use \nthe\ncurrent (mis)behavior of the value parser in config.c.\n\nI think the only right thing to do at this point is to document it \nproperly\nand correctly in the git-config(1) manual page. A bit later I'll \nprepare\nand send a patch that does it.\n"},{"id":"490442","messageId":"361f3554c172f43c0aff5d1a46dc7e29@manjaro.org","threadId":"61043","inReplyTo":"xmqq7ci8pi9d.fsf@gitster.g","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-12T02:38:46Z","receivedAt":"2024-03-12T02:38:49Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"On 2024-03-11 22:31, Junio C Hamano wrote:\n> Dragan Simic writes:\n> \n>> However, I think there should be some way to allow the users to choose\n>> the kind of spacing between the automatically added comments and their\n>> respective values. Yes, readability may be subjective, but I think\n>> that the users should be allowed some kind of choice.\n> \n> As to the whitespace _before_ the '#', we can just pick one and\n> leave the choice to users' editor, which they have to resort to\n> anyway for even trivial things like fixing typos. I am fine to\n> defer the choice between HT and SP to Ralph as the person in the\n> driver seat,\n\nI'd vote for tabs before hashes (i.e. \n\"name(SP)=(SP)value(HT)#(SP)comment\"),\nbecause that's what I've seen used in numerous codebases. Thus, that \nshould\nintroduce some kind of additional consistency.\n\n> As to the whitespace _after_ the '#', I would say we have obligation\n> to the users to be consistent with other codepaths that we already\n> write our own comments. \"# \" is the only sensible choice of the\n> default there, and we can allow other styles as I outlined in a\n> separate message if we wanted to.\n\nI'd agree with that, as I already noted in an earlier reply.\n"},{"id":"490446","messageId":"cf21edb2-7681-42a9-8b58-beca5d04c179@seichter.de","threadId":"61043","inReplyTo":"9166fa83-4ed5-41fd-83f7-337fd524d0e2@seichter.de","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Ralph Seichter","fromEmail":"github@seichter.de","sentAt":"2024-03-12T06:19:26Z","receivedAt":"2024-03-12T06:19:32Z","isPatch":true,"sender":{"key":"github@seichter.de","avatar":"https://avatars.githubusercontent.com/u/30873939?v=4"},"body":"* Ralph Seichter:\n\n > LF is a more interesting question. I haven't yet tried actively\n > introducing a linefeed. Does strbuf_addf() have any related\n > filtering logic?\n\nI have now tried several times to inject a LF into a comment string,\nand did not manage to break the resulting config file. For example,\n\n ./git config --comment \"foo\\012bar\" --add section.key value\n\nleads to this entry:\n\n [section]\n key = value #foo\\012bar\n\nVariable expansion with LF=\"\\012\" and --comment \"foo${LF}bar\" did not\nintroduce a linefeed either. Have you guys perhaps managed to create\nan invalid config file?\n\n-Ralph\n"},{"id":"490447","messageId":"CAPx1Gvd4FjCTSn4oUWNDtLS2G4V3yrddggafX0WHfHEdojtCNQ@mail.gmail.com","threadId":"61043","inReplyTo":"cf21edb2-7681-42a9-8b58-beca5d04c179@seichter.de","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2024-03-12T06:37:42Z","receivedAt":"2024-03-12T06:37:55Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Mon, Mar 11, 2024 at 11:22 PM Ralph Seichter wrote:\n> I have now tried several times to inject a LF into a comment string ...\n> Variable expansion with LF=\"\\012\" ...\n\nYou must use a literal line feed, e.g.:\n\n LF='\n '\n\n(a single quoted newline) or, in a shell that supports this syntax:\n\n LF=$'\\012'\n\n(note $ and single quote: double quotes here do not work)\nto get the line-feed into the shell variable. You can also capture\nthe output of a program that emits the line-feed, but these are the direct\nassignment methods.\n\nFor instance:\n\n $ LF=$'\\012'\n $ echo \"foo${LF}bar\"\n foo\n bar\n $\n\nLacking such capabilities, it's easy enough to use the printf shell\ncommand to produce special characters as output (which you can\nthen capture, but various shells may also have Special Rules about\nsuch captured output, so the direct assignment method is better, in\nmy opinion).\n\nChris\n"},{"id":"490448","messageId":"877ci7lxid.fsf@ra.horus-it.com","threadId":"61043","inReplyTo":"CAPx1Gvd4FjCTSn4oUWNDtLS2G4V3yrddggafX0WHfHEdojtCNQ@mail.gmail.com","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Ralph Seichter","fromEmail":"github@seichter.de","sentAt":"2024-03-12T07:28:26Z","receivedAt":"2024-03-12T07:28:31Z","isPatch":true,"sender":{"key":"github@seichter.de","avatar":"https://avatars.githubusercontent.com/u/30873939?v=4"},"body":"* Chris Torek:\n\n> You must use a literal line feed, e.g.:\n>\n> LF='\n> '\n\nOf course, silly me. Easily done in a shell script. I was too focused on\ntrying it in an interactive shell. Thanks, Chris.\n\nDo you perhaps know of a function in the Git code base which could be\nused to sanitise strings? It is quite a lot of code to sift through if\none is unfamiliar with it, so I'll gladly take hints.\n\n-Ralph\n"},{"id":"490449","messageId":"xmqqmsr3lwry.fsf@gitster.g","threadId":"61043","inReplyTo":"877ci7lxid.fsf@ra.horus-it.com","subject":"Re: [PATCH v2] config: add --comment option to add a comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-12T07:44:17Z","receivedAt":"2024-03-12T07:44:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ralph Seichter writes:\n\n> * Chris Torek:\n>\n>> You must use a literal line feed, e.g.:\n>>\n>> LF='\n>> '\n>\n> Of course, silly me. Easily done in a shell script. I was too focused on\n> trying it in an interactive shell. Thanks, Chris.\n\n;-) \n\nI think I've already given you that in my first message that\nmentioned multi-line input. In the test suite, we already have that\n$LF defined so your tests can use it without defining it yourself.\n\n> Do you perhaps know of a function in the Git code base which could be\n> used to sanitise strings? It is quite a lot of code to sift through if\n> one is unfamiliar with it, so I'll gladly take hints.\n\nDon't silently butcher end-user input; instead do something like\nthis to make it clear that you do not support it.\n\n\tif (strchr(comment_message, '\\n'))\n\t\tdie(_(\"multi-line comment not supported: '%s'\"),\n\t\t\tcomment_message);\n\n"},{"id":"490512","messageId":"pull.1681.v3.git.1710280020508.gitgitgadget@gmail.com","threadId":"61043","inReplyTo":"pull.1681.v2.git.1709824540636.gitgitgadget@gmail.com","subject":"[PATCH v3] config: add --comment option to add a comment","fromName":"Ralph Seichter via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-12T21:47:00Z","receivedAt":"2024-03-12T21:47:04Z","isPatch":true,"sender":{"key":"github@seichter.de","avatar":"https://avatars.githubusercontent.com/u/30873939?v=4"},"body":"From: Ralph Seichter \n\nIntroduce the ability to append comments to modifications\nmade using git-config. Example usage:\n\n git config --comment \"changed via script\" \\\n --add safe.directory /home/alice/repo.git\n\nbased on the proposed patch, the output produced is:\n\n [safe]\n directory = /home/alice/repo.git #changed via script\n\nUsers need to be able to distinguish between config entries made\nusing automation and entries made by a human. Automation can add\ncomments containing a URL pointing to explanations for the change\nmade, avoiding questions from users as to why their config file\nwas changed by a third party.\n\nThe implementation ensures that a # character is unconditionally\nprepended to the provided comment string, and that the comment\ntext is appended as a suffix to the changed key-value-pair in the\nsame line of text. Multi-line comments (i.e. comments containing\nlinefeed) are rejected as errors, causing Git to exit without\nmaking changes.\n\nComments are aimed at humans who inspect or change their Git\nconfig using a pager or editor. Comments are not meant to be\nread or displayed by git-config at a later time.\n\nSigned-off-by: Ralph Seichter \n---\n config: add --comment option to add a comment\n \n Changes since v2:\n \n * Address reviewers' remarks.\n \n Changes since v1:\n \n * Rewrite commit message to address reviewers' questions.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1681%2Frseichter%2Fissue-1680-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1681/rseichter/issue-1680-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1681\n\nRange-diff vs v2:\n\n 1: 1e6ccc81685 ! 1: 99cc839a115 config: add --comment option to add a comment\n @@ Commit message\n [safe]\n directory = /home/alice/repo.git #changed via script\n \n - * Motivation:\n + Users need to be able to distinguish between config entries made\n + using automation and entries made by a human. Automation can add\n + comments containing a URL pointing to explanations for the change\n + made, avoiding questions from users as to why their config file\n + was changed by a third party.\n \n - The use case which triggered my submitting this patch is\n - my need to distinguish between config entries made using\n - automation and entries made by a human. Automation can\n - add comments containing a URL pointing to explanations\n - for the change made, avoiding questions from users as to\n - why their config file was changed by a third party.\n + The implementation ensures that a # character is unconditionally\n + prepended to the provided comment string, and that the comment\n + text is appended as a suffix to the changed key-value-pair in the\n + same line of text. Multi-line comments (i.e. comments containing\n + linefeed) are rejected as errors, causing Git to exit without\n + making changes.\n \n - * Design considerations and implementation details:\n -\n - The implementation ensures that a # character is always\n - prepended to the provided comment string, and that the\n - comment text is appended as a suffix to the changed key-\n - value-pair in the same line of text. Multiline comments\n - are deliberately not supported, because their usefulness\n - does not justifiy the possible problems they pose when\n - it comes to removing ML comments later.\n -\n - * Target audience:\n -\n - Regarding the intended consumers of the comments made:\n - They are aimed at humans who inspect or change their Git\n - config using a pager or editor. Comments are not meant\n - to be read or displayed by git-config at a later time.\n + Comments are aimed at humans who inspect or change their Git\n + config using a pager or editor. Comments are not meant to be\n + read or displayed by git-config at a later time.\n \n Signed-off-by: Ralph Seichter \n \n @@ Documentation/git-config.txt: OPTIONS\n \tvalues. This is the same as providing '^$' as the `value-pattern`\n \tin `--replace-all`.\n \n -+--comment::\n -+\tAppend a comment to new or modified lines. A '#' character\n -+\twill be automatically prepended to the value.\n ++--comment ::\n ++\tAppend a comment to new or modified lines. A '#' character will be\n ++\tunconditionally prepended to the value. The value must not contain\n ++\tlinefeed characters (no multi-line comments are permitted).\n +\n --get::\n \tGet the value for a given key (optionally filtered by a regex\n @@ builtin/config.c: int cmd_config(int argc, const char **argv, const char *prefix\n \t\tusage_builtin_config();\n \t}\n \n -+\tif (comment && !(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) {\n -+\t\terror(_(\"--comment is only applicable to add/set/replace operations\"));\n -+\t\tusage_builtin_config();\n ++\tif (comment &&\n ++\t\t!(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) {\n ++\t\t\terror(_(\"--comment is only applicable to add/set/replace operations\"));\n ++\t\t\tusage_builtin_config();\n +\t}\n +\n \t/* check usage of --fixed-value */\n @@ config.c: static ssize_t write_pair(int fd, const char *key, const char *value,\n \t\t\tbreak;\n \t\t}\n -\tstrbuf_addf(&sb, \"%s\\n\", quote);\n -+\tif (comment)\n -+\t\tstrbuf_addf(&sb, \"%s #%s\\n\", quote, comment);\n -+\telse\n ++\n ++\tif (comment) {\n ++\t\tif (strchr(comment, '\\n'))\n ++\t\t\tdie(_(\"multi-line comments are not permitted: '%s'\"), comment);\n ++\t\telse\n ++\t\t\tstrbuf_addf(&sb, \"%s #%s\\n\", quote, comment);\n ++\t} else\n +\t\tstrbuf_addf(&sb, \"%s\\n\", quote);\n \n \tret = write_in_full(fd, sb.buf, sb.len);\n @@ t/t1300-config.sh: test_expect_success 'replace with non-match (actually matchin\n -\tpenguin = kingpin\n +\tpenguin = gentoo #Pygoscelis papua\n +\tdisposition = peckish #find fish\n ++\tfoo = bar ## abc\n [Sections]\n \tWhatEver = Second\n EOF\n +test_expect_success 'append comments' '\n +\tgit config --replace-all --comment=\"Pygoscelis papua\" section.penguin gentoo &&\n +\tgit config --comment=\"find fish\" section.disposition peckish &&\n ++\tgit config --comment=\"# abc\" section.foo bar &&\n +\ttest_cmp expect .git/config\n ++'\n ++\n ++test_expect_success 'Prohibited LF in comment' '\n ++\ttest_must_fail git config --comment=\"a${LF}b\" section.k v\n +'\n \n test_expect_success 'non-match result' 'test_cmp expect .git/config'\n\n\n Documentation/git-config.txt | 11 ++++++++---\n builtin/config.c | 22 +++++++++++++++-------\n builtin/gc.c | 4 ++--\n builtin/submodule--helper.c | 2 +-\n builtin/worktree.c | 4 ++--\n config.c | 25 +++++++++++++++++--------\n config.h | 4 ++--\n sequencer.c | 28 ++++++++++++++--------------\n submodule-config.c | 2 +-\n submodule.c | 2 +-\n t/t1300-config.sh | 15 +++++++++++++--\n worktree.c | 4 ++--\n 12 files changed, 78 insertions(+), 45 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex dff39093b5e..e608d5ffef2 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -9,9 +9,9 @@ git-config - Get and set repository or global options\n SYNOPSIS\n --------\n [verse]\n-'git config' [] [--type=] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] [ []]\n-'git config' [] [--type=] --add \n-'git config' [] [--type=] [--fixed-value] --replace-all []\n+'git config' [] [--type=] [--comment=] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] [ []]\n+'git config' [] [--type=] [--comment=] --add \n+'git config' [] [--type=] [--comment=] [--fixed-value] --replace-all []\n 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get []\n 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all []\n 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp []\n@@ -87,6 +87,11 @@ OPTIONS\n \tvalues. This is the same as providing '^$' as the `value-pattern`\n \tin `--replace-all`.\n \n+--comment ::\n+\tAppend a comment to new or modified lines. A '#' character will be\n+\tunconditionally prepended to the value. The value must not contain\n+\tlinefeed characters (no multi-line comments are permitted).\n+\n --get::\n \tGet the value for a given key (optionally filtered by a regex\n \tmatching the value). Returns error code 1 if the key was not\ndiff --git a/builtin/config.c b/builtin/config.c\nindex b55bfae7d66..c54e9941a5f 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -44,6 +44,7 @@ static struct config_options config_options;\n static int show_origin;\n static int show_scope;\n static int fixed_value;\n+static const char *comment;\n \n #define ACTION_GET (1<<0)\n #define ACTION_GET_ALL (1<<1)\n@@ -173,6 +174,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BOOL(0, \"show-origin\", &show_origin, N_(\"show origin of config (file, standard input, blob, command line)\")),\n \tOPT_BOOL(0, \"show-scope\", &show_scope, N_(\"show scope of config (worktree, local, global, system, command)\")),\n \tOPT_STRING(0, \"default\", &default_value, N_(\"value\"), N_(\"with --get, use default value when missing entry\")),\n+\tOPT_STRING(0, \"comment\", &comment, N_(\"value\"), N_(\"human-readable comment string (# will be prepended automatically)\")),\n \tOPT_END(),\n };\n \n@@ -797,6 +799,12 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tusage_builtin_config();\n \t}\n \n+\tif (comment &&\n+\t\t!(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) {\n+\t\t\terror(_(\"--comment is only applicable to add/set/replace operations\"));\n+\t\t\tusage_builtin_config();\n+\t}\n+\n \t/* check usage of --fixed-value */\n \tif (fixed_value) {\n \t\tint allowed_usage = 0;\n@@ -880,7 +888,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tcheck_write();\n \t\tcheck_argc(argc, 2, 2);\n \t\tvalue = normalize_value(argv[0], argv[1], &default_kvi);\n-\t\tret = git_config_set_in_file_gently(given_config_source.file, argv[0], value);\n+\t\tret = git_config_set_in_file_gently(given_config_source.file, argv[0], comment, value);\n \t\tif (ret == CONFIG_NOTHING_SET)\n \t\t\terror(_(\"cannot overwrite multiple values with a single value\\n\"\n \t\t\t\" Use a regexp, --add or --replace-all to change %s.\"), argv[0]);\n@@ -891,7 +899,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tvalue = normalize_value(argv[0], argv[1], &default_kvi);\n \t\tret = git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], value, argv[2],\n-\t\t\t\t\t\t\t flags);\n+\t\t\t\t\t\t\t comment, flags);\n \t}\n \telse if (actions == ACTION_ADD) {\n \t\tcheck_write();\n@@ -900,7 +908,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tret = git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], value,\n \t\t\t\t\t\t\t CONFIG_REGEX_NONE,\n-\t\t\t\t\t\t\t flags);\n+\t\t\t\t\t\t\t comment, flags);\n \t}\n \telse if (actions == ACTION_REPLACE_ALL) {\n \t\tcheck_write();\n@@ -908,7 +916,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tvalue = normalize_value(argv[0], argv[1], &default_kvi);\n \t\tret = git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], value, argv[2],\n-\t\t\t\t\t\t\t flags | CONFIG_FLAGS_MULTI_REPLACE);\n+\t\t\t\t\t\t\t comment, flags | CONFIG_FLAGS_MULTI_REPLACE);\n \t}\n \telse if (actions == ACTION_GET) {\n \t\tcheck_argc(argc, 1, 2);\n@@ -936,17 +944,17 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tif (argc == 2)\n \t\t\treturn git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t\t argv[0], NULL, argv[1],\n-\t\t\t\t\t\t\t\t flags);\n+\t\t\t\t\t\t\t\t NULL, flags);\n \t\telse\n \t\t\treturn git_config_set_in_file_gently(given_config_source.file,\n-\t\t\t\t\t\t\t argv[0], NULL);\n+\t\t\t\t\t\t\t argv[0], NULL, NULL);\n \t}\n \telse if (actions == ACTION_UNSET_ALL) {\n \t\tcheck_write();\n \t\tcheck_argc(argc, 1, 2);\n \t\treturn git_config_set_multivar_in_file_gently(given_config_source.file,\n \t\t\t\t\t\t\t argv[0], NULL, argv[1],\n-\t\t\t\t\t\t\t flags | CONFIG_FLAGS_MULTI_REPLACE);\n+\t\t\t\t\t\t\t NULL, flags | CONFIG_FLAGS_MULTI_REPLACE);\n \t}\n \telse if (actions == ACTION_RENAME_SECTION) {\n \t\tcheck_write();\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex cb80ced6cb5..342907f7bdb 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1553,7 +1553,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"$HOME not set\"));\n \t\trc = git_config_set_multivar_in_file_gently(\n \t\t\tconfig_file, \"maintenance.repo\", maintpath,\n-\t\t\tCONFIG_REGEX_NONE, 0);\n+\t\t\tCONFIG_REGEX_NONE, NULL, 0);\n \t\tfree(global_config_file);\n \n \t\tif (rc)\n@@ -1620,7 +1620,7 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \t\tif (!config_file)\n \t\t\tdie(_(\"$HOME not set\"));\n \t\trc = git_config_set_multivar_in_file_gently(\n-\t\t\tconfig_file, key, NULL, maintpath,\n+\t\t\tconfig_file, key, NULL, maintpath, NULL,\n \t\t\tCONFIG_FLAGS_MULTI_REPLACE | CONFIG_FLAGS_FIXED_VALUE);\n \t\tfree(global_config_file);\n \ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex fda50f2af1e..e4e18adb575 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1283,7 +1283,7 @@ static void sync_submodule(const char *path, const char *prefix,\n \tsubmodule_to_gitdir(&sb, path);\n \tstrbuf_addstr(&sb, \"/config\");\n \n-\tif (git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url))\n+\tif (git_config_set_in_file_gently(sb.buf, remote_key, NULL, sub_origin_url))\n \t\tdie(_(\"failed to update remote for submodule '%s'\"),\n \t\t path);\n \ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 9c76b62b02d..a20cc8820e5 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -365,12 +365,12 @@ static void copy_filtered_worktree_config(const char *worktree_git_dir)\n \t\tif (!git_configset_get_bool(&cs, \"core.bare\", &bare) &&\n \t\t\tbare &&\n \t\t\tgit_config_set_multivar_in_file_gently(\n-\t\t\t\tto_file, \"core.bare\", NULL, \"true\", 0))\n+\t\t\t\tto_file, \"core.bare\", NULL, \"true\", NULL, 0))\n \t\t\terror(_(\"failed to unset '%s' in '%s'\"),\n \t\t\t\t\"core.bare\", to_file);\n \t\tif (!git_configset_get(&cs, \"core.worktree\") &&\n \t\t\tgit_config_set_in_file_gently(to_file,\n-\t\t\t\t\t\t\t\"core.worktree\", NULL))\n+\t\t\t\t\t\t\t\"core.worktree\", NULL, NULL))\n \t\t\terror(_(\"failed to unset '%s' in '%s'\"),\n \t\t\t\t\"core.worktree\", to_file);\n \ndiff --git a/config.c b/config.c\nindex 3cfeb3d8bd9..2f3f6d123c6 100644\n--- a/config.c\n+++ b/config.c\n@@ -3001,6 +3001,7 @@ static ssize_t write_section(int fd, const char *key,\n }\n \n static ssize_t write_pair(int fd, const char *key, const char *value,\n+\t\t\t const char *comment,\n \t\t\t const struct config_store_data *store)\n {\n \tint i;\n@@ -3041,7 +3042,14 @@ static ssize_t write_pair(int fd, const char *key, const char *value,\n \t\t\tstrbuf_addch(&sb, value[i]);\n \t\t\tbreak;\n \t\t}\n-\tstrbuf_addf(&sb, \"%s\\n\", quote);\n+\n+\tif (comment) {\n+\t\tif (strchr(comment, '\\n'))\n+\t\t\tdie(_(\"multi-line comments are not permitted: '%s'\"), comment);\n+\t\telse\n+\t\t\tstrbuf_addf(&sb, \"%s #%s\\n\", quote, comment);\n+\t} else\n+\t\tstrbuf_addf(&sb, \"%s\\n\", quote);\n \n \tret = write_in_full(fd, sb.buf, sb.len);\n \tstrbuf_release(&sb);\n@@ -3130,9 +3138,9 @@ static void maybe_remove_section(struct config_store_data *store,\n }\n \n int git_config_set_in_file_gently(const char *config_filename,\n-\t\t\t\t const char *key, const char *value)\n+\t\t\t\t const char *key, const char *comment, const char *value)\n {\n-\treturn git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, 0);\n+\treturn git_config_set_multivar_in_file_gently(config_filename, key, value, NULL, comment, 0);\n }\n \n void git_config_set_in_file(const char *config_filename,\n@@ -3153,7 +3161,7 @@ int repo_config_set_worktree_gently(struct repository *r,\n \tif (r->repository_format_worktree_config) {\n \t\tchar *file = repo_git_path(r, \"config.worktree\");\n \t\tint ret = git_config_set_multivar_in_file_gently(\n-\t\t\t\t\tfile, key, value, NULL, 0);\n+\t\t\t\t\tfile, key, value, NULL, NULL, 0);\n \t\tfree(file);\n \t\treturn ret;\n \t}\n@@ -3195,6 +3203,7 @@ void git_config_set(const char *key, const char *value)\n int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\t\t\t const char *key, const char *value,\n \t\t\t\t\t const char *value_pattern,\n+\t\t\t\t\t const char *comment,\n \t\t\t\t\t unsigned flags)\n {\n \tint fd = -1, in_fd = -1;\n@@ -3245,7 +3254,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\tfree(store.key);\n \t\tstore.key = xstrdup(key);\n \t\tif (write_section(fd, key, &store) < 0 ||\n-\t\t write_pair(fd, key, value, &store) < 0)\n+\t\t write_pair(fd, key, value, comment, &store) < 0)\n \t\t\tgoto write_err_out;\n \t} else {\n \t\tstruct stat st;\n@@ -3399,7 +3408,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \t\t\t\tif (write_section(fd, key, &store) < 0)\n \t\t\t\t\tgoto write_err_out;\n \t\t\t}\n-\t\t\tif (write_pair(fd, key, value, &store) < 0)\n+\t\t\tif (write_pair(fd, key, value, comment, &store) < 0)\n \t\t\t\tgoto write_err_out;\n \t\t}\n \n@@ -3444,7 +3453,7 @@ void git_config_set_multivar_in_file(const char *config_filename,\n \t\t\t\t const char *value_pattern, unsigned flags)\n {\n \tif (!git_config_set_multivar_in_file_gently(config_filename, key, value,\n-\t\t\t\t\t\t value_pattern, flags))\n+\t\t\t\t\t\t value_pattern, NULL, flags))\n \t\treturn;\n \tif (value)\n \t\tdie(_(\"could not set '%s' to '%s'\"), key, value);\n@@ -3467,7 +3476,7 @@ int repo_config_set_multivar_gently(struct repository *r, const char *key,\n \tint res = git_config_set_multivar_in_file_gently(file,\n \t\t\t\t\t\t\t key, value,\n \t\t\t\t\t\t\t value_pattern,\n-\t\t\t\t\t\t\t flags);\n+\t\t\t\t\t\t\t NULL, flags);\n \tfree(file);\n \treturn res;\n }\ndiff --git a/config.h b/config.h\nindex 5dba984f770..a85a8271696 100644\n--- a/config.h\n+++ b/config.h\n@@ -290,7 +290,7 @@ int git_config_pathname(const char **, const char *, const char *);\n \n int git_config_expiry_date(timestamp_t *, const char *, const char *);\n int git_config_color(char *, const char *, const char *);\n-int git_config_set_in_file_gently(const char *, const char *, const char *);\n+int git_config_set_in_file_gently(const char *, const char *, const char *, const char *);\n \n /**\n * write config values to a specific config file, takes a key/value pair as\n@@ -336,7 +336,7 @@ int git_config_parse_key(const char *, char **, size_t *);\n int git_config_set_multivar_gently(const char *, const char *, const char *, unsigned);\n void git_config_set_multivar(const char *, const char *, const char *, unsigned);\n int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);\n-int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, unsigned);\n+int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, const char *, unsigned);\n \n /**\n * takes four parameters:\ndiff --git a/sequencer.c b/sequencer.c\nindex f49a871ac06..4c91ca5a844 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3460,54 +3460,54 @@ static int save_opts(struct replay_opts *opts)\n \n \tif (opts->no_commit)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.no-commit\", \"true\");\n+\t\t\t\t\t\"options.no-commit\", NULL, \"true\");\n \tif (opts->edit >= 0)\n-\t\tres |= git_config_set_in_file_gently(opts_file, \"options.edit\",\n+\t\tres |= git_config_set_in_file_gently(opts_file, \"options.edit\", NULL,\n \t\t\t\t\t\t opts->edit ? \"true\" : \"false\");\n \tif (opts->allow_empty)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.allow-empty\", \"true\");\n+\t\t\t\t\t\"options.allow-empty\", NULL, \"true\");\n \tif (opts->allow_empty_message)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\"options.allow-empty-message\", \"true\");\n+\t\t\t\t\"options.allow-empty-message\", NULL, \"true\");\n \tif (opts->keep_redundant_commits)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\"options.keep-redundant-commits\", \"true\");\n+\t\t\t\t\"options.keep-redundant-commits\", NULL, \"true\");\n \tif (opts->signoff)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.signoff\", \"true\");\n+\t\t\t\t\t\"options.signoff\", NULL, \"true\");\n \tif (opts->record_origin)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.record-origin\", \"true\");\n+\t\t\t\t\t\"options.record-origin\", NULL, \"true\");\n \tif (opts->allow_ff)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.allow-ff\", \"true\");\n+\t\t\t\t\t\"options.allow-ff\", NULL, \"true\");\n \tif (opts->mainline) {\n \t\tstruct strbuf buf = STRBUF_INIT;\n \t\tstrbuf_addf(&buf, \"%d\", opts->mainline);\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.mainline\", buf.buf);\n+\t\t\t\t\t\"options.mainline\", NULL, buf.buf);\n \t\tstrbuf_release(&buf);\n \t}\n \tif (opts->strategy)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.strategy\", opts->strategy);\n+\t\t\t\t\t\"options.strategy\", NULL, opts->strategy);\n \tif (opts->gpg_sign)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\t\"options.gpg-sign\", opts->gpg_sign);\n+\t\t\t\t\t\"options.gpg-sign\", NULL, opts->gpg_sign);\n \tfor (size_t i = 0; i < opts->xopts.nr; i++)\n \t\tres |= git_config_set_multivar_in_file_gently(opts_file,\n \t\t\t\t\"options.strategy-option\",\n-\t\t\t\topts->xopts.v[i], \"^$\", 0);\n+\t\t\t\topts->xopts.v[i], \"^$\", NULL, 0);\n \tif (opts->allow_rerere_auto)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\"options.allow-rerere-auto\",\n+\t\t\t\t\"options.allow-rerere-auto\", NULL,\n \t\t\t\topts->allow_rerere_auto == RERERE_AUTOUPDATE ?\n \t\t\t\t\"true\" : \"false\");\n \n \tif (opts->explicit_cleanup)\n \t\tres |= git_config_set_in_file_gently(opts_file,\n-\t\t\t\t\"options.default-msg-cleanup\",\n+\t\t\t\t\"options.default-msg-cleanup\", NULL,\n \t\t\t\tdescribe_cleanup_mode(opts->default_msg_cleanup));\n \treturn res;\n }\ndiff --git a/submodule-config.c b/submodule-config.c\nindex 54130f6a385..11428b4adad 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -978,7 +978,7 @@ int config_set_in_gitmodules_file_gently(const char *key, const char *value)\n {\n \tint ret;\n \n-\tret = git_config_set_in_file_gently(GITMODULES_FILE, key, value);\n+\tret = git_config_set_in_file_gently(GITMODULES_FILE, key, NULL, value);\n \tif (ret < 0)\n \t\t/* Maybe the user already did that, don't error out here */\n \t\twarning(_(\"Could not update .gitmodules entry %s\"), key);\ndiff --git a/submodule.c b/submodule.c\nindex 213da79f661..86630932d09 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -2052,7 +2052,7 @@ void submodule_unset_core_worktree(const struct submodule *sub)\n \tsubmodule_name_to_gitdir(&config_path, the_repository, sub->name);\n \tstrbuf_addstr(&config_path, \"/config\");\n \n-\tif (git_config_set_in_file_gently(config_path.buf, \"core.worktree\", NULL))\n+\tif (git_config_set_in_file_gently(config_path.buf, \"core.worktree\", NULL, NULL))\n \t\twarning(_(\"Could not unset core.worktree setting in submodule '%s'\"),\n \t\t\t sub->path);\n \ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex 31c38786870..ac7b02e6b07 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -69,13 +69,24 @@ test_expect_success 'replace with non-match (actually matching)' '\n \n cat > expect << EOF\n [section]\n-\tpenguin = very blue\n \tMovie = BadPhysics\n \tUPPERCASE = true\n-\tpenguin = kingpin\n+\tpenguin = gentoo #Pygoscelis papua\n+\tdisposition = peckish #find fish\n+\tfoo = bar ## abc\n [Sections]\n \tWhatEver = Second\n EOF\n+test_expect_success 'append comments' '\n+\tgit config --replace-all --comment=\"Pygoscelis papua\" section.penguin gentoo &&\n+\tgit config --comment=\"find fish\" section.disposition peckish &&\n+\tgit config --comment=\"# abc\" section.foo bar &&\n+\ttest_cmp expect .git/config\n+'\n+\n+test_expect_success 'Prohibited LF in comment' '\n+\ttest_must_fail git config --comment=\"a${LF}b\" section.k v\n+'\n \n test_expect_success 'non-match result' 'test_cmp expect .git/config'\n \ndiff --git a/worktree.c b/worktree.c\nindex b02a05a74a3..cf5eea8c931 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -807,9 +807,9 @@ int should_prune_worktree(const char *id, struct strbuf *reason, char **wtpath,\n static int move_config_setting(const char *key, const char *value,\n \t\t\t const char *from_file, const char *to_file)\n {\n-\tif (git_config_set_in_file_gently(to_file, key, value))\n+\tif (git_config_set_in_file_gently(to_file, key, NULL, value))\n \t\treturn error(_(\"unable to set %s in '%s'\"), key, to_file);\n-\tif (git_config_set_in_file_gently(from_file, key, NULL))\n+\tif (git_config_set_in_file_gently(from_file, key, NULL, NULL))\n \t\treturn error(_(\"unable to unset %s in '%s'\"), key, from_file);\n \treturn 0;\n }\n\nbase-commit: 0f9d4d28b7e6021b7e6db192b7bf47bd3a0d0d1d\n-- \ngitgitgadget\n"},{"id":"490741","messageId":"xmqq8r2jp2eq.fsf@gitster.g","threadId":"61043","inReplyTo":"pull.1681.v3.git.1710280020508.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] config: add --comment option to add a comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-15T22:15:41Z","receivedAt":"2024-03-15T22:18:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Ralph Seichter via GitGitGadget\" writes:\n\n> From: Ralph Seichter \n\nThanks for updating. The proposed commit log message reads much\nsmoother.\n\n> Users need to be able to distinguish between config entries made\n> using automation and entries made by a human. Automation can add\n> comments containing a URL pointing to explanations for the change\n> made, avoiding questions from users as to why their config file\n> was changed by a third party.\n\nWhile I can understand what you want to say, primarily because we\nwere both on this topic for some time by now, a casual reader of the\nabove would say:\n\n Sure, but a human also would add comment to clarify what\n motivated the setting to be added. The fact that one entry has\n comment does not help distinguishing between automation and\n human input all that much.\n\nMore importantly, the reason why users would want to mark entries is\nnot limited to telling automation vs human. So, it seems to me that\nthe paragraph needs a bit more work. Here is my attempt.\n\n While the subcommands (including \"git config\") of \"git\" all can\n skip comment strings in the configuration files, and users do\n write comments in the configuration files with editors, there is\n no easy way that can be used by scripts and automation to leave\n comments when they add/modify configuration entries.\n\n Introduce a \"--comment\" option that can be used with the \"git\n config\" command when it is used for writing, so that each\n affected entry can be annotated with a comment that comes after\n the \"variable = value\" on the same line. The option argument is\n deliberately limited to a single-line comment to match the line\n oriented nature of the configuration file, and \"git config\" will\n error out early when fed a multi-line string. This way, removal\n of the variable will also remove the comment given to it by this\n mechanism.\n\n> The implementation ensures that a # character is unconditionally\n> prepended to the provided comment string, and that\n\nI thought we already discussed this and unconditional \"#comment\" has\nalready been declared a non starter. I'll follow this review\nresponse with a few patches that are designed to apply on top to\nshow a more ergonomic way to do this.\n\n> the comment\n> text is appended as a suffix to the changed key-value-pair in the\n> same line of text. Multi-line comments (i.e. comments containing\n> linefeed) are rejected as errors, causing Git to exit without\n> making changes.\n\nThese will become redundant if we rewrite the earlier paragraph like\nillustrated above.\n\n> Comments are aimed at humans who inspect or change their Git\n> config using a pager or editor. Comments are not meant to be\n> read or displayed by git-config at a later time.\n\nAs we sold why comments are useful and lack of mechanical way to add\none hurts earlier in the proposed log message, if we rewrite the\nearlier paragraph like illustrated above, this would become\nredundant. I also do not think we need to limit future direction\nwith the last sentence. I do not foresee a reason to forbid other\ndevelopers from adding an option to the \"get\" subcommand, e.g.\n\n\t$ git config --get --with-comments vari.able\n\tvari.able = value # added on Fri Mar 15 15:02:37 2024\n\n> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\n> index dff39093b5e..e608d5ffef2 100644\n> --- a/Documentation/git-config.txt\n> +++ b/Documentation/git-config.txt\n> @@ -9,9 +9,9 @@ git-config - Get and set repository or global options\n> SYNOPSIS\n> --------\n> [verse]\n> -'git config' [] [--type=] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] [ []]\n> -'git config' [] [--type=] --add \n> -'git config' [] [--type=] [--fixed-value] --replace-all []\n> +'git config' [] [--type=] [--comment=] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] [ []]\n> +'git config' [] [--type=] [--comment=] --add \n> +'git config' [] [--type=] [--comment=] [--fixed-value] --replace-all []\n> 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get []\n> 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all []\n> 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp []\n\n\"--comment=\" will become awkward when we write about how the\ncomment message is placed after the value in the description.\nYou'll see, in the patch I promised to follow this review with,\nI've used --comment= instead to avoid that issue.\n\n> @@ -87,6 +87,11 @@ OPTIONS\n> \tvalues. This is the same as providing '^$' as the `value-pattern`\n> \tin `--replace-all`.\n> \n> +--comment ::\n> +\tAppend a comment to new or modified lines. A '#' character will be\n> +\tunconditionally prepended to the value. The value must not contain\n> +\tlinefeed characters (no multi-line comments are permitted).\n\nAbout the \"unconditional\" part I won't repeat. As to the formatting\nstyle, the recent trend is to write placeholder emphasized and\ninside angle bracket, e.g.\n\n\t--comment ::\n\t\tAppend a comment __ after the value on the\n\t\tsame line. ...\n\nThis illustration is only to show the formatting, and not what the\ndescription says.\n\n> [section]\n> -\tpenguin = very blue\n> \tMovie = BadPhysics\n> \tUPPERCASE = true\n> -\tpenguin = kingpin\n> +\tpenguin = gentoo #Pygoscelis papua\n> +\tdisposition = peckish #find fish\n> +\tfoo = bar ## abc\n\nThe first one, \"very blue\", will be gone, which I found a bit\nsurprising, but it is OK. This is because \"--replace-all\" used\nfirst will eat all the \"penguin\" and leaves only a single one.\n\n> [Sections]\n> \tWhatEver = Second\n> EOF\n> +test_expect_success 'append comments' '\n> +\tgit config --replace-all --comment=\"Pygoscelis papua\" section.penguin gentoo &&\n> +\tgit config --comment=\"find fish\" section.disposition peckish &&\n> +\tgit config --comment=\"# abc\" section.foo bar &&\n> +\ttest_cmp expect .git/config\n> +'\n\nNotice that none of these would produce the most natural (at least\nin the context of Git command set)\n\n\tvariable = value # comment\n\nYou would have to say --comment=\" comment\" which is rather\nunfortunate.\n\n-------- >8 --------------- >8 --------------- >8 --------------- >8 --------\nFrom: Junio C Hamano \nDate: Fri, 15 Mar 2024 12:43:58 -0700\nSubject: [PATCH] config: fix --comment formatting\n\nWhen git adds comments itself (like \"rebase -i\" todo list and\n\"commit -e\" log message editor), it always gives a comment\nintroducer \"#\" followed by a Space before the message, except for\nthe recently introduced \"git config --comment\", where the users are\nforced to say \" this is my comment\" if they want to add their\ncomment in this usual format; otherwise their comment string will\nend up without a space after the \"#\".\n\nMake it more ergonomic, while keeping it possible to also use this\nunusual style, by massaging the comment string at the UI layer with\na set of simple rules:\n\n * If the given comment string begins with '#', it is passed intact.\n * Otherwise, \"# \" is prefixed.\n * A string with LF in it cannot be used as a comment string.\n\nRight now there is only one \"front-end\" that accepts end-user\ncomment string and calls the underlying machinery to add or modify\nconfiguration file with comments, but to make sure that the future\ncallers perform similar massaging as they see fit, add a sanity\ncheck logic in git_config_set_multivar_in_file_gently(), which is\nthe single choke point in the codepaths that consumes the comment\nstring.\n\nSigned-off-by: Junio C Hamano \n---\n\nEven with this fix, the code unconditionally places a single Space\ncharacter after the value before the '#' comment introducer. If we\nwanted to allow it to be replaced with other kind of whitespaces,\nwe could tweak the massaging logic further.\n\nThe updated tweaking rule may become\n\n * If the given comment string begins with one or more whitespace\n characters followed by '#', it is passed intact.\n * If the given comment string begins with '#', then a SP is\n prefixed.\n * Otherwise, \" # \" (Space, '#', Space) is prefixed.\n * A string with LF in it cannot be used as a comment string.\n\nAnd the sanity checking rule may become\n\n * comment string after the above massaging that comes into\n git_config_set_multivar_in_file_gently() must\n\n - begin with one or more whitespace characters followed by '#'.\n - not have a LF in it.\n\nFinally the code to add the massaged comment will simply become:\n\n\tstrbuf_addf(&sb, \"%s%s\\n\", quote, comment);\n\nI personally think that is over-engineered, so this commit stops\nshort of doing any of that. If we were to do so, the tweaking logic\nmust be encapsulated into a helper function, so that the second and\nsubsequent \"front-end\" can reuse it.\n\n Documentation/git-config.txt | 15 ++++++++-------\n builtin/config.c | 15 +++++++++++----\n config.c | 20 ++++++++++++++------\n t/t1300-config.sh | 9 +++++----\n 4 files changed, 38 insertions(+), 21 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex e608d5ffef..af374ee2e0 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -9,9 +9,9 @@ git-config - Get and set repository or global options\n SYNOPSIS\n --------\n [verse]\n-'git config' [] [--type=] [--comment=] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] [ []]\n-'git config' [] [--type=] [--comment=] --add \n-'git config' [] [--type=] [--comment=] [--fixed-value] --replace-all []\n+'git config' [] [--type=] [--comment=] [--fixed-value] [--show-origin] [--show-scope] [-z|--null] [ []]\n+'git config' [] [--type=] [--comment=] --add \n+'git config' [] [--type=] [--comment=] [--fixed-value] --replace-all []\n 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get []\n 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] --get-all []\n 'git config' [] [--type=] [--show-origin] [--show-scope] [-z|--null] [--fixed-value] [--name-only] --get-regexp []\n@@ -87,10 +87,11 @@ OPTIONS\n \tvalues. This is the same as providing '^$' as the `value-pattern`\n \tin `--replace-all`.\n \n---comment ::\n-\tAppend a comment to new or modified lines. A '#' character will be\n-\tunconditionally prepended to the value. The value must not contain\n-\tlinefeed characters (no multi-line comments are permitted).\n+--comment ::\n+\tAppend a comment at the end of new or modified lines.\n+\tUnless __ begins with \"#\", a string \"# \" (hash\n+\tfollowed by a space) is prepended to it. The __ must not\n+\tcontain linefeed characters (no multi-line comments are permitted).\n \n --get::\n \tGet the value for a given key (optionally filtered by a regex\ndiff --git a/builtin/config.c b/builtin/config.c\nindex c54e9941a5..e859a659f4 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -174,7 +174,7 @@ static struct option builtin_config_options[] = {\n \tOPT_BOOL(0, \"show-origin\", &show_origin, N_(\"show origin of config (file, standard input, blob, command line)\")),\n \tOPT_BOOL(0, \"show-scope\", &show_scope, N_(\"show scope of config (worktree, local, global, system, command)\")),\n \tOPT_STRING(0, \"default\", &default_value, N_(\"value\"), N_(\"with --get, use default value when missing entry\")),\n-\tOPT_STRING(0, \"comment\", &comment, N_(\"value\"), N_(\"human-readable comment string (# will be prepended automatically)\")),\n+\tOPT_STRING(0, \"comment\", &comment, N_(\"value\"), N_(\"human-readable comment string (# will be prepended as needed)\")),\n \tOPT_END(),\n };\n \n@@ -800,9 +800,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (comment &&\n-\t\t!(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) {\n-\t\t\terror(_(\"--comment is only applicable to add/set/replace operations\"));\n-\t\t\tusage_builtin_config();\n+\t !(actions & (ACTION_ADD|ACTION_SET|ACTION_SET_ALL|ACTION_REPLACE_ALL))) {\n+\t\terror(_(\"--comment is only applicable to add/set/replace operations\"));\n+\t\tusage_builtin_config();\n \t}\n \n \t/* check usage of --fixed-value */\n@@ -841,6 +841,13 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tflags |= CONFIG_FLAGS_FIXED_VALUE;\n \t}\n \n+\tif (comment) {\n+\t\tif (strchr(comment, '\\n'))\n+\t\t\tdie(_(\"no multi-line comment allowed: '%s'\"), comment);\n+\t\tif (comment[0] != '#')\n+\t\t\tcomment = xstrfmt(\"# %s\", comment);\n+\t}\n+\n \tif (actions & PAGING_ACTIONS)\n \t\tsetup_auto_pager(\"config\", 1);\n \ndiff --git a/config.c b/config.c\nindex 2f3f6d123c..15019cb9a5 100644\n--- a/config.c\n+++ b/config.c\n@@ -3043,12 +3043,9 @@ static ssize_t write_pair(int fd, const char *key, const char *value,\n \t\t\tbreak;\n \t\t}\n \n-\tif (comment) {\n-\t\tif (strchr(comment, '\\n'))\n-\t\t\tdie(_(\"multi-line comments are not permitted: '%s'\"), comment);\n-\t\telse\n-\t\t\tstrbuf_addf(&sb, \"%s #%s\\n\", quote, comment);\n-\t} else\n+\tif (comment)\n+\t\tstrbuf_addf(&sb, \"%s %s\\n\", quote, comment);\n+\telse\n \t\tstrbuf_addf(&sb, \"%s\\n\", quote);\n \n \tret = write_in_full(fd, sb.buf, sb.len);\n@@ -3214,6 +3211,17 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \tsize_t contents_sz;\n \tstruct config_store_data store = CONFIG_STORE_INIT;\n \n+\tif (comment) {\n+\t\t/*\n+\t\t * The front-end must have massaged the comment string\n+\t\t * properly before calling us.\n+\t\t */\n+\t\tif (strchr(comment, '\\n'))\n+\t\t\tBUG(\"multi-line comments are not permitted: '%s'\", comment);\n+\t\tif (comment[0] != '#')\n+\t\t\tBUG(\"comment should begin with '#': '%s'\", comment);\n+\t}\n+\n \t/* parse-key returns negative; flip the sign to feed exit(3) */\n \tret = 0 - git_config_parse_key(key, &store.key, &store.baselen);\n \tif (ret)\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex ac7b02e6b0..d5dfb45877 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -71,16 +71,17 @@ cat > expect << EOF\n [section]\n \tMovie = BadPhysics\n \tUPPERCASE = true\n-\tpenguin = gentoo #Pygoscelis papua\n-\tdisposition = peckish #find fish\n-\tfoo = bar ## abc\n+\tpenguin = gentoo # Pygoscelis papua\n+\tdisposition = peckish # find fish\n+\tfoo = bar #abc\n [Sections]\n \tWhatEver = Second\n EOF\n+\n test_expect_success 'append comments' '\n \tgit config --replace-all --comment=\"Pygoscelis papua\" section.penguin gentoo &&\n \tgit config --comment=\"find fish\" section.disposition peckish &&\n-\tgit config --comment=\"# abc\" section.foo bar &&\n+\tgit config --comment=\"#abc\" section.foo bar &&\n \ttest_cmp expect .git/config\n '\n \n-- \n2.44.0-248-g4f9b731bde\n\n"},{"id":"490743","messageId":"xmqq4jd7p1wf.fsf_-_@gitster.g","threadId":"61043","inReplyTo":"xmqq8r2jp2eq.fsf@gitster.g","subject":"[PATCH 3/1] config: allow tweaking whitespace between value and comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-15T22:26:40Z","receivedAt":"2024-03-15T22:26:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Extending the previous step, this allows the whitespace placed after\nthe value before the \"# comment message\" to be tweaked by tweaking\nthe preprocessing rule to:\n\n * If the given comment string begins with one or more whitespace\n characters followed by '#', it is passed intact.\n\n * If the given comment string begins with '#', a Space is\n prepended.\n\n * Otherwise, \" # \" (Space, '#', Space) is prefixed.\n\n * A string with LF in it cannot be used as a comment string.\n\nUnlike the previous step, which unconditionally added a space after\nthe value before writing the \"# comment string\", because the above\npreprocessing already gives a whitespace before the '#', the\nresulting string is written immediately after copying the value.\n\nAnd the sanity checking rule becomes\n\n * comment string after the above massaging that comes into\n git_config_set_multivar_in_file_gently() must\n\n - begin with zero or more whitespace characters followed by '#'.\n - not have a LF in it.\n\nI personally think this is over-engineered, but since I thought\nthings through anyway, here it is in the patch form. The logic to\ntweak end-user supplied comment string is encapsulated in a new\nhelper function, git_config_prepare_comment_string(), so if new\nfront-end callers would want to use the same massaging rules, it is\neasily reused.\n\nUnfortunately I do not think of a way to tweak the preprocessing\nrules further to optionally allow having no blank after the value,\ni.e. to produce\n\n\t[section]\n\t\tvariable = value#comment\n\n(which is a valid way to say section.variable=value, by the way)\nwithout sacrificing the ergonomics for the more usual case, so this\ntime I really stop here.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This is [3/1] as the one attached to my review of the single\n patch is [2/1].\n\n Documentation/git-config.txt | 12 +++++--\n builtin/config.c | 7 +---\n config.c | 69 ++++++++++++++++++++++++++++++------\n config.h | 2 ++\n t/t1300-config.sh | 6 ++++\n 5 files changed, 76 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/git-config.txt b/Documentation/git-config.txt\nindex af374ee2e0..e4f2e07926 100644\n--- a/Documentation/git-config.txt\n+++ b/Documentation/git-config.txt\n@@ -89,9 +89,15 @@ OPTIONS\n \n --comment ::\n \tAppend a comment at the end of new or modified lines.\n-\tUnless __ begins with \"#\", a string \"# \" (hash\n-\tfollowed by a space) is prepended to it. The __ must not\n-\tcontain linefeed characters (no multi-line comments are permitted).\n+\n+\tIf __ begins with one or more whitespaces followed\n+\tby \"#\", it is used as-is. If it begins with \"#\", a space is\n+\tprepended before it is used. Otherwise, a string \" # \" (a\n+\tspace followed by a hash followed by a space) is prepended\n+\tto it. And the resulting string is placed immediately after\n+\tthe value defined for the variable. The __ must\n+\tnot contain linefeed characters (no multi-line comments are\n+\tpermitted).\n \n --get::\n \tGet the value for a given key (optionally filtered by a regex\ndiff --git a/builtin/config.c b/builtin/config.c\nindex e859a659f4..0015620dde 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -841,12 +841,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tflags |= CONFIG_FLAGS_FIXED_VALUE;\n \t}\n \n-\tif (comment) {\n-\t\tif (strchr(comment, '\\n'))\n-\t\t\tdie(_(\"no multi-line comment allowed: '%s'\"), comment);\n-\t\tif (comment[0] != '#')\n-\t\t\tcomment = xstrfmt(\"# %s\", comment);\n-\t}\n+\tcomment = git_config_prepare_comment_string(comment);\n \n \tif (actions & PAGING_ACTIONS)\n \t\tsetup_auto_pager(\"config\", 1);\ndiff --git a/config.c b/config.c\nindex 15019cb9a5..f1d4263a68 100644\n--- a/config.c\n+++ b/config.c\n@@ -3044,7 +3044,7 @@ static ssize_t write_pair(int fd, const char *key, const char *value,\n \t\t}\n \n \tif (comment)\n-\t\tstrbuf_addf(&sb, \"%s %s\\n\", quote, comment);\n+\t\tstrbuf_addf(&sb, \"%s%s\\n\", quote, comment);\n \telse\n \t\tstrbuf_addf(&sb, \"%s\\n\", quote);\n \n@@ -3172,6 +3172,62 @@ void git_config_set(const char *key, const char *value)\n \ttrace2_cmd_set_config(key, value);\n }\n \n+/*\n+ * The ownership rule is that the caller will own the string\n+ * if it receives a piece of memory different from what it passed\n+ * as the parameter.\n+ */\n+const char *git_config_prepare_comment_string(const char *comment)\n+{\n+\tsize_t leading_blanks;\n+\n+\tif (!comment)\n+\t\treturn NULL;\n+\n+\tif (strchr(comment, '\\n'))\n+\t\tdie(_(\"no multi-line comment allowed: '%s'\"), comment);\n+\n+\t/*\n+\t * If it begins with one or more leading whitespace characters\n+\t * followed by '#\", the comment string is used as-is.\n+\t *\n+\t * If it begins with '#', a SP is inserted between the comment\n+\t * and the value the comment is about.\n+\t *\n+\t * Otherwise, the value is followed by a SP followed by '#'\n+\t * followed by SP and then the comment string comes.\n+\t */\n+\n+\tleading_blanks = strspn(comment, \" \\t\");\n+\tif (leading_blanks && comment[leading_blanks] == '#')\n+\t\t; /* use it as-is */\n+\telse if (comment[0] == '#')\n+\t\tcomment = xstrfmt(\" %s\", comment);\n+\telse\n+\t\tcomment = xstrfmt(\" # %s\", comment);\n+\n+\treturn comment;\n+}\n+\n+static void validate_comment_string(const char *comment)\n+{\n+\tsize_t leading_blanks;\n+\n+\tif (!comment)\n+\t\treturn;\n+\t/*\n+\t * The front-end must have massaged the comment string\n+\t * properly before calling us.\n+\t */\n+\tif (strchr(comment, '\\n'))\n+\t\tBUG(\"multi-line comments are not permitted: '%s'\", comment);\n+\n+\tleading_blanks = strspn(comment, \" \\t\");\n+\tif (!leading_blanks || comment[leading_blanks] != '#')\n+\t\tBUG(\"comment must begin with one or more SP followed by '#': '%s'\",\n+\t\t comment);\n+}\n+\n /*\n * If value==NULL, unset in (remove from) config,\n * if value_pattern!=NULL, disregard key/value pairs where value does not match.\n@@ -3211,16 +3267,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,\n \tsize_t contents_sz;\n \tstruct config_store_data store = CONFIG_STORE_INIT;\n \n-\tif (comment) {\n-\t\t/*\n-\t\t * The front-end must have massaged the comment string\n-\t\t * properly before calling us.\n-\t\t */\n-\t\tif (strchr(comment, '\\n'))\n-\t\t\tBUG(\"multi-line comments are not permitted: '%s'\", comment);\n-\t\tif (comment[0] != '#')\n-\t\t\tBUG(\"comment should begin with '#': '%s'\", comment);\n-\t}\n+\tvalidate_comment_string(comment);\n \n \t/* parse-key returns negative; flip the sign to feed exit(3) */\n \tret = 0 - git_config_parse_key(key, &store.key, &store.baselen);\ndiff --git a/config.h b/config.h\nindex a85a827169..f4966e3749 100644\n--- a/config.h\n+++ b/config.h\n@@ -338,6 +338,8 @@ void git_config_set_multivar(const char *, const char *, const char *, unsigned)\n int repo_config_set_multivar_gently(struct repository *, const char *, const char *, const char *, unsigned);\n int git_config_set_multivar_in_file_gently(const char *, const char *, const char *, const char *, const char *, unsigned);\n \n+const char *git_config_prepare_comment_string(const char *);\n+\n /**\n * takes four parameters:\n *\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex d5dfb45877..cc050b3c20 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -74,6 +74,8 @@ cat > expect << EOF\n \tpenguin = gentoo # Pygoscelis papua\n \tdisposition = peckish # find fish\n \tfoo = bar #abc\n+\tspsp = value # and comment\n+\thtsp = value\t# and comment\n [Sections]\n \tWhatEver = Second\n EOF\n@@ -82,6 +84,10 @@ test_expect_success 'append comments' '\n \tgit config --replace-all --comment=\"Pygoscelis papua\" section.penguin gentoo &&\n \tgit config --comment=\"find fish\" section.disposition peckish &&\n \tgit config --comment=\"#abc\" section.foo bar &&\n+\n+\tgit config --comment=\"and comment\" section.spsp value &&\n+\tgit config --comment=\"\t# and comment\" section.htsp value &&\n+\n \ttest_cmp expect .git/config\n '\n \n-- \n2.44.0-248-g4f9b731bde\n\n"},{"id":"490746","messageId":"CAPig+cQwWu=FA4vnKK9+aLkTzAzMXp6h5aJedEh3FT+1bNTjAQ@mail.gmail.com","threadId":"61043","inReplyTo":"xmqq8r2jp2eq.fsf@gitster.g","subject":"Re: [PATCH v3] config: add --comment option to add a comment","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-03-15T23:10:37Z","receivedAt":"2024-03-15T23:10:49Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 15, 2024 at 6:19 PM Junio C Hamano wrote:\n> ---comment ::\n> - Append a comment to new or modified lines. A '#' character will be\n> - unconditionally prepended to the value. The value must not contain\n> - linefeed characters (no multi-line comments are permitted).\n> +--comment ::\n> + Append a comment at the end of new or modified lines.\n> + Unless __ begins with \"#\", a string \"# \" (hash\n> + followed by a space) is prepended to it. The __ must not\n> + contain linefeed characters (no multi-line comments are permitted).\n\nIn an earlier review, you wrote about the potential difficulties and\nissues surrounding comments other than the simple \"inline\" comments\ntackled by this patch. Do we foresee a future in which git-config may\nbe extended to handle automation of comments other than inline? If so,\ndo we really want to lock ourselves into having the generic option\n`--comment=` be restricted to only inline comments? Or would\nit be better to plan for the future by instead having some sort of\nannotation which indicates that we are requesting (or dealing with)\nonly inline comments, such as `--inline-comment=` or\n`--comment=inline:`?\n"},{"id":"490747","messageId":"xmqqttl7njv2.fsf@gitster.g","threadId":"61043","inReplyTo":"CAPig+cQwWu=FA4vnKK9+aLkTzAzMXp6h5aJedEh3FT+1bNTjAQ@mail.gmail.com","subject":"Re: [PATCH v3] config: add --comment option to add a comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-15T23:41:37Z","receivedAt":"2024-03-15T23:41:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> tackled by this patch. Do we foresee a future in which git-config may\n> be extended to handle automation of comments other than inline? If so,\n> do we really want to lock ourselves into having the generic option\n> `--comment=` be restricted to only inline comments? Or would\n> it be better to plan for the future by instead having some sort of\n> annotation which indicates that we are requesting (or dealing with)\n> only inline comments, such as `--inline-comment=` or\n> `--comment=inline:`?\n\nI do not see a need for anything, while we fail a request with a\nmulti-line message and say \"don't feed me a multi-line message\".\n\nWhen we start supporting multi-line comment (which I do not think is\na good idea at all, by the way), the code can switch between\n\n\t[vari]\n\t\table = value # single line comment\n\t\t# a comment with\n\t\t# more than one line\n\t\table = value\n\ndepending on what is in , so \"inline:\" prefix does not even\nhave to exist, I would imagine. Of course you could even go fancier\nand allow something like this with unnecessary tailing LF ...\n\n\t$ git config --comment='another single line comment\n\t'\n\n... as a signal to turn a single line commet on a separate line.\n\n\t[vari]\n\t\table = value # single line comment\n\t\t# a comment with\n\t\t# more than one line\n\t\table = value\n\t\t# another single value comment\n\t\table = value\n\nThere will most likely need where e.g., above or below, around the\n\"var = val\" line to place the comment as well, so I do not see much\nvalue in investing more brain cycles on what the \"--comment\" option\nshould look like, while we only support single-liners and explicitly\nreject multi-line messages.\n"},{"id":"490748","messageId":"xmqqplvvnjpz.fsf@gitster.g","threadId":"61043","inReplyTo":"xmqqttl7njv2.fsf@gitster.g","subject":"Re: [PATCH v3] config: add --comment option to add a comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-15T23:44:40Z","receivedAt":"2024-03-15T23:44:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> There will most likely need where e.g., above or below, around the\n\n\"need\" -> \"need for adding a new option to specify\".\n\nIn sort, we would need to have more than just \"use this message\"\nanyway, and cramming everything into the single \"--comment\" option,\neven if it were feasible, would not be a good idea.\n\n> \"var = val\" line to place the comment as well, so I do not see much\n> value in investing more brain cycles on what the \"--comment\" option\n> should look like, while we only support single-liners and explicitly\n> reject multi-line messages.\n"},{"id":"491614","messageId":"xmqqcyrg7imy.fsf@gitster.g","threadId":"61043","inReplyTo":"xmqq4jd7p1wf.fsf_-_@gitster.g","subject":"Re: [PATCH 3/1] config: allow tweaking whitespace between value and comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-26T22:06:29Z","receivedAt":"2024-03-26T22:06:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Extending the previous step, this allows ...\n\nIf I am not mistaken, this topic, originating at\n\nhttps://lore.kernel.org/git/pull.1681.v3.git.1710280020508.gitgitgadget@gmail.com/\n\nis expecting responses from you. Can you unblock it to let us move\nforward?\n\nThanks.\n"},{"id":"491619","messageId":"874jcsvccq.fsf@ra.horus-it.com","threadId":"61043","inReplyTo":"xmqqcyrg7imy.fsf@gitster.g","subject":"Re: [PATCH 3/1] config: allow tweaking whitespace between value and comment","fromName":"Ralph Seichter","fromEmail":"github@seichter.de","sentAt":"2024-03-26T22:48:21Z","receivedAt":"2024-03-26T22:55:50Z","isPatch":true,"sender":{"key":"github@seichter.de","avatar":"https://avatars.githubusercontent.com/u/30873939?v=4"},"body":"* Junio C. Hamano:\n\n> Can you unblock [this topic] to let us move forward?\n\nI don't see any obvious way to alter the pull request's state, or where\na response of mine might be inserted to move things along. I also have\nnot found anything related in the GitGitGadget docs. Thus, I'll try to\nissue another /submit command in the hope that this might affect the\nprocess.\n\n-Ralph\n"},{"id":"491620","messageId":"xmqq4jcs7evl.fsf@gitster.g","threadId":"61043","inReplyTo":"874jcsvccq.fsf@ra.horus-it.com","subject":"Re: [PATCH 3/1] config: allow tweaking whitespace between value and comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-26T23:27:42Z","receivedAt":"2024-03-26T23:27:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ralph Seichter writes:\n\n>> Can you unblock [this topic] to let us move forward?\n>\n> I don't see any obvious way to alter the pull request's state, or where\n> a response of mine might be inserted to move things along.\n\nThe easiest would have been for you to say \"Yup, the two additional\npatches queued on top of mine seem to make it better. Let me review\nthem in detail\", followed by \"Yeah, they looked good to me\", and\nafter that we can just merge the topic with three patches down to\n'next' and later to 'master'.\n\nOf course, if you do not agree with the two follow-up patches, you\ncan point out issues in them and argue why they are bad idea. That\nwould take more cycles, of course.\n"},{"id":"491624","messageId":"871q7wv7s1.fsf@ra.horus-it.com","threadId":"61043","inReplyTo":"xmqq4jcs7evl.fsf@gitster.g","subject":"Re: [PATCH 3/1] config: allow tweaking whitespace between value and comment","fromName":"Ralph Seichter","fromEmail":"github@seichter.de","sentAt":"2024-03-27T00:27:10Z","receivedAt":"2024-03-27T00:27:16Z","isPatch":true,"sender":{"key":"github@seichter.de","avatar":"https://avatars.githubusercontent.com/u/30873939?v=4"},"body":"* Junio C. Hamano:\n\n> The easiest would have been for you to say \"Yup, the two additional\n> patches queued on top of mine seem to make it better. [...]\n\nWould it? Well, you wrote the following two weeks ago, among other\nthings:\n\n>> I thought we already discussed this and unconditional \"#comment\" has\n>> already been declared a non starter.\n\nThis unilateral decision of yours, and the following prolonged debate\nabout spaces/tabs (which I clearly stated I consider a waste of time)\nleft me with the impression that what I think doesn't matter much\nanyway. Also, it seems to me that this whole subject has already been\nblown far out of proportion. If this exercise leads to the feature I was\nproposing, you guys can do whatever, and I won't slow things down by\nexpressing my opinions, or by continuing to formulate my own commit\nmessages. So much time has already been spent.\n\n-Ralph\n"},{"id":"491626","messageId":"8d05e767aa5ba30e91522835fb2941b8@manjaro.org","threadId":"61043","inReplyTo":"871q7wv7s1.fsf@ra.horus-it.com","subject":"Re: [PATCH 3/1] config: allow tweaking whitespace between value and comment","fromName":"Dragan Simic","fromEmail":"dsimic@manjaro.org","sentAt":"2024-03-27T01:23:52Z","receivedAt":"2024-03-27T01:23:56Z","isPatch":true,"sender":{"key":"dsimic@manjaro.org","avatar":null},"body":"Hello Ralph,\n\nOn 2024-03-27 01:27, Ralph Seichter wrote:\n> * Junio C. Hamano:\n> \n>> The easiest would have been for you to say \"Yup, the two additional\n>> patches queued on top of mine seem to make it better. [...]\n> \n> Would it? Well, you wrote the following two weeks ago, among other\n> things:\n> \n>>> I thought we already discussed this and unconditional \"#comment\" has\n>>> already been declared a non starter.\n> \n> This unilateral decision of yours, and the following prolonged debate\n> about spaces/tabs (which I clearly stated I consider a waste of time)\n> left me with the impression that what I think doesn't matter much\n> anyway. Also, it seems to me that this whole subject has already been\n> blown far out of proportion. If this exercise leads to the feature I \n> was\n> proposing, you guys can do whatever, and I won't slow things down by\n> expressing my opinions, or by continuing to formulate my own commit\n> messages. So much time has already been spent.\n\nWell, is getting patches accepted to git hard and often challenging?\nAbsolutely. Do additional patches and discussions sprout all over the\nplace? They obviously do. But, does a high-profile open-source project\ndeserve such an approach? Without doubt.\n\nIn fact, pretty much every good software project should employ such an\napproach, but that unfortunately isn't always the case.\n\nThat's how I see it.\n"},{"id":"491706","messageId":"xmqqwmpn1t6t.fsf@gitster.g","threadId":"61043","inReplyTo":"871q7wv7s1.fsf@ra.horus-it.com","subject":"Re: [PATCH 3/1] config: allow tweaking whitespace between value and comment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-27T17:27:22Z","receivedAt":"2024-03-27T17:27:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ralph Seichter writes:\n\n>>> I thought we already discussed this and unconditional \"#comment\" has\n>>> already been declared a non starter.\n>\n> This unilateral decision of yours, and the following prolonged debate\n> about spaces/tabs (which I clearly stated I consider a waste of time)\n> left me with the impression that what I think doesn't matter much\n> anyway.\n\nYou can call it unilateral or whatever you like, but there are\nproject principles, e.g., \"try to keep things consistent\", and\n\"making unusual things possible is fine, but it shouldn't make the\ndefault thing harder\", and they are not negotiable.\n\nWhen it comes to quality of the end product, it is true that your\nconsidering it a waste of time does not matter. It is effort needed\nto polish your topic to an acceptable level. Either we do so with\nthe help of reviewer input, or alternatively we could drop the\ntopic.\n\nIn any case, I'll keep the three patches separate and mark the topic\nfor 'next'.\n\nThanks.\n"}]}