{"thread":{"id":"58518","subject":"[PATCH 1/5] config: relax requirements on multi-value return","startedAt":"2022-09-27T14:08:49Z","lastAt":"2022-09-28T20:07:58Z","messageCount":21,"participants":["Derrick Stolee via GitGitGadget","Ævar Arnfjörð Bjarmason","Derrick Stolee","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"463720","messageId":"b2558a737f31465b38ad361a412d923ef186a38a.1664287711.git.gitgitgadget@gmail.com","threadId":"58518","inReplyTo":"pull.1369.git.1664287711.gitgitgadget@gmail.com","subject":"[PATCH 1/5] config: relax requirements on multi-value return","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-27T14:08:27Z","receivedAt":"2022-09-27T14:08:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe git_configset_get_value() method has an assert() statement\nguaranteeing that the result from git_configset_get_value_multi() is\neither NULL or has at least one element. We want to change that return\nto provide an empty list instead of a NULL list, so change the earlier\n'return 1' condition to care about a NULL or empty list.\n\nSigned-off-by: Derrick Stolee \n---\n config.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex cbb5a3bab74..bf89afbdab0 100644\n--- a/config.c\n+++ b/config.c\n@@ -2407,9 +2407,8 @@ int git_configset_get_value(struct config_set *cs, const char *key, const char *\n \t */\n \tvalues = git_configset_get_value_multi(cs, key);\n \n-\tif (!values)\n+\tif (!values || !values->nr)\n \t\treturn 1;\n-\tassert(values->nr > 0);\n \t*value = values->items[values->nr - 1].string;\n \treturn 0;\n }\n-- \ngitgitgadget\n\n"},{"id":"463721","messageId":"pull.1369.git.1664287711.gitgitgadget@gmail.com","threadId":"58518","inReplyTo":null,"subject":"[PATCH 0/5] [RFC] config API: return empty list, not NULL","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-27T14:08:26Z","receivedAt":"2022-09-27T14:08:51Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"This work changes the behavior of asking for a multi-valued config key to\nreturn an empty list instead of a NULL value. This simplifies the handling\nof the result and is safer for development in the future.\n\nThis is based on v4 of my unregister series [1]\n\n[1]\nhttps://lore.kernel.org/git/pull.1358.v4.git.1664287021.gitgitgadget@gmail.com/\n\nThis idea came about due to a bug in the git maintenance unregister work\nwhere the result from git_config_get_value_multi() was sent directly to\nfor_each_string_list_item() without checking for a NULL value first.\n\nI'm sending this as an RFC mostly because I'm not 100% sure this shift is\nworth the refactoring pain and effort. I personally think getting an empty\nlist is a safer choice, but I can also understand if someone has a different\nopinion.\n\nThanks, -Stolee\n\nDerrick Stolee (5):\n config: relax requirements on multi-value return\n *: relax git_configset_get_value_multi result\n config: add BUG() statement instead of possible segfault\n config: return an empty list, not NULL\n *: expect a non-NULL list of config values\n\n builtin/for-each-repo.c | 8 --------\n builtin/gc.c | 20 ++++++++------------\n builtin/log.c | 10 ++++------\n builtin/pack-objects.c | 3 ---\n builtin/repack.c | 13 +++++--------\n builtin/submodule--helper.c | 4 ++--\n config.c | 15 ++++++++++-----\n config.h | 6 +++---\n pack-bitmap.c | 3 ---\n submodule.c | 2 +-\n t/helper/test-config.c | 4 ++--\n versioncmp.c | 8 ++++----\n 12 files changed, 39 insertions(+), 57 deletions(-)\n\n\nbase-commit: 73a262cdca46a45aeeda6f47ea3357aaeb937e7b\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1369%2Fderrickstolee%2Fconfig-empty-list-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1369/derrickstolee/config-empty-list-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1369\n-- \ngitgitgadget\n"},{"id":"463722","messageId":"f277a7a429db8f54fa06dd1965d62ec491e6d84b.1664287711.git.gitgitgadget@gmail.com","threadId":"58518","inReplyTo":"pull.1369.git.1664287711.gitgitgadget@gmail.com","subject":"[PATCH 3/5] config: add BUG() statement instead of possible segfault","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-27T14:08:29Z","receivedAt":"2022-09-27T14:08:54Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe git_die_config() method calls git_config_get_value_multi() but\nimmediately navigates to its first value without checking if the result\nis NULL or empty. Callers should only call git_die_config() if there is\nat least one value for the given 'key', but such a mistaken use might\nslip through. It would be better to show a BUG() statement than a\npossible segfault.\n\nSigned-off-by: Derrick Stolee \n---\n config.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex bf89afbdab0..0c41606c7d4 100644\n--- a/config.c\n+++ b/config.c\n@@ -2833,8 +2833,13 @@ void git_die_config(const char *key, const char *err, ...)\n \t\tva_end(params);\n \t}\n \tvalues = git_config_get_value_multi(key);\n-\tkv_info = values->items[values->nr - 1].util;\n-\tgit_die_config_linenr(key, kv_info->filename, kv_info->linenr);\n+\n+\tif (values && values->nr) {\n+\t\tkv_info = values->items[values->nr - 1].util;\n+\t\tgit_die_config_linenr(key, kv_info->filename, kv_info->linenr);\n+\t} else {\n+\t\tBUG(\"expected a non-empty list of values\");\n+\t}\n }\n \n /*\n-- \ngitgitgadget\n\n"},{"id":"463723","messageId":"396343ce7dd17f86bbbc66197c6f0b4012caf445.1664287711.git.gitgitgadget@gmail.com","threadId":"58518","inReplyTo":"pull.1369.git.1664287711.gitgitgadget@gmail.com","subject":"[PATCH 4/5] config: return an empty list, not NULL","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-27T14:08:30Z","receivedAt":"2022-09-27T14:08:57Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nFor the multi-valued config API methods, Git previously returned a NULL\nlist instead of an empty list. Previous changes adjusted all callers to\ninstead expect an empty, non-NULL list, making this a safe change.\n\nThe next change will remove the NULL checks from all callers.\n\nSigned-off-by: Derrick Stolee \n---\n config.c | 3 ++-\n config.h | 6 +++---\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 0c41606c7d4..2d4ca1ae6dc 100644\n--- a/config.c\n+++ b/config.c\n@@ -2415,8 +2415,9 @@ int git_configset_get_value(struct config_set *cs, const char *key, const char *\n \n const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)\n {\n+\tstatic struct string_list empty_list = STRING_LIST_INIT_NODUP;\n \tstruct config_set_element *e = configset_find_element(cs, key);\n-\treturn e ? &e->value_list : NULL;\n+\treturn e ? &e->value_list : &empty_list;\n }\n \n int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\ndiff --git a/config.h b/config.h\nindex ca994d77147..9897b97c0b9 100644\n--- a/config.h\n+++ b/config.h\n@@ -458,7 +458,7 @@ int git_configset_add_parameters(struct config_set *cs);\n /**\n * Finds and returns the value list, sorted in order of increasing priority\n * for the configuration variable `key` and config set `cs`. When the\n- * configuration variable `key` is not found, returns NULL. The caller\n+ * configuration variable `key` is not found, returns an empty list. The caller\n * should not free or modify the returned pointer, as it is owned by the cache.\n */\n const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key);\n@@ -543,8 +543,8 @@ int git_config_get_value(const char *key, const char **value);\n /**\n * Finds and returns the value list, sorted in order of increasing priority\n * for the configuration variable `key`. When the configuration variable\n- * `key` is not found, returns NULL. The caller should not free or modify\n- * the returned pointer, as it is owned by the cache.\n+ * `key` is not found, returns an empty list. The caller should not free or\n+ * modify the returned pointer, as it is owned by the cache.\n */\n const struct string_list *git_config_get_value_multi(const char *key);\n \n-- \ngitgitgadget\n\n"},{"id":"463724","messageId":"af036388d4e5abe35c23acf4210d46cd69b2d264.1664287711.git.gitgitgadget@gmail.com","threadId":"58518","inReplyTo":"pull.1369.git.1664287711.gitgitgadget@gmail.com","subject":"[PATCH 2/5] *: relax git_configset_get_value_multi result","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-27T14:08:28Z","receivedAt":"2022-09-27T14:08:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWe will change the config API to return an empty list instead of a NULL\nlist when there are no values in a multi-valued key. Update checks that\nlook for NULL and have them instead check for NULL or empty lists.\n\nThere are other checks that look for NULL, but then do an operation that\nwould be a no-op on an empty list. These are not modified here but will\nbe modified after we change the behavior of the config API.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/submodule--helper.c | 10 ++++++++--\n submodule.c | 2 +-\n t/helper/test-config.c | 4 ++--\n versioncmp.c | 8 ++++----\n 4 files changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 0b4acb442b2..1ddc08b076c 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -531,6 +531,7 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \tstruct init_cb info = INIT_CB_INIT;\n \tstruct pathspec pathspec = { 0 };\n \tstruct module_list list = MODULE_LIST_INIT;\n+\tconst struct string_list *active_modules;\n \tint quiet = 0;\n \tstruct option module_init_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"suppress output for initializing a submodule\")),\n@@ -552,7 +553,9 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \t * If there are no path args and submodule.active is set then,\n \t * by default, only initialize 'active' modules.\n \t */\n-\tif (!argc && git_config_get_value_multi(\"submodule.active\"))\n+\tif (!argc &&\n+\t (active_modules = git_config_get_value_multi(\"submodule.active\")) &&\n+\t active_modules->nr)\n \t\tmodule_list_active(&list);\n \n \tinfo.prefix = prefix;\n@@ -2706,6 +2709,7 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t\topt.warn_if_uninitialized = 1;\n \n \tif (opt.init) {\n+\t\tconst struct string_list *active_modules;\n \t\tstruct module_list list = MODULE_LIST_INIT;\n \t\tstruct init_cb info = INIT_CB_INIT;\n \n@@ -2720,7 +2724,9 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t\t * If there are no path args and submodule.active is set then,\n \t\t * by default, only initialize 'active' modules.\n \t\t */\n-\t\tif (!argc && git_config_get_value_multi(\"submodule.active\"))\n+\t\tif (!argc &&\n+\t\t (active_modules = git_config_get_value_multi(\"submodule.active\")) &&\n+\t\t active_modules->nr)\n \t\t\tmodule_list_active(&list);\n \n \t\tinfo.prefix = opt.prefix;\ndiff --git a/submodule.c b/submodule.c\nindex bf7a2c79183..06230961c80 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -275,7 +275,7 @@ int is_tree_submodule_active(struct repository *repo,\n \n \t/* submodule.active is set */\n \tsl = repo_config_get_value_multi(repo, \"submodule.active\");\n-\tif (sl) {\n+\tif (sl && sl->nr) {\n \t\tstruct pathspec ps;\n \t\tstruct strvec args = STRVEC_INIT;\n \t\tconst struct string_list_item *item;\ndiff --git a/t/helper/test-config.c b/t/helper/test-config.c\nindex 4ba9eb65606..62644dd71d7 100644\n--- a/t/helper/test-config.c\n+++ b/t/helper/test-config.c\n@@ -96,7 +96,7 @@ int cmd__config(int argc, const char **argv)\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_value_multi\")) {\n \t\tstrptr = git_config_get_value_multi(argv[2]);\n-\t\tif (strptr) {\n+\t\tif (strptr && strptr->nr) {\n \t\t\tfor (i = 0; i < strptr->nr; i++) {\n \t\t\t\tv = strptr->items[i].string;\n \t\t\t\tif (!v)\n@@ -160,7 +160,7 @@ int cmd__config(int argc, const char **argv)\n \t\t\t}\n \t\t}\n \t\tstrptr = git_configset_get_value_multi(&cs, argv[2]);\n-\t\tif (strptr) {\n+\t\tif (strptr && strptr->nr) {\n \t\t\tfor (i = 0; i < strptr->nr; i++) {\n \t\t\t\tv = strptr->items[i].string;\n \t\t\t\tif (!v)\ndiff --git a/versioncmp.c b/versioncmp.c\nindex 069ee94a4d7..8772f869042 100644\n--- a/versioncmp.c\n+++ b/versioncmp.c\n@@ -164,14 +164,14 @@ int versioncmp(const char *s1, const char *s2)\n \t\tinitialized = 1;\n \t\tprereleases = git_config_get_value_multi(\"versionsort.suffix\");\n \t\tdeprecated_prereleases = git_config_get_value_multi(\"versionsort.prereleasesuffix\");\n-\t\tif (prereleases) {\n-\t\t\tif (deprecated_prereleases)\n+\t\tif (prereleases && prereleases->nr) {\n+\t\t\tif (deprecated_prereleases && deprecated_prereleases->nr)\n \t\t\t\twarning(\"ignoring versionsort.prereleasesuffix because versionsort.suffix is set\");\n \t\t} else\n \t\t\tprereleases = deprecated_prereleases;\n \t}\n-\tif (prereleases && swap_prereleases(s1, s2, (const char *) p1 - s1 - 1,\n-\t\t\t\t\t &diff))\n+\tif (prereleases && prereleases->nr &&\n+\t swap_prereleases(s1, s2, (const char *) p1 - s1 - 1, &diff))\n \t\treturn diff;\n \n \tstate = result_type[state * 3 + (((c2 == '0') + (isdigit (c2) != 0)))];\n-- \ngitgitgadget\n\n"},{"id":"463725","messageId":"e06cb4df081bc2222731f9185a22ed7ad67e3814.1664287711.git.gitgitgadget@gmail.com","threadId":"58518","inReplyTo":"pull.1369.git.1664287711.gitgitgadget@gmail.com","subject":"[PATCH 5/5] *: expect a non-NULL list of config values","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-27T14:08:31Z","receivedAt":"2022-09-27T14:09:01Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe previous change altered the multi-valued config API to return an\nempty list intead of a NULL list. Update all callers to rely on the\nnon-NULL result and instead check for an empty list.\n\nSeveral callers only checked for a NULL list so they could safely call\nfor_each_string_list_item(), which no-ops on an empty list.\n\nThe bitmap_preferred_tips() method is a thin wrapper on the config API,\nso its callers are also updated to match this expectation.\n\nSigned-off-by: Derrick Stolee \n---\n builtin/for-each-repo.c | 8 --------\n builtin/gc.c | 20 ++++++++------------\n builtin/log.c | 10 ++++------\n builtin/pack-objects.c | 3 ---\n builtin/repack.c | 13 +++++--------\n builtin/submodule--helper.c | 10 ++--------\n config.c | 2 +-\n pack-bitmap.c | 3 ---\n t/helper/test-config.c | 2 +-\n 9 files changed, 21 insertions(+), 50 deletions(-)\n\ndiff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c\nindex fd86e5a8619..635ea5e15fd 100644\n--- a/builtin/for-each-repo.c\n+++ b/builtin/for-each-repo.c\n@@ -44,14 +44,6 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)\n \n \tvalues = repo_config_get_value_multi(the_repository,\n \t\t\t\t\t config_key);\n-\n-\t/*\n-\t * Do nothing on an empty list, which is equivalent to the case\n-\t * where the config variable does not exist at all.\n-\t */\n-\tif (!values)\n-\t\treturn 0;\n-\n \tfor (i = 0; !result && i < values->nr; i++)\n \t\tresult = run_command_on_repo(values->items[i].string, argc, argv);\n \ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 7a585f0b71d..1e9ac2ac7e3 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1493,12 +1493,10 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n \t\tgit_config_set(\"maintenance.strategy\", \"incremental\");\n \n \tlist = git_config_get_value_multi(key);\n-\tif (list) {\n-\t\tfor_each_string_list_item(item, list) {\n-\t\t\tif (!strcmp(maintpath, item->string)) {\n-\t\t\t\tfound = 1;\n-\t\t\t\tbreak;\n-\t\t\t}\n+\tfor_each_string_list_item(item, list) {\n+\t\tif (!strcmp(maintpath, item->string)) {\n+\t\t\tfound = 1;\n+\t\t\tbreak;\n \t\t}\n \t}\n \n@@ -1550,12 +1548,10 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n \t\t\t\t options);\n \n \tlist = git_config_get_value_multi(key);\n-\tif (list) {\n-\t\tfor_each_string_list_item(item, list) {\n-\t\t\tif (!strcmp(maintpath, item->string)) {\n-\t\t\t\tfound = 1;\n-\t\t\t\tbreak;\n-\t\t\t}\n+\tfor_each_string_list_item(item, list) {\n+\t\tif (!strcmp(maintpath, item->string)) {\n+\t\t\tfound = 1;\n+\t\t\tbreak;\n \t\t}\n \t}\n \ndiff --git a/builtin/log.c b/builtin/log.c\nindex ee19dc5d450..719ef966045 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -182,15 +182,13 @@ static void set_default_decoration_filter(struct decoration_filter *decoration_f\n \tint i;\n \tchar *value = NULL;\n \tstruct string_list *include = decoration_filter->include_ref_pattern;\n+\tstruct string_list_item *item;\n \tconst struct string_list *config_exclude =\n \t\t\tgit_config_get_value_multi(\"log.excludeDecoration\");\n \n-\tif (config_exclude) {\n-\t\tstruct string_list_item *item;\n-\t\tfor_each_string_list_item(item, config_exclude)\n-\t\t\tstring_list_append(decoration_filter->exclude_ref_config_pattern,\n-\t\t\t\t\t item->string);\n-\t}\n+\tfor_each_string_list_item(item, config_exclude)\n+\t\tstring_list_append(decoration_filter->exclude_ref_config_pattern,\n+\t\t\t\t item->string);\n \n \t/*\n \t * By default, decorate_all is disabled. Enable it if\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 3658c05cafc..d15e1857e67 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3982,9 +3982,6 @@ static void mark_bitmap_preferred_tips(void)\n \tconst struct string_list *preferred_tips;\n \n \tpreferred_tips = bitmap_preferred_tips(the_repository);\n-\tif (!preferred_tips)\n-\t\treturn;\n-\n \tfor_each_string_list_item(item, preferred_tips) {\n \t\tfor_each_ref_in(item->string, mark_bitmap_preferred_tip, NULL);\n \t}\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a5bacc77974..1655747f6e0 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -539,6 +539,7 @@ static int midx_snapshot_ref_one(const char *refname UNUSED,\n static void midx_snapshot_refs(struct tempfile *f)\n {\n \tstruct midx_snapshot_ref_data data;\n+\tstruct string_list_item *item;\n \tconst struct string_list *preferred = bitmap_preferred_tips(the_repository);\n \n \tdata.f = f;\n@@ -549,14 +550,10 @@ static void midx_snapshot_refs(struct tempfile *f)\n \t\t die(_(\"could not open tempfile %s for writing\"),\n \t\t get_tempfile_path(f));\n \n-\tif (preferred) {\n-\t\tstruct string_list_item *item;\n-\n-\t\tdata.preferred = 1;\n-\t\tfor_each_string_list_item(item, preferred)\n-\t\t\tfor_each_ref_in(item->string, midx_snapshot_ref_one, &data);\n-\t\tdata.preferred = 0;\n-\t}\n+\tdata.preferred = 1;\n+\tfor_each_string_list_item(item, preferred)\n+\t\tfor_each_ref_in(item->string, midx_snapshot_ref_one, &data);\n+\tdata.preferred = 0;\n \n \tfor_each_ref(midx_snapshot_ref_one, &data);\n \ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 1ddc08b076c..5a8b6120157 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -531,7 +531,6 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \tstruct init_cb info = INIT_CB_INIT;\n \tstruct pathspec pathspec = { 0 };\n \tstruct module_list list = MODULE_LIST_INIT;\n-\tconst struct string_list *active_modules;\n \tint quiet = 0;\n \tstruct option module_init_options[] = {\n \t\tOPT__QUIET(&quiet, N_(\"suppress output for initializing a submodule\")),\n@@ -553,9 +552,7 @@ static int module_init(int argc, const char **argv, const char *prefix)\n \t * If there are no path args and submodule.active is set then,\n \t * by default, only initialize 'active' modules.\n \t */\n-\tif (!argc &&\n-\t (active_modules = git_config_get_value_multi(\"submodule.active\")) &&\n-\t active_modules->nr)\n+\tif (!argc && git_config_get_value_multi(\"submodule.active\")->nr)\n \t\tmodule_list_active(&list);\n \n \tinfo.prefix = prefix;\n@@ -2709,7 +2706,6 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t\topt.warn_if_uninitialized = 1;\n \n \tif (opt.init) {\n-\t\tconst struct string_list *active_modules;\n \t\tstruct module_list list = MODULE_LIST_INIT;\n \t\tstruct init_cb info = INIT_CB_INIT;\n \n@@ -2724,9 +2720,7 @@ static int module_update(int argc, const char **argv, const char *prefix)\n \t\t * If there are no path args and submodule.active is set then,\n \t\t * by default, only initialize 'active' modules.\n \t\t */\n-\t\tif (!argc &&\n-\t\t (active_modules = git_config_get_value_multi(\"submodule.active\")) &&\n-\t\t active_modules->nr)\n+\t\tif (!argc && git_config_get_value_multi(\"submodule.active\")->nr)\n \t\t\tmodule_list_active(&list);\n \n \t\tinfo.prefix = opt.prefix;\ndiff --git a/config.c b/config.c\nindex 2d4ca1ae6dc..8bd55688352 100644\n--- a/config.c\n+++ b/config.c\n@@ -2407,7 +2407,7 @@ int git_configset_get_value(struct config_set *cs, const char *key, const char *\n \t */\n \tvalues = git_configset_get_value_multi(cs, key);\n \n-\tif (!values || !values->nr)\n+\tif (!values->nr)\n \t\treturn 1;\n \t*value = values->items[values->nr - 1].string;\n \treturn 0;\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 9a208abc1fd..77d02c7bbe7 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -2310,9 +2310,6 @@ int bitmap_is_preferred_refname(struct repository *r, const char *refname)\n \tconst struct string_list *preferred_tips = bitmap_preferred_tips(r);\n \tstruct string_list_item *item;\n \n-\tif (!preferred_tips)\n-\t\treturn 0;\n-\n \tfor_each_string_list_item(item, preferred_tips) {\n \t\tif (starts_with(refname, item->string))\n \t\t\treturn 1;\ndiff --git a/t/helper/test-config.c b/t/helper/test-config.c\nindex 62644dd71d7..90810946783 100644\n--- a/t/helper/test-config.c\n+++ b/t/helper/test-config.c\n@@ -96,7 +96,7 @@ int cmd__config(int argc, const char **argv)\n \t\t}\n \t} else if (argc == 3 && !strcmp(argv[1], \"get_value_multi\")) {\n \t\tstrptr = git_config_get_value_multi(argv[2]);\n-\t\tif (strptr && strptr->nr) {\n+\t\tif (strptr->nr) {\n \t\t\tfor (i = 0; i < strptr->nr; i++) {\n \t\t\t\tv = strptr->items[i].string;\n \t\t\t\tif (!v)\n-- \ngitgitgadget\n"},{"id":"463734","messageId":"220927.86wn9oyejm.gmgdl@evledraar.gmail.com","threadId":"58518","inReplyTo":"f277a7a429db8f54fa06dd1965d62ec491e6d84b.1664287711.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/5] config: add BUG() statement instead of possible segfault","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-27T16:17:40Z","receivedAt":"2022-09-27T16:20:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 27 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n>\n> The git_die_config() method calls git_config_get_value_multi() but\n> immediately navigates to its first value without checking if the result\n> is NULL or empty. Callers should only call git_die_config() if there is\n> at least one value for the given 'key', but such a mistaken use might\n> slip through. It would be better to show a BUG() statement than a\n> possible segfault.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> config.c | 9 +++++++--\n> 1 file changed, 7 insertions(+), 2 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index bf89afbdab0..0c41606c7d4 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2833,8 +2833,13 @@ void git_die_config(const char *key, const char *err, ...)\n> \t\tva_end(params);\n> \t}\n> \tvalues = git_config_get_value_multi(key);\n> -\tkv_info = values->items[values->nr - 1].util;\n> -\tgit_die_config_linenr(key, kv_info->filename, kv_info->linenr);\n> +\n> +\tif (values && values->nr) {\n> +\t\tkv_info = values->items[values->nr - 1].util;\n> +\t\tgit_die_config_linenr(key, kv_info->filename, kv_info->linenr);\n> +\t} else {\n> +\t\tBUG(\"expected a non-empty list of values\");\n> +\t}\n> }\n> \n> /*\n\nAFAIKT the intent of the current code on \"master\" is that this will only\nget called if the likes of git_configset_get_string() returns < 0, not\nif it returns > 0.\n\nSo isn't the combination of your 1/5 and this 3/5 now conflating these\ntwo conditions? See e.g. repo_config_get_string_tmp() and when it would\ncall git_die_config().\n\nI.e. isn't the whole point of git_die_config() to print an error message\nabout a configuration *value* that we've parsed out of the config?\n\nIf e.g. the key itself is bad we'll get a -1, but in this case it seems\nwe would have a BUG(), but it's not that we \"expected a non-empty list\nof values\", but that the state of the world changed between our previous\nconfigset invocation, no?\n"},{"id":"463735","messageId":"220927.86sfkcyebf.gmgdl@evledraar.gmail.com","threadId":"58518","inReplyTo":"396343ce7dd17f86bbbc66197c6f0b4012caf445.1664287711.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/5] config: return an empty list, not NULL","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-27T16:21:31Z","receivedAt":"2022-09-27T16:26:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 27 2022, Derrick Stolee via GitGitGadget wrote:\n\n> From: Derrick Stolee \n>\n> For the multi-valued config API methods, Git previously returned a NULL\n> list instead of an empty list. Previous changes adjusted all callers to\n> instead expect an empty, non-NULL list, making this a safe change.\n>\n> The next change will remove the NULL checks from all callers.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> config.c | 3 ++-\n> config.h | 6 +++---\n> 2 files changed, 5 insertions(+), 4 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index 0c41606c7d4..2d4ca1ae6dc 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2415,8 +2415,9 @@ int git_configset_get_value(struct config_set *cs, const char *key, const char *\n> \n> const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)\n> {\n> +\tstatic struct string_list empty_list = STRING_LIST_INIT_NODUP;\n> \tstruct config_set_element *e = configset_find_element(cs, key);\n> -\treturn e ? &e->value_list : NULL;\n> +\treturn e ? &e->value_list : &empty_list;\n> }\n> \n> int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n> diff --git a/config.h b/config.h\n> index ca994d77147..9897b97c0b9 100644\n> --- a/config.h\n> +++ b/config.h\n> @@ -458,7 +458,7 @@ int git_configset_add_parameters(struct config_set *cs);\n> /**\n> * Finds and returns the value list, sorted in order of increasing priority\n> * for the configuration variable `key` and config set `cs`. When the\n> - * configuration variable `key` is not found, returns NULL. The caller\n> + * configuration variable `key` is not found, returns an empty list. The caller\n> * should not free or modify the returned pointer, as it is owned by the cache.\n> */\n> const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key);\n> @@ -543,8 +543,8 @@ int git_config_get_value(const char *key, const char **value);\n> /**\n> * Finds and returns the value list, sorted in order of increasing priority\n> * for the configuration variable `key`. When the configuration variable\n> - * `key` is not found, returns NULL. The caller should not free or modify\n> - * the returned pointer, as it is owned by the cache.\n> + * `key` is not found, returns an empty list. The caller should not free or\n> + * modify the returned pointer, as it is owned by the cache.\n> */\n> const struct string_list *git_config_get_value_multi(const char *key);\n\nAside from the \"DWIM API\" aspect of this (which I don't mind) I think\nthis is really taking the low-level function in the wrong direction, and\nthat we should just add a new simple wrapper instead.\n\nI.e. both the pre-image API docs & this series gloss over the fact that\nwe'd not just return NULL here if the config wasn't there, but also if\ngit_config_parse_key() failed.\n\nSo it seems to me that a better direction would be starting with\nsomething like the WIP below (which doesn't compile the whole code, I\nstopped at config.[ch] and pack-bitmap.c). I.e. the same \"int\" return\nand \"dest\" pattern that most other things in the config API have.\n\nThe functionality you want in this series would then just be a wrapper\nfor that which wouldn't care about the difference between \"bad key\" and\n\"no such config entry\".\n\nBut I really think the low-level config API should care about the\ndifference, granted before your change it's rather crappy, and would\nreturn NULL for both, but I think we can do better...\n\ndiff --git a/config.c b/config.c\nindex cbb5a3bab74..a9536d3a9e8 100644\n--- a/config.c\n+++ b/config.c\n@@ -2275,23 +2275,28 @@ void read_very_early_config(config_fn_t cb, void *data)\n \tconfig_with_options(cb, data, NULL, &opts);\n }\n \n-static struct config_set_element *configset_find_element(struct config_set *cs, const char *key)\n+static int configset_find_element(struct config_set *cs, const char *key,\n+\t\t\t\t struct config_set_element **dest)\n {\n \tstruct config_set_element k;\n \tstruct config_set_element *found_entry;\n \tchar *normalized_key;\n+\tint ret;\n+\n \t/*\n \t * `key` may come from the user, so normalize it before using it\n \t * for querying entries from the hashmap.\n \t */\n-\tif (git_config_parse_key(key, &normalized_key, NULL))\n-\t\treturn NULL;\n+\tret = git_config_parse_key(key, &normalized_key, NULL);\n+\tif (ret < 0)\n+\t\treturn ret;\n \n \thashmap_entry_init(&k.ent, strhash(normalized_key));\n \tk.key = normalized_key;\n \tfound_entry = hashmap_get_entry(&cs->config_hash, &k, ent, NULL);\n \tfree(normalized_key);\n-\treturn found_entry;\n+\t*dest = found_entry;\n+\treturn 0;\n }\n \n static int configset_add_value(struct config_set *cs, const char *key, const char *value)\n@@ -2300,8 +2305,11 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha\n \tstruct string_list_item *si;\n \tstruct configset_list_item *l_item;\n \tstruct key_value_info *kv_info = xmalloc(sizeof(*kv_info));\n+\tint ret;\n \n-\te = configset_find_element(cs, key);\n+\tret = configset_find_element(cs, key, &e);\n+\tif (ret < 0)\n+\t\treturn ret;\n \t/*\n \t * Since the keys are being fed by git_config*() callback mechanism, they\n \t * are already normalized. So simply add them without any further munging.\n@@ -2400,24 +2408,34 @@ int git_configset_add_parameters(struct config_set *cs)\n int git_configset_get_value(struct config_set *cs, const char *key, const char **value)\n {\n \tconst struct string_list *values = NULL;\n+\tint ret;\n+\n \t/*\n \t * Follows \"last one wins\" semantic, i.e., if there are multiple matches for the\n \t * queried key in the files of the configset, the value returned will be the last\n \t * value in the value list for that key.\n \t */\n-\tvalues = git_configset_get_value_multi(cs, key);\n+\tret = git_configset_get_value_multi(cs, key, &values);\n+\n+\tif (ret < 0)\n+\t\treturn ret;\n \n-\tif (!values)\n-\t\treturn 1;\n \tassert(values->nr > 0);\n \t*value = values->items[values->nr - 1].string;\n \treturn 0;\n }\n \n-const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)\n+int git_configset_get_value_multi(struct config_set *cs, const char *key,\n+\t\t\t\t const struct string_list **dest)\n {\n-\tstruct config_set_element *e = configset_find_element(cs, key);\n-\treturn e ? &e->value_list : NULL;\n+\tstruct config_set_element *e = NULL;\n+\tint ret;\n+\n+\tret = configset_find_element(cs, key, &e);\n+\tif (ret < 0)\n+\t\treturn ret;\n+\t*dest = &e->value_list;\n+\treturn 0;\n }\n \n int git_configset_get_string(struct config_set *cs, const char *key, char **dest)\n@@ -2563,11 +2581,12 @@ int repo_config_get_value(struct repository *repo,\n \treturn git_configset_get_value(repo->config, key, value);\n }\n \n-const struct string_list *repo_config_get_value_multi(struct repository *repo,\n-\t\t\t\t\t\t const char *key)\n+int repo_config_get_value_multi(struct repository *repo,\n+\t\t\t\tconst char *key,\n+\t\t\t\tconst struct string_list **dest)\n {\n \tgit_config_check_init(repo);\n-\treturn git_configset_get_value_multi(repo->config, key);\n+\treturn git_configset_get_value_multi(repo->config, key, dest);\n }\n \n int repo_config_get_string(struct repository *repo,\n@@ -2684,9 +2703,9 @@ int git_config_get_value(const char *key, const char **value)\n \treturn repo_config_get_value(the_repository, key, value);\n }\n \n-const struct string_list *git_config_get_value_multi(const char *key)\n+int git_config_get_value_multi(const char *key, const struct string_list **dest)\n {\n-\treturn repo_config_get_value_multi(the_repository, key);\n+\treturn repo_config_get_value_multi(the_repository, key, dest);\n }\n \n int git_config_get_string(const char *key, char **dest)\n@@ -2826,6 +2845,7 @@ void git_die_config(const char *key, const char *err, ...)\n \tconst struct string_list *values;\n \tstruct key_value_info *kv_info;\n \treport_fn error_fn = get_error_routine();\n+\tint ret;\n \n \tif (err) {\n \t\tva_list params;\n@@ -2833,7 +2853,9 @@ void git_die_config(const char *key, const char *err, ...)\n \t\terror_fn(err, params);\n \t\tva_end(params);\n \t}\n-\tvalues = git_config_get_value_multi(key);\n+\tret = git_config_get_value_multi(key, &values);\n+\tif (ret < 0)\n+\t\tBUG(\"...\");\n \tkv_info = values->items[values->nr - 1].util;\n \tgit_die_config_linenr(key, kv_info->filename, kv_info->linenr);\n }\ndiff --git a/config.h b/config.h\nindex ca994d77147..99b8dc1944c 100644\n--- a/config.h\n+++ b/config.h\n@@ -461,7 +461,8 @@ int git_configset_add_parameters(struct config_set *cs);\n * configuration variable `key` is not found, returns NULL. The caller\n * should not free or modify the returned pointer, as it is owned by the cache.\n */\n-const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key);\n+int git_configset_get_value_multi(struct config_set *cs, const char *key,\n+\t\t\t\t const struct string_list **dest);\n \n /**\n * Clears `config_set` structure, removes all saved variable-value pairs.\n@@ -495,8 +496,9 @@ struct repository;\n void repo_config(struct repository *repo, config_fn_t fn, void *data);\n int repo_config_get_value(struct repository *repo,\n \t\t\t const char *key, const char **value);\n-const struct string_list *repo_config_get_value_multi(struct repository *repo,\n-\t\t\t\t\t\t const char *key);\n+int repo_config_get_value_multi(struct repository *repo,\n+\t\t\t\tconst char *key,\n+\t\t\t\tconst struct string_list **dest);\n int repo_config_get_string(struct repository *repo,\n \t\t\t const char *key, char **dest);\n int repo_config_get_string_tmp(struct repository *repo,\n@@ -546,7 +548,8 @@ int git_config_get_value(const char *key, const char **value);\n * `key` is not found, returns NULL. The caller should not free or modify\n * the returned pointer, as it is owned by the cache.\n */\n-const struct string_list *git_config_get_value_multi(const char *key);\n+int git_config_get_value_multi(const char *key,\n+\t\t\t const struct string_list **dest);\n \n /**\n * Resets and invalidates the config cache.\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 440407f1be7..0d704dc76ef 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -2301,7 +2301,13 @@ int bitmap_is_midx(struct bitmap_index *bitmap_git)\n \n const struct string_list *bitmap_preferred_tips(struct repository *r)\n {\n-\treturn repo_config_get_value_multi(r, \"pack.preferbitmaptips\");\n+\tconst struct string_list *dest;\n+\tint ret;\n+\n+\tret = repo_config_get_value_multi(r, \"pack.preferbitmaptips\", &dest);\n+\tif (ret < 0)\n+\t\tBUG(\"got %d from repo_config_get_value_multi()\", ret);\n+\treturn dest;\n }\n \n int bitmap_is_preferred_refname(struct repository *r, const char *refname)\n"},{"id":"463741","messageId":"4d0cf1a7-0902-af0e-93b0-ad6727bf6c2c@github.com","threadId":"58518","inReplyTo":"220927.86wn9oyejm.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 3/5] config: add BUG() statement instead of possible segfault","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-27T16:46:06Z","receivedAt":"2022-09-27T16:47:46Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/27/2022 12:17 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Sep 27 2022, Derrick Stolee via GitGitGadget wrote:\n> \n>> From: Derrick Stolee \n>>\n>> The git_die_config() method calls git_config_get_value_multi() but\n>> immediately navigates to its first value without checking if the result\n>> is NULL or empty. Callers should only call git_die_config() if there is\n>> at least one value for the given 'key', but such a mistaken use might\n>> slip through. It would be better to show a BUG() statement than a\n>> possible segfault.\n \n> AFAIKT the intent of the current code on \"master\" is that this will only\n> get called if the likes of git_configset_get_string() returns < 0, not\n> if it returns > 0.\n> \n> So isn't the combination of your 1/5 and this 3/5 now conflating these\n> two conditions? See e.g. repo_config_get_string_tmp() and when it would\n> call git_die_config().\n> \n> I.e. isn't the whole point of git_die_config() to print an error message\n> about a configuration *value* that we've parsed out of the config?\n> \n> If e.g. the key itself is bad we'll get a -1, but in this case it seems\n> we would have a BUG(), but it's not that we \"expected a non-empty list\n> of values\", but that the state of the world changed between our previous\n> configset invocation, no?\n\nIf git_die_config() was static to config.c, then I would agree with you\nthat its use is controlled enough to avoid that possibility. However, it\nis available in config.h and its doc comment does not say anything about\n\"make sure 'key' properly parses at least one value\".\n\nThe point is defense-in-depth to prevent a segfault if someone adds an\nincorrect caller. It looks too easy to add an erroneous caller.\n\nThanks,\n-Stolee\n"},{"id":"463742","messageId":"b85d8a43-6640-aa9c-3103-0c3d43c2a479@github.com","threadId":"58518","inReplyTo":"220927.86sfkcyebf.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 4/5] config: return an empty list, not NULL","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-27T16:50:37Z","receivedAt":"2022-09-27T16:52:24Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/27/2022 12:21 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Sep 27 2022, Derrick Stolee via GitGitGadget wrote:\n\n>> /**\n>> * Finds and returns the value list, sorted in order of increasing priority\n>> * for the configuration variable `key`. When the configuration variable\n>> - * `key` is not found, returns NULL. The caller should not free or modify\n>> - * the returned pointer, as it is owned by the cache.\n>> + * `key` is not found, returns an empty list. The caller should not free or\n>> + * modify the returned pointer, as it is owned by the cache.\n>> */\n>> const struct string_list *git_config_get_value_multi(const char *key);\n> \n> Aside from the \"DWIM API\" aspect of this (which I don't mind) I think\n> this is really taking the low-level function in the wrong direction, and\n> that we should just add a new simple wrapper instead.\n> \n> I.e. both the pre-image API docs & this series gloss over the fact that\n> we'd not just return NULL here if the config wasn't there, but also if\n> git_config_parse_key() failed.\n> \n> So it seems to me that a better direction would be starting with\n> something like the WIP below (which doesn't compile the whole code, I\n> stopped at config.[ch] and pack-bitmap.c). I.e. the same \"int\" return\n> and \"dest\" pattern that most other things in the config API have.\n\nDo you have an example where a caller would benefit from this\ndistinction? Without such an example, I don't think it is worth\ncreating such a huge change for purity's sake alone.\n\nI'm pretty happy that the diff for this series is an overall\nreduction in code, while also not being too large in the interim:\n\n 12 files changed, 39 insertions(+), 57 deletions(-)\n\nIf all callers that use the *_multi() methods would only use the\nwrapper, then what is the point of doing the low-level manipulations?\n\nThanks,\n-Stolee\n"},{"id":"463745","messageId":"xmqqmtak685r.fsf@gitster.g","threadId":"58518","inReplyTo":"b2558a737f31465b38ad361a412d923ef186a38a.1664287711.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/5] config: relax requirements on multi-value return","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-27T17:26:08Z","receivedAt":"2022-09-27T17:26:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> From: Derrick Stolee \n>\n> The git_configset_get_value() method has an assert() statement\n> guaranteeing that the result from git_configset_get_value_multi() is\n> either NULL or has at least one element. We want to change that return\n> to provide an empty list instead of a NULL list, so change the earlier\n> 'return 1' condition to care about a NULL or empty list.\n\nIf we are allowing to return an empty string_list from the function\nin later steps, the assert() that insists the list has at least 1\nitem on it will get in the way. OK.\n\n> Signed-off-by: Derrick Stolee \n> ---\n> config.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index cbb5a3bab74..bf89afbdab0 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -2407,9 +2407,8 @@ int git_configset_get_value(struct config_set *cs, const char *key, const char *\n> \t */\n> \tvalues = git_configset_get_value_multi(cs, key);\n> \n> -\tif (!values)\n> +\tif (!values || !values->nr)\n> \t\treturn 1;\n> -\tassert(values->nr > 0);\n> \t*value = values->items[values->nr - 1].string;\n\nBut we need to guard against an empty list, obviously. OK.\n\n> \treturn 0;\n> }\n"},{"id":"463746","messageId":"220927.86o7v0yb8h.gmgdl@evledraar.gmail.com","threadId":"58518","inReplyTo":"4d0cf1a7-0902-af0e-93b0-ad6727bf6c2c@github.com","subject":"Re: [PATCH 3/5] config: add BUG() statement instead of possible segfault","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-27T17:22:46Z","receivedAt":"2022-09-27T17:32:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 27 2022, Derrick Stolee wrote:\n\n> On 9/27/2022 12:17 PM, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Tue, Sep 27 2022, Derrick Stolee via GitGitGadget wrote:\n>> \n>>> From: Derrick Stolee \n>>>\n>>> The git_die_config() method calls git_config_get_value_multi() but\n>>> immediately navigates to its first value without checking if the result\n>>> is NULL or empty. Callers should only call git_die_config() if there is\n>>> at least one value for the given 'key', but such a mistaken use might\n>>> slip through. It would be better to show a BUG() statement than a\n>>> possible segfault.\n> \n>> AFAIKT the intent of the current code on \"master\" is that this will only\n>> get called if the likes of git_configset_get_string() returns < 0, not\n>> if it returns > 0.\n>> \n>> So isn't the combination of your 1/5 and this 3/5 now conflating these\n>> two conditions? See e.g. repo_config_get_string_tmp() and when it would\n>> call git_die_config().\n>> \n>> I.e. isn't the whole point of git_die_config() to print an error message\n>> about a configuration *value* that we've parsed out of the config?\n>> \n>> If e.g. the key itself is bad we'll get a -1, but in this case it seems\n>> we would have a BUG(), but it's not that we \"expected a non-empty list\n>> of values\", but that the state of the world changed between our previous\n>> configset invocation, no?\n>\n> If git_die_config() was static to config.c, then I would agree with you\n> that its use is controlled enough to avoid that possibility. However, it\n> is available in config.h and its doc comment does not say anything about\n> \"make sure 'key' properly parses at least one value\".\n\nIt does, it's part of the configset API, which any non-trivial user\nunderstands adhares to the semantics of the configset cache.\n\nE.g. the fast-import.c caller is doing, basically:\n\n\tif (!git_config_get_int(\"pack.indexversion\", &indexversion_value))\n\t\t/* ...if we're not happy with indexversion_value ...*/\n\t\tgit_die_config(\"pack.indexversion\", ...);\n\nAnyway, I'm all for adding some extra paranoia with a BUG(), it was just\nunclear to me if this was some segfault we were expecting to perhaps\nhave now, or with API use after this series.\n\nAFAICT it's really neither of those, but just some side-paranoia added\nwhile-at-it for a *different* use-case than the primary goal of this\nseries. I.e. one where you're looking up a key you don't know exists,\nwhereas git_die_config() is for a key we *know* exists, but don't like\nits value.\n\nI think it's better if we just clarify that it's supposed to be used by\nthat in the API docs, and if you insist on the BUG then the:\n\n\tBUG(\"expected a non-empty list of values\");\n\nShould really get to the point directly, and say something like:\n\n\tBUG(\"called with non-existing key '%s'? \"\n \"Call git_die_config(...) only with keys whose value you don't like\", key);\n\n\n"},{"id":"463754","messageId":"220927.86k05oy5oi.gmgdl@evledraar.gmail.com","threadId":"58518","inReplyTo":"b85d8a43-6640-aa9c-3103-0c3d43c2a479@github.com","subject":"Re: [PATCH 4/5] config: return an empty list, not NULL","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-27T19:18:48Z","receivedAt":"2022-09-27T19:32:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Sep 27 2022, Derrick Stolee wrote:\n\n> On 9/27/2022 12:21 PM, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Tue, Sep 27 2022, Derrick Stolee via GitGitGadget wrote:\n>\n>>> /**\n>>> * Finds and returns the value list, sorted in order of increasing priority\n>>> * for the configuration variable `key`. When the configuration variable\n>>> - * `key` is not found, returns NULL. The caller should not free or modify\n>>> - * the returned pointer, as it is owned by the cache.\n>>> + * `key` is not found, returns an empty list. The caller should not free or\n>>> + * modify the returned pointer, as it is owned by the cache.\n>>> */\n>>> const struct string_list *git_config_get_value_multi(const char *key);\n>> \n>> Aside from the \"DWIM API\" aspect of this (which I don't mind) I think\n>> this is really taking the low-level function in the wrong direction, and\n>> that we should just add a new simple wrapper instead.\n>> \n>> I.e. both the pre-image API docs & this series gloss over the fact that\n>> we'd not just return NULL here if the config wasn't there, but also if\n>> git_config_parse_key() failed.\n>> \n>> So it seems to me that a better direction would be starting with\n>> something like the WIP below (which doesn't compile the whole code, I\n>> stopped at config.[ch] and pack-bitmap.c). I.e. the same \"int\" return\n>> and \"dest\" pattern that most other things in the config API have.\n>\n> Do you have an example where a caller would benefit from this\n> distinction? Without such an example, I don't think it is worth\n> creating such a huge change for purity's sake alone.\n\nNot initially, I started poking at this because the CL/series/commits\nsays that we don't care about the case of non-existing keys, without\nbeing clear as to why we want to conflate that with other errors we\nmight get from this API.\n\nBut after some digging I found:\n\n\t$ for k in a a.b. \"'x.y\"; do ./git for-each-repo --config=$k; echo $?; done\n\terror: key does not contain a section: a\n\t0\n\terror: key does not contain variable name: a.b.\n\t0\n\terror: invalid key: 'x.y\n\t0\n\t\nI.e. the repo_config_get_value_multi() you added in for-each-repo\ndoesn't distinguish between bad keys and non-existing keys, and returns\n0 even though it printed an \"error\".\n\n> I'm pretty happy that the diff for this series is an overall\n> reduction in code, while also not being too large in the interim:\n>\n> 12 files changed, 39 insertions(+), 57 deletions(-)\n>\n> If all callers that use the *_multi() methods would only use the\n> wrapper, then what is the point of doing the low-level manipulations?\n\nI hacked up something that's at least RFC-quality based on this\napproach, but CI is running etc., so not submitting it\nnow:\n\n\thttps://github.com/git/git/compare/master...avar:git:avar/have-git_configset_get_value-use-dest-and-int-pattern\n\nI think the resulting diff is more idiomatic API use, i.e. you ended up\nwith:\n\n\t /* submodule.active is set */\n\t sl = repo_config_get_value_multi(repo, \"submodule.active\");\n\t- if (sl) {\n\t+ if (sl && sl->nr) {\n\nBut I ended up doing:\n\n\t /* submodule.active is set */\n\t- sl = repo_config_get_value_multi(repo, \"submodule.active\");\n\t- if (sl) {\n\t+ if (!repo_config_get_const_value_multi(repo, \"submodule.active\", &sl)) {\n\nNote the \"const\" in the function name, i.e. there's wrappers that handle\nthe case where we have a hardcoded key name, in which case we can BUG()\nout if we'd return < 0, so all we have left is just \"does key exist\".\n\nIn any case, I'm all for having some simple wrapper for the common cases.\n\nBut I didn't find a single case where we actually needed this \"never\ngive me a non-NULL list\" behavior, it could just be generalized to\n\"let's have the API tell us if the key exist\".\n\nIf you use the non-\"const\" API you can distinguish the err < 0 case, so\nfor-each-repo can now error out appropriately:\n\n\t$ ./git for-each-repo --config=a; echo $?\n\terror: key does not contain a section: a\n\tfatal: got bad config --config=a\n\t\n\tusage: git for-each-repo --config= \n\t\n\t --config config key storing a list of repository paths\n\t\n\t129\n"},{"id":"463782","messageId":"xmqq35cc1arp.fsf@gitster.g","threadId":"58518","inReplyTo":"pull.1369.git.1664287711.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/5] [RFC] config API: return empty list, not NULL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-28T02:40:58Z","receivedAt":"2022-09-28T02:41:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Derrick Stolee via GitGitGadget\" writes:\n\n> This work changes the behavior of asking for a multi-valued config key to\n> return an empty list instead of a NULL value. This simplifies the handling\n> of the result and is safer for development in the future.\n>\n> This is based on v4 of my unregister series [1]\n>\n> [1]\n> https://lore.kernel.org/git/pull.1358.v4.git.1664287021.gitgitgadget@gmail.com/\n>\n> This idea came about due to a bug in the git maintenance unregister work\n> where the result from git_config_get_value_multi() was sent directly to\n> for_each_string_list_item() without checking for a NULL value first.\n>\n> I'm sending this as an RFC mostly because I'm not 100% sure this shift is\n> worth the refactoring pain and effort. I personally think getting an empty\n> list is a safer choice, but I can also understand if someone has a different\n> opinion.\n\nThanks.\n\nI actually am in favor of the idea that a NULL can be passed around\nto signal the lack of a string_list (or the lack of a instance of\nany \"collection\" type), and the current code is structured as such,\nand it gives us extra flexibility. Of course, we need to see if\nthat extra flexibility is worth it.\n\nWith a colleciton col, \"if (col && col->nr)\" checks if we have\nsomething to work on. But a code like this (which is a longhand for\nthe for_each_string_list_item() issue we just reencountered):\n\n col = git_get_some_collection(...);\n if (!col)\n\treturn; /* no collection */\n if (!col->nr)\n\tgit_add_to_some_collection(col, the default item);\n for (i = 0; i < col->nr; i++)\n\tdo things on col.stuff[i];\n\ncan react differently to cases where we have an empty collection\nand where we do not have any collection to begin with. \n\nThe other side of the coin is that it would make it harder to treat\nthe lack of collection itself and the collection being empty the\nsame way. The above code might need to become\n\n col = git_get_some_collection(...);\n if (!col)\n\tcol = git_get_empty_collection();\n if (!col->nr)\n\tgit_add_to_some_collection(col, the default item);\n for (i = 0; i < col->nr; i++)\n\tdo things on col.stuff[i];\n\nbut if the \"get the collection\" thing returns an empty collection\nwhen there is actually no collection, we can lose two lines from\nthere.\n\nBetween these two positions, I am in favor of the flexibility that\nwe can use NULL to signal the lack of collection, not a presence of\na collection with zero items, as it feels conceptually cleaner.\n\nCounting the hunks in [2/5] and [5/5], it seems that \"when no\nvariable with given key is defined, we return an empty set\" makes us\nto have more code in 7 places in [PATCH 2/5], while allowing us to\nlose code in 10 places in [PATCH 5/5].\n"},{"id":"463842","messageId":"26b3c9ef-5dd7-18f2-89c4-8d210a409ce4@github.com","threadId":"58518","inReplyTo":"220927.86k05oy5oi.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 4/5] config: return an empty list, not NULL","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-28T13:46:48Z","receivedAt":"2022-09-28T13:46:54Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/27/22 3:18 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Sep 27 2022, Derrick Stolee wrote:\n> \n>> On 9/27/2022 12:21 PM, Ævar Arnfjörð Bjarmason wrote:\n>>>\n>>> On Tue, Sep 27 2022, Derrick Stolee via GitGitGadget wrote:\n>>\n>>>> /**\n>>>> * Finds and returns the value list, sorted in order of increasing priority\n>>>> * for the configuration variable `key`. When the configuration variable\n>>>> - * `key` is not found, returns NULL. The caller should not free or modify\n>>>> - * the returned pointer, as it is owned by the cache.\n>>>> + * `key` is not found, returns an empty list. The caller should not free or\n>>>> + * modify the returned pointer, as it is owned by the cache.\n>>>> */\n>>>> const struct string_list *git_config_get_value_multi(const char *key);\n>>>\n>>> Aside from the \"DWIM API\" aspect of this (which I don't mind) I think\n>>> this is really taking the low-level function in the wrong direction, and\n>>> that we should just add a new simple wrapper instead.\n>>>\n>>> I.e. both the pre-image API docs & this series gloss over the fact that\n>>> we'd not just return NULL here if the config wasn't there, but also if\n>>> git_config_parse_key() failed.\n>>>\n>>> So it seems to me that a better direction would be starting with\n>>> something like the WIP below (which doesn't compile the whole code, I\n>>> stopped at config.[ch] and pack-bitmap.c). I.e. the same \"int\" return\n>>> and \"dest\" pattern that most other things in the config API have.\n>>\n>> Do you have an example where a caller would benefit from this\n>> distinction? Without such an example, I don't think it is worth\n>> creating such a huge change for purity's sake alone.\n> \n> Not initially, I started poking at this because the CL/series/commits\n> says that we don't care about the case of non-existing keys, without\n> being clear as to why we want to conflate that with other errors we\n> might get from this API.\n> \n> But after some digging I found:\n> \n> \t$ for k in a a.b. \"'x.y\"; do ./git for-each-repo --config=$k; echo $?; done\n> \terror: key does not contain a section: a\n> \t0\n> \terror: key does not contain variable name: a.b.\n> \t0\n> \terror: invalid key: 'x.y\n> \t0\n> \t\n> I.e. the repo_config_get_value_multi() you added in for-each-repo\n> doesn't distinguish between bad keys and non-existing keys, and returns\n> 0 even though it printed an \"error\".\n\nI can understand wanting to inform the user that they provided an\ninvalid key using a nonzero exit code. I can also understand that\nthe command does what is asked: it did nothing because the given\nkey has no values (because it can't). I think the use of an \"error\"\nmessage balances things towards wanting a nonzero exit code.\n\n>> I'm pretty happy that the diff for this series is an overall\n>> reduction in code, while also not being too large in the interim:\n>>\n>> 12 files changed, 39 insertions(+), 57 deletions(-)\n>>\n>> If all callers that use the *_multi() methods would only use the\n>> wrapper, then what is the point of doing the low-level manipulations?\n> \n> I hacked up something that's at least RFC-quality based on this\n> approach, but CI is running etc., so not submitting it\n> now:\n> \n> \thttps://github.com/git/git/compare/master...avar:git:avar/have-git_configset_get_value-use-dest-and-int-pattern\n> \n> I think the resulting diff is more idiomatic API use, i.e. you ended up\n> with:\n> \n> \t /* submodule.active is set */\n> \t sl = repo_config_get_value_multi(repo, \"submodule.active\");\n> \t- if (sl) {\n> \t+ if (sl && sl->nr) {\n\nYou're right that I forgot to change this one to \"if (sl->nr)\"\nin patch 5.\n\n> But I ended up doing:\n> \n> \t /* submodule.active is set */\n> \t- sl = repo_config_get_value_multi(repo, \"submodule.active\");\n> \t- if (sl) {\n> \t+ if (!repo_config_get_const_value_multi(repo, \"submodule.active\", &sl)) {\n> \n> Note the \"const\" in the function name, i.e. there's wrappers that handle\n> the case where we have a hardcoded key name, in which case we can BUG()\n> out if we'd return < 0, so all we have left is just \"does key exist\".\n\nThe problem here is that the block actually cares that the list is non-empty\nand should not run if the list is empty. In that case, you would need to add\n\"&& sl->nr\" to the condition.\n\nI'm of course assuming that an empty list is different from an error. In\nyour for-each-repo example, we would not want to return a non-zero exit\ncode on an empty list, only on a bad key (or other I/O problem).\n\nIf we return a negative value on an error and the number of matches on\nsuccess, then this change could instead be \"if (repo_config....() > 0)\".\n\n> In any case, I'm all for having some simple wrapper for the common cases\nA simple wrapper would be nice, and be exactly the method as it is\nupdated in this series. The error-result version could be adopted when\nthere is reason to do so.\n\n> But I didn't find a single case where we actually needed this \"never\n> give me a non-NULL list\" behavior, it could just be generalized to\n> \"let's have the API tell us if the key exist\".\n\nMost cases want to feed the result into the for_each_string_list_item()\nmacro. Based on the changes in patch 5, I think the empty list is a\nbetter pattern and leads to prettier code in almost all cases.\n\nThanks,\n-Stolee\n\n"},{"id":"463846","messageId":"220928.86y1u3wnaz.gmgdl@evledraar.gmail.com","threadId":"58518","inReplyTo":"26b3c9ef-5dd7-18f2-89c4-8d210a409ce4@github.com","subject":"Re: [PATCH 4/5] config: return an empty list, not NULL","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-28T14:37:55Z","receivedAt":"2022-09-28T15:07:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 28 2022, Derrick Stolee wrote:\n\n> On 9/27/22 3:18 PM, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Tue, Sep 27 2022, Derrick Stolee wrote:\n>> \n>>> On 9/27/2022 12:21 PM, Ævar Arnfjörð Bjarmason wrote:\n>>>>\n>>>> On Tue, Sep 27 2022, Derrick Stolee via GitGitGadget wrote:\n>>>\n>>>>> /**\n>>>>> * Finds and returns the value list, sorted in order of increasing priority\n>>>>> * for the configuration variable `key`. When the configuration variable\n>>>>> - * `key` is not found, returns NULL. The caller should not free or modify\n>>>>> - * the returned pointer, as it is owned by the cache.\n>>>>> + * `key` is not found, returns an empty list. The caller should not free or\n>>>>> + * modify the returned pointer, as it is owned by the cache.\n>>>>> */\n>>>>> const struct string_list *git_config_get_value_multi(const char *key);\n>>>>\n>>>> Aside from the \"DWIM API\" aspect of this (which I don't mind) I think\n>>>> this is really taking the low-level function in the wrong direction, and\n>>>> that we should just add a new simple wrapper instead.\n>>>>\n>>>> I.e. both the pre-image API docs & this series gloss over the fact that\n>>>> we'd not just return NULL here if the config wasn't there, but also if\n>>>> git_config_parse_key() failed.\n>>>>\n>>>> So it seems to me that a better direction would be starting with\n>>>> something like the WIP below (which doesn't compile the whole code, I\n>>>> stopped at config.[ch] and pack-bitmap.c). I.e. the same \"int\" return\n>>>> and \"dest\" pattern that most other things in the config API have.\n>>>\n>>> Do you have an example where a caller would benefit from this\n>>> distinction? Without such an example, I don't think it is worth\n>>> creating such a huge change for purity's sake alone.\n>> \n>> Not initially, I started poking at this because the CL/series/commits\n>> says that we don't care about the case of non-existing keys, without\n>> being clear as to why we want to conflate that with other errors we\n>> might get from this API.\n>> \n>> But after some digging I found:\n>> \n>> \t$ for k in a a.b. \"'x.y\"; do ./git for-each-repo --config=$k; echo $?; done\n>> \terror: key does not contain a section: a\n>> \t0\n>> \terror: key does not contain variable name: a.b.\n>> \t0\n>> \terror: invalid key: 'x.y\n>> \t0\n>> \t\n>> I.e. the repo_config_get_value_multi() you added in for-each-repo\n>> doesn't distinguish between bad keys and non-existing keys, and returns\n>> 0 even though it printed an \"error\".\n>\n> I can understand wanting to inform the user that they provided an\n> invalid key using a nonzero exit code. I can also understand that\n> the command does what is asked: it did nothing because the given\n> key has no values (because it can't). I think the use of an \"error\"\n> message balances things towards wanting a nonzero exit code.\n\nRight, to be clear I think 6c62f015520 (for-each-repo: do nothing on\nempty config, 2021-01-08) is sensible, i.e. we want to return 0 on a\nnon-existing key.\n\nWe just shouldn't conflate that with e.g. these parse errors, which the\nAPI squashing the underlying negative return values and the \"NULL list\"\nimposes on the user.\n\n>>> I'm pretty happy that the diff for this series is an overall\n>>> reduction in code, while also not being too large in the interim:\n>>>\n>>> 12 files changed, 39 insertions(+), 57 deletions(-)\n>>>\n>>> If all callers that use the *_multi() methods would only use the\n>>> wrapper, then what is the point of doing the low-level manipulations?\n>> \n>> I hacked up something that's at least RFC-quality based on this\n>> approach, but CI is running etc., so not submitting it\n>> now:\n>> \n>> \thttps://github.com/git/git/compare/master...avar:git:avar/have-git_configset_get_value-use-dest-and-int-pattern\n>> \n>> I think the resulting diff is more idiomatic API use, i.e. you ended up\n>> with:\n>> \n>> \t /* submodule.active is set */\n>> \t sl = repo_config_get_value_multi(repo, \"submodule.active\");\n>> \t- if (sl) {\n>> \t+ if (sl && sl->nr) {\n>\n> You're right that I forgot to change this one to \"if (sl->nr)\"\n> in patch 5.\n\nIf I am I didn't mean to point that out, I ws just pointing out the\nend-API use. I.e. int return value v.s. the \"populate dest\" pattern, but\nyes, in your end-state you'd drop the \"sl &&\" part.\n\n>> But I ended up doing:\n>> \n>> \t /* submodule.active is set */\n>> \t- sl = repo_config_get_value_multi(repo, \"submodule.active\");\n>> \t- if (sl) {\n>> \t+ if (!repo_config_get_const_value_multi(repo, \"submodule.active\", &sl)) {\n>> \n>> Note the \"const\" in the function name, i.e. there's wrappers that handle\n>> the case where we have a hardcoded key name, in which case we can BUG()\n>> out if we'd return < 0, so all we have left is just \"does key exist\".\n>\n> The problem here is that the block actually cares that the list is non-empty\n> and should not run if the list is empty. In that case, you would need to add\n> \"&& sl->nr\" to the condition.\n>\n> I'm of course assuming that an empty list is different from an error. In\n> your for-each-repo example, we would not want to return a non-zero exit\n> code on an empty list, only on a bad key (or other I/O problem).\n>\n> If we return a negative value on an error and the number of matches on\n> success, then this change could instead be \"if (repo_config....() > 0)\".\n\nHrm, I think you're confusing the worldview your series here is\nadvocating for, and what I'm suggesting as an alternative.\n\nThere isn't any way on \"master\" to have \"an empty list\", that's a\nworldview you're proposing. In particular your 1/5 here removes:\n\n\tassert(values->nr > 0);\n\nMore generally the config format has no notion of \"an empty list\", if\nyou have a valid key-value pair at all you have a list of \".nr >= 1\".\n\nThe \"empty list\" is a construct you're introducing in this series,\nbecause you wanted the idiom of passing things to\nfor_each_string_list_item.\n\nI'm advocating for not going that route, and instead make the *_multi()\nmethod like the rest of the config API. I.e. to use the \"return int,\npopulate dest\" pattern.\n\nIt's fine if we disagree, but I get the sense that it's not clear what\nwe're disagreeing *on* :)\n\n>> In any case, I'm all for having some simple wrapper for the common cases\n> A simple wrapper would be nice, and be exactly the method as it is\n> updated in this series. The error-result version could be adopted when\n> there is reason to do so.\n\nWell, no :) We ended up with two different \"simple wrapper[s]\", mine\ndoesn't have this notion of a \"struct string_list *list\" with .nr == 0.\n\n>> But I didn't find a single case where we actually needed this \"never\n>> give me a non-NULL list\" behavior, it could just be generalized to\n>> \"let's have the API tell us if the key exist\".\n>\n> Most cases want to feed the result into the for_each_string_list_item()\n> macro. Based on the changes in patch 5, I think the empty list is a\n> better pattern and leads to prettier code in almost all cases.\n\nI updated the WIP RFC series I linked to upthread a bit since my initial\nreply (the link is still good, I force-pushed), I then rebased your\nseries here on \"master\", below is a diff of some select files.\n\nThe overall diff is much bigger obviously (API changes and all), but the\nbelow demonstrates some of the API changes (yours is \"-\", mine is\n\"+\"). I've commented inline on some of it:\n\n\tdiff --git a/builtin/for-each-repo.c b/builtin/for-each-repo.c\n\tindex 635ea5e15fd..16e9a76d04a 100644\n\t--- a/builtin/for-each-repo.c\n\t+++ b/builtin/for-each-repo.c\n\t@@ -29,6 +29,7 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)\n\t \tstatic const char *config_key = NULL;\n\t \tint i, result = 0;\n\t \tconst struct string_list *values;\n\t+\tint err;\n\t \n\t \tconst struct option options[] = {\n\t \t\tOPT_STRING(0, \"config\", &config_key, N_(\"config\"),\n\t@@ -42,8 +43,13 @@ int cmd_for_each_repo(int argc, const char **argv, const char *prefix)\n\t \tif (!config_key)\n\t \t\tdie(_(\"missing --config=\"));\n\t \n\t-\tvalues = repo_config_get_value_multi(the_repository,\n\t-\t\t\t\t\t config_key);\n\t+\terr = repo_config_get_value_multi(the_repository, config_key, &values);\n\t+\tif (err < 0)\n\t+\t\tusage_msg_optf(_(\"got bad config --config=%s\"),\n\t+\t\t\t for_each_repo_usage, options, config_key);\n\t+\telse if (err)\n\t+\t\treturn 0;\n\t+\n\t \tfor (i = 0; !result && i < values->nr; i++)\n\t \t\tresult = run_command_on_repo(values->items[i].string, argc, argv);\n\nHere we're relying an error to the user that we couldn't before, because\nrepo_config_get_value_multi() would return \"NULL\" for both \"key is bad\"\nand \"key doesn't exist\". There's a corresponding test modification\nbelow.\n\n\tdiff --git a/builtin/gc.c b/builtin/gc.c\n\tindex 1e9ac2ac7e3..94b77a88a99 100644\n\t--- a/builtin/gc.c\n\t+++ b/builtin/gc.c\n\t@@ -1472,9 +1472,7 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n\t \t};\n\t \tint found = 0;\n\t \tconst char *key = \"maintenance.repo\";\n\t-\tchar *config_value;\n\t \tchar *maintpath = get_maintpath();\n\t-\tstruct string_list_item *item;\n\t \tconst struct string_list *list;\n\t \n\t \targc = parse_options(argc, argv, prefix, options,\n\t@@ -1487,18 +1485,11 @@ static int maintenance_register(int argc, const char **argv, const char *prefix)\n\t \tgit_config_set(\"maintenance.auto\", \"false\");\n\t \n\t \t/* Set maintenance strategy, if unset */\n\t-\tif (!git_config_get_string(\"maintenance.strategy\", &config_value))\n\t-\t\tfree(config_value);\n\t-\telse\n\t+\tif (git_config_lookup_value(\"maintenance.strategy\"))\n\t \t\tgit_config_set(\"maintenance.strategy\", \"incremental\");\n\nIn looking at this I thought we were way overdue for a \"does this key\nexist?\" helper, this and a few other API users use it.\n\t \n\t-\tlist = git_config_get_value_multi(key);\n\t-\tfor_each_string_list_item(item, list) {\n\t-\t\tif (!strcmp(maintpath, item->string)) {\n\t-\t\t\tfound = 1;\n\t-\t\t\tbreak;\n\t-\t\t}\n\t-\t}\n\t+\tif (!git_config_get_const_value_multi(key, &list))\n\t+\t\tfound = unsorted_string_list_has_string(list, maintpath);\n\nSo, it turns out that the initial reason you wanted the \"pass NULL to\nfor_each_string_list_item\" is actually something we can do with\nunsorted_string_list_has_string(), which implements the same loop.\n\nThe difference here is *the* API difference we're discussing. I.e. we'll\nnever get a NULL \"list\", we'll instead always get a non-NULL list with\n>= 1 item if we can get this key at all.\n\nThe \"const value\" helper is a wrapper that handles the \"err < 0\" case. I\ncases where we hardcode the key it's a BUG() if we get \"err < 0\". The\nwrapper is just:\n\n\tint err = git_configset_get_value_multi(cs, key, dest);\n if (err < 0)\n\t\tBUG(\"failed to parse constant key '%s'!\", key);\n\treturn err;\n\n\t[...]\n\t@@ -1547,13 +1537,8 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi\n\t \t\tusage_with_options(builtin_maintenance_unregister_usage,\n\t \t\t\t\t options);\n\t \n\t-\tlist = git_config_get_value_multi(key);\n\t-\tfor_each_string_list_item(item, list) {\n\t-\t\tif (!strcmp(maintpath, item->string)) {\n\t-\t\t\tfound = 1;\n\t-\t\t\tbreak;\n\t-\t\t}\n\t-\t}\n\t+\tif (!git_config_get_const_value_multi(key, &list))\n\t+\t\tfound = unsorted_string_list_has_string(list, maintpath);\n\nDitto the same git_config_get_const_value_multi() &\nunsorted_string_list_has_string() pattern.\n\t \n\t \tif (found) {\n\t \t\tint rc;\n\tdiff --git a/builtin/log.c b/builtin/log.c\n\tindex 719ef966045..bdb87f6c42b 100644\n\t--- a/builtin/log.c\n\t+++ b/builtin/log.c\n\t@@ -182,13 +182,15 @@ static void set_default_decoration_filter(struct decoration_filter *decoration_f\n\t \tint i;\n\t \tchar *value = NULL;\n\t \tstruct string_list *include = decoration_filter->include_ref_pattern;\n\t-\tstruct string_list_item *item;\n\t-\tconst struct string_list *config_exclude =\n\t-\t\t\tgit_config_get_value_multi(\"log.excludeDecoration\");\n\t+\tconst struct string_list *config_exclude;\n\t \n\t-\tfor_each_string_list_item(item, config_exclude)\n\t-\t\tstring_list_append(decoration_filter->exclude_ref_config_pattern,\n\t-\t\t\t\t item->string);\n\t+\tif (!git_config_get_const_value_multi(\"log.excludeDecoration\",\n\t+\t\t\t\t\t &config_exclude)) {\n\t+\t\tstruct string_list_item *item;\n\t+\t\tfor_each_string_list_item(item, config_exclude)\n\t+\t\t\tstring_list_append(decoration_filter->exclude_ref_config_pattern,\n\t+\t\t\t\t\t item->string);\n\t+\t}\n\nHere's a case where we need to use for_each_string_list_item(), I think\nit's nice how we can now scope the \"item\" variable.\n\n\t \t/*\n\t \t * By default, decorate_all is disabled. Enable it if\n\tdiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n\tindex 5a8b6120157..b758255f816 100644\n\t--- a/builtin/submodule--helper.c\n\t+++ b/builtin/submodule--helper.c\n\t@@ -552,7 +552,7 @@ static int module_init(int argc, const char **argv, const char *prefix)\n\t \t * If there are no path args and submodule.active is set then,\n\t \t * by default, only initialize 'active' modules.\n\t \t */\n\t-\tif (!argc && git_config_get_value_multi(\"submodule.active\")->nr)\n\t+\tif (!argc && !git_config_lookup_value(\"submodule.active\"))\n\t \t\tmodule_list_active(&list);\n\nYou changed these in your 2/5, but they really just wanted the new \"does\nthis key exist?\" API. No need to construct the string_list just to throw\nit away...\n\t \n\t \tinfo.prefix = prefix;\n\t@@ -2720,7 +2720,7 @@ static int module_update(int argc, const char **argv, const char *prefix)\n\t \t\t * If there are no path args and submodule.active is set then,\n\t \t\t * by default, only initialize 'active' modules.\n\t \t\t */\n\t-\t\tif (!argc && git_config_get_value_multi(\"submodule.active\")->nr)\n\t+\t\tif (!argc && !git_config_lookup_value(\"submodule.active\"))\n\t \t\t\tmodule_list_active(&list);\n\nDitto.\n\t \n\t \t\tinfo.prefix = opt.prefix;\n\t@@ -3164,7 +3164,6 @@ static int config_submodule_in_gitmodules(const char *name, const char *var, con\n\t static void configure_added_submodule(struct add_data *add_data)\n\t {\n\t \tchar *key;\n\t-\tconst char *val;\n\t \tstruct child_process add_submod = CHILD_PROCESS_INIT;\n\t \tstruct child_process add_gitmodules = CHILD_PROCESS_INIT;\n\t \n\t@@ -3209,7 +3208,7 @@ static void configure_added_submodule(struct add_data *add_data)\n\t \t * is_submodule_active(), since that function needs to find\n\t \t * out the value of \"submodule.active\" again anyway.\n\t \t */\n\t-\tif (!git_config_get_string_tmp(\"submodule.active\", &val)) {\n\t+\tif (!git_config_lookup_value(\"submodule.active\")) {\n\t \t\t/*\n\t \t\t * If the submodule being added isn't already covered by the\n\t \t\t * current configured pathspec, set the submodule's active flag\n\nDitto.\n\n\tdiff --git a/submodule.c b/submodule.c\n\tindex 06230961c80..4474cf9ed2d 100644\n\t--- a/submodule.c\n\t+++ b/submodule.c\n\t@@ -274,8 +274,7 @@ int is_tree_submodule_active(struct repository *repo,\n\t \tfree(key);\n\t \n\t \t/* submodule.active is set */\n\t-\tsl = repo_config_get_value_multi(repo, \"submodule.active\");\n\t-\tif (sl && sl->nr) {\n\t+\tif (!repo_config_get_const_value_multi(repo, \"submodule.active\", &sl)) {\n\t \t\tstruct pathspec ps;\n\t \t\tstruct strvec args = STRVEC_INIT;\n\t \t\tconst struct string_list_item *item;\n\nAnother \"*the* API difference we're discussing\". I.e. sure, your end\nstate would be \"if (sl->nr)\", but if we make it return \"int\"...\n\n\tdiff --git a/t/helper/test-config.c b/t/helper/test-config.c\n\tindex 90810946783..432ad047537 100644\n\t--- a/t/helper/test-config.c\n\t+++ b/t/helper/test-config.c\n\t@@ -95,8 +95,7 @@ int cmd__config(int argc, const char **argv)\n\t \t\t\tgoto exit1;\n\t \t\t}\n\t \t} else if (argc == 3 && !strcmp(argv[1], \"get_value_multi\")) {\n\t-\t\tstrptr = git_config_get_value_multi(argv[2]);\n\t-\t\tif (strptr->nr) {\n\t+\t\tif (!git_config_get_const_value_multi(argv[2], &strptr)) {\n\t \t\t\tfor (i = 0; i < strptr->nr; i++) {\n\t \t\t\t\tv = strptr->items[i].string;\n\t \t\t\t\tif (!v)\n\nDitto, (this one converts away from your preferred API use).\n\n\t@@ -159,8 +158,7 @@ int cmd__config(int argc, const char **argv)\n\t \t\t\t\tgoto exit2;\n\t \t\t\t}\n\t \t\t}\n\t-\t\tstrptr = git_configset_get_value_multi(&cs, argv[2]);\n\t-\t\tif (strptr && strptr->nr) {\n\t+\t\tif (!git_configset_get_const_value_multi(&cs, argv[2], &strptr)) {\n\t \t\t\tfor (i = 0; i < strptr->nr; i++) {\n\t \t\t\t\tv = strptr->items[i].string;\n\t \t\t\t\tif (!v)\n\nDitto, sans that you'd presumably want s/strptr && // here.\n\n\tdiff --git a/t/t0068-for-each-repo.sh b/t/t0068-for-each-repo.sh\n\tindex 4675e852517..115221c9ca5 100755\n\t--- a/t/t0068-for-each-repo.sh\n\t+++ b/t/t0068-for-each-repo.sh\n\t@@ -33,4 +33,10 @@ test_expect_success 'do nothing on empty config' '\n\t \tgit for-each-repo --config=bogus.config -- help --no-such-option\n\t '\n\t \n\t+test_expect_success 'error on bad config keys' '\n\t+\ttest_expect_code 129 git for-each-repo --config=a &&\n\t+\ttest_expect_code 129 git for-each-repo --config=a.b. &&\n\t+\ttest_expect_code 129 git for-each-repo --config=\"'\\''.b\"\n\t+'\n\t+\n\t test_done\n\nA test showing behavior change we can implement now that we don't sweep\nthe \"err < 0\" under the rug.\n\nThat branch also grew to have some other changes we may or may not want,\none thing was to convert the various *_get_*() functionts that now\nnormalize the non-zero return value with e.g.:\n\n\t int git_configset_get_int(struct config_set *cs, const char *key, int *dest)\n\t {\n\t const char *value;\n\t- if (!git_configset_get_value(cs, key, &value)) {\n\t- *dest = git_config_int(key, value);\n\t- return 0;\n\t- } else\n\t- return 1;\n\t+ int err;\n\t+\n\t+ if ((err = git_configset_get_value(cs, key, &value)))\n\t+ return err;\n\t+ *dest = git_config_int(key, value);\n\t+ return 0;\n\t }\n\nNo caller currently cares about it, but I think it makes sense generally\nnot to throw away errors if we can (whether that part is worth the churn\nis another topic).\n\nAnyway, the reason I started looking at this RFC to begin with was\nbecause this *_multi() part of the config API has often seemed odd to\nme, i.e. I wondered why we couldn't just have it use the \"return int,\npopulate dest\" pattern. I'd just never tried to see if I could get that\nto work.\n\nIt's a bit of one-off churn to get to this point, but I think the end\nresult of having all the API functions act the same way to signal key\nexistence v.s. validity is worth it.\n"},{"id":"463852","messageId":"YzRvHFIOG3e6Lhch@nand.local","threadId":"58518","inReplyTo":"af036388d4e5abe35c23acf4210d46cd69b2d264.1664287711.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/5] *: relax git_configset_get_value_multi result","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-09-28T15:58:20Z","receivedAt":"2022-09-28T15:59:25Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 27, 2022 at 02:08:28PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n\nShould the patch message be:\n\n *: relax git_config_get_value_multi() result\n\ninstead of referring to git_configset_get_value_multi?\n\n> ---\n> builtin/submodule--helper.c | 10 ++++++++--\n> submodule.c | 2 +-\n> t/helper/test-config.c | 4 ++--\n> versioncmp.c | 8 ++++----\n> 4 files changed, 15 insertions(+), 9 deletions(-)\n\nAll looks good here. I did a git grep for git_config_get_value_multi()\nto make sure all of those spots were covered here. No comments from me,\nother than a little thinking aloud below on a couple of the conversions.\n\n> @@ -552,7 +553,9 @@ static int module_init(int argc, const char **argv, const char *prefix)\n> \t * If there are no path args and submodule.active is set then,\n> \t * by default, only initialize 'active' modules.\n> \t */\n> -\tif (!argc && git_config_get_value_multi(\"submodule.active\"))\n> +\tif (!argc &&\n> +\t (active_modules = git_config_get_value_multi(\"submodule.active\")) &&\n> +\t active_modules->nr)\n\nYuck ;-). I was going to suggest that the addition of a helper function\nsomething like:\n\n static int any_configured(const char *key)\n {\n const struct string_list *vals = git_configset_get_value_multi(key);\n return vals && vals->nr;\n }\n\nwould be worthwhile for this and the below conversion, but the change at\nthe end of this series makes it a moot point. So the temporary eye-sore\nis just fine.\n\n> diff --git a/t/helper/test-config.c b/t/helper/test-config.c\n> index 4ba9eb65606..62644dd71d7 100644\n> --- a/t/helper/test-config.c\n> +++ b/t/helper/test-config.c\n> @@ -96,7 +96,7 @@ int cmd__config(int argc, const char **argv)\n> \t\t}\n> \t} else if (argc == 3 && !strcmp(argv[1], \"get_value_multi\")) {\n> \t\tstrptr = git_config_get_value_multi(argv[2]);\n> -\t\tif (strptr) {\n> +\t\tif (strptr && strptr->nr) {\n> \t\t\tfor (i = 0; i < strptr->nr; i++) {\n> \t\t\t\tv = strptr->items[i].string;\n> \t\t\t\tif (!v)\n\nGood catch. I was thinking that this whole \"if\" statement could have\nbeen dropped, but the goto that is hidden by the context meant that this\n*would* have been a behavior change otherwise. So the conversion here is\nappropriate.\n\nThanks,\nTaylor\n"},{"id":"463869","messageId":"31ceb36b-3c60-c4c2-c6fb-3f602eb9d4ea@github.com","threadId":"58518","inReplyTo":"220928.86y1u3wnaz.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 4/5] config: return an empty list, not NULL","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-28T18:10:04Z","receivedAt":"2022-09-28T18:10:12Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/28/2022 10:37 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Sep 28 2022, Derrick Stolee wrote:\n\n>> If we return a negative value on an error and the number of matches on\n>> success, then this change could instead be \"if (repo_config....() > 0)\".\n> \n> Hrm, I think you're confusing the worldview your series here is\n> advocating for, and what I'm suggesting as an alternative.\n> \n> There isn't any way on \"master\" to have \"an empty list\", that's a\n> worldview you're proposing. In particular your 1/5 here removes:\n> \n> \tassert(values->nr > 0);\n\nYes, I think the lack of a key should be the same to an empty list\nbecause it is logically equivalent and an empty list is safer to use\nthan a possibly-NULL list. That's what started this whole investigation.\n\nBy no longer returning NULL, we can eliminate an entire class of bugs\nfrom happening.\n\n> More generally the config format has no notion of \"an empty list\", if\n> you have a valid key-value pair at all you have a list of \".nr >= 1\".\n\nThe critical part is that you have a return code that has three modes:\n\n 1. Negative: some kind of parsing error happened, such as a malformed\n 'key' parameter.\n\n 2. Zero: The 'key' was found with multiple values.\n\n 3. Positive: The 'key' was not found. (Are there other innocuous\n errors that fit in this case?)\n\nThis \"positive means not found\" is very non-obvious.\n\nEven with your goal of exposing those parsing errors when the 'key' is\nuser-supplied, I still think it would be better to provide a non-NULL\nempty string_list and only care about these return values:\n\n 1. Negative: some kind of parsing error happened.\n\n 2. Nonnegative: parsing succeeded. The string_list has all found values\n (might be empty) and the return value is equal to the string_list's\n 'nr' member.\n\nIn these cases, I see two modes of use.\n\nFirst, check for an error and exit early (empty list no-ops naturally):\n\n\tif (git_config_get_const_value_multi(key, &list) < 0)\n\t\treturn -1;\n\n\tfor_each_string_list_item(item, list) {\n\t\t...\n\t}\n\nSecond, ignore errors. Care about non-empty list:\n\n\tif (git_config_get_const_value_multi(\"known.key\", &list) > 0) {\n\t\t...\n\t}\n\nBut this is just a matter of taste at this point, and I'm getting the\nfeeling that my taste for reducing the chances of NULL references is\nnot very popular.\n\nThanks,\n-Stolee\n"},{"id":"463871","messageId":"35f6d380-f7a5-558d-1cfc-f5eb6ea6c818@github.com","threadId":"58518","inReplyTo":"xmqq35cc1arp.fsf@gitster.g","subject":"Re: [PATCH 0/5] [RFC] config API: return empty list, not NULL","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-28T18:38:21Z","receivedAt":"2022-09-28T18:38:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/27/2022 10:40 PM, Junio C Hamano wrote:\n> \"Derrick Stolee via GitGitGadget\" writes:\n> \n>> This work changes the behavior of asking for a multi-valued config key to\n>> return an empty list instead of a NULL value. This simplifies the handling\n>> of the result and is safer for development in the future.\n>>\n>> This is based on v4 of my unregister series [1]\n>>\n>> [1]\n>> https://lore.kernel.org/git/pull.1358.v4.git.1664287021.gitgitgadget@gmail.com/\n>>\n>> This idea came about due to a bug in the git maintenance unregister work\n>> where the result from git_config_get_value_multi() was sent directly to\n>> for_each_string_list_item() without checking for a NULL value first.\n>>\n>> I'm sending this as an RFC mostly because I'm not 100% sure this shift is\n>> worth the refactoring pain and effort. I personally think getting an empty\n>> list is a safer choice, but I can also understand if someone has a different\n>> opinion.\n> \n> Thanks.\n> \n> I actually am in favor of the idea that a NULL can be passed around\n> to signal the lack of a string_list (or the lack of a instance of\n> any \"collection\" type), and the current code is structured as such,\n> and it gives us extra flexibility. Of course, we need to see if\n> that extra flexibility is worth it.\n> \n> With a colleciton col, \"if (col && col->nr)\" checks if we have\n> something to work on. But a code like this (which is a longhand for\n> the for_each_string_list_item() issue we just reencountered):\n> \n> col = git_get_some_collection(...);\n> if (!col)\n> \treturn; /* no collection */\n> if (!col->nr)\n> \tgit_add_to_some_collection(col, the default item);\n> for (i = 0; i < col->nr; i++)\n> \tdo things on col.stuff[i];\n> \n> can react differently to cases where we have an empty collection\n> and where we do not have any collection to begin with. \n> \n> The other side of the coin is that it would make it harder to treat\n> the lack of collection itself and the collection being empty the\n> same way. The above code might need to become\n> \n> col = git_get_some_collection(...);\n> if (!col)\n> \tcol = git_get_empty_collection();\n> if (!col->nr)\n> \tgit_add_to_some_collection(col, the default item);\n> for (i = 0; i < col->nr; i++)\n> \tdo things on col.stuff[i];\n> \n> but if the \"get the collection\" thing returns an empty collection\n> when there is actually no collection, we can lose two lines from\n> there.\n\nI'm all for conveying more information when possible, but how can\nthe config API provide a distinction between an empty list and a\nNULL list? The only thing I can think about is a case where the\nempty value clears the list and no new values are added, such as\n\n\t[bogus \"key\"]\n\t\titem = one\n\t\titem = two\n\t\titem =\n\nWith this, the key exists in the config file, but the multi-valued\nlist is empty. Is that an important distinction? I don't think so.\n\n> Between these two positions, I am in favor of the flexibility that\n> we can use NULL to signal the lack of collection, not a presence of\n> a collection with zero items, as it feels conceptually cleaner.\n> \n> Counting the hunks in [2/5] and [5/5], it seems that \"when no\n> variable with given key is defined, we return an empty set\" makes us\n> to have more code in 7 places in [PATCH 2/5], while allowing us to\n> lose code in 10 places in [PATCH 5/5].\n\nOutside of the \"if (sl && sl->nr) {\" that I forgot to convert into\n\"if (sl->nr) {\" in patch 5, all of those 7 places that have \"more code\"\nend up with only that extra \"->nr\". The code looks uglier temporarily\nin patch 2 to create the compatibility mode so patch 4 can change only\nthe config API without test failures.\n\nThanks,\n-Stolee\n"},{"id":"463873","messageId":"220928.86czbfwb18.gmgdl@evledraar.gmail.com","threadId":"58518","inReplyTo":"35f6d380-f7a5-558d-1cfc-f5eb6ea6c818@github.com","subject":"Re: [PATCH 0/5] [RFC] config API: return empty list, not NULL","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-28T19:27:56Z","receivedAt":"2022-09-28T19:32:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 28 2022, Derrick Stolee wrote:\n\n> On 9/27/2022 10:40 PM, Junio C Hamano wrote:\n>> \"Derrick Stolee via GitGitGadget\" writes:\n>> \n>>> This work changes the behavior of asking for a multi-valued config key to\n>>> return an empty list instead of a NULL value. This simplifies the handling\n>>> of the result and is safer for development in the future.\n>>>\n>>> This is based on v4 of my unregister series [1]\n>>>\n>>> [1]\n>>> https://lore.kernel.org/git/pull.1358.v4.git.1664287021.gitgitgadget@gmail.com/\n>>>\n>>> This idea came about due to a bug in the git maintenance unregister work\n>>> where the result from git_config_get_value_multi() was sent directly to\n>>> for_each_string_list_item() without checking for a NULL value first.\n>>>\n>>> I'm sending this as an RFC mostly because I'm not 100% sure this shift is\n>>> worth the refactoring pain and effort. I personally think getting an empty\n>>> list is a safer choice, but I can also understand if someone has a different\n>>> opinion.\n>> \n>> Thanks.\n>> \n>> I actually am in favor of the idea that a NULL can be passed around\n>> to signal the lack of a string_list (or the lack of a instance of\n>> any \"collection\" type), and the current code is structured as such,\n>> and it gives us extra flexibility. Of course, we need to see if\n>> that extra flexibility is worth it.\n>> \n>> With a colleciton col, \"if (col && col->nr)\" checks if we have\n>> something to work on. But a code like this (which is a longhand for\n>> the for_each_string_list_item() issue we just reencountered):\n>> \n>> col = git_get_some_collection(...);\n>> if (!col)\n>> \treturn; /* no collection */\n>> if (!col->nr)\n>> \tgit_add_to_some_collection(col, the default item);\n>> for (i = 0; i < col->nr; i++)\n>> \tdo things on col.stuff[i];\n>> \n>> can react differently to cases where we have an empty collection\n>> and where we do not have any collection to begin with. \n>> \n>> The other side of the coin is that it would make it harder to treat\n>> the lack of collection itself and the collection being empty the\n>> same way. The above code might need to become\n>> \n>> col = git_get_some_collection(...);\n>> if (!col)\n>> \tcol = git_get_empty_collection();\n>> if (!col->nr)\n>> \tgit_add_to_some_collection(col, the default item);\n>> for (i = 0; i < col->nr; i++)\n>> \tdo things on col.stuff[i];\n>> \n>> but if the \"get the collection\" thing returns an empty collection\n>> when there is actually no collection, we can lose two lines from\n>> there.\n>\n> I'm all for conveying more information when possible, but how can\n> the config API provide a distinction between an empty list and a\n> NULL list? The only thing I can think about is a case where the\n> empty value clears the list and no new values are added, such as\n>\n> \t[bogus \"key\"]\n> \t\titem = one\n> \t\titem = two\n> \t\titem =\n>\n> With this, the key exists in the config file, but the multi-valued\n> list is empty.\n\nIt's not empty, that's a list with three items: [\"one\", \"two\", \"\"], the\nlast one is just the empty string.\n\nWe then have some stateful parsing logic for individual keys that\ndecides to apply the business logic that an empty value clears the list,\nbut none of that's implemented as part of the config API itself.\n\nI think we might want to create some helper function for such a\nspecial-cased \"multi\" list, but...\n\n> Is that an important distinction? I don't think so.\n\n...even then no, I don't think there should be a distinction. It\nshouldn't be an \"empty list\" in the \"list.nr == 0\" sense if the config\nAPI understood that sort of construct natively, it should just act as\nthough the key wasn't specified, surely...\n"},{"id":"463874","messageId":"220928.868rm3w9d4.gmgdl@evledraar.gmail.com","threadId":"58518","inReplyTo":"31ceb36b-3c60-c4c2-c6fb-3f602eb9d4ea@github.com","subject":"Re: [PATCH 4/5] config: return an empty list, not NULL","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-28T19:33:44Z","receivedAt":"2022-09-28T20:07:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 28 2022, Derrick Stolee wrote:\n\n> On 9/28/2022 10:37 AM, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Wed, Sep 28 2022, Derrick Stolee wrote:\n>\n>>> If we return a negative value on an error and the number of matches on\n>>> success, then this change could instead be \"if (repo_config....() > 0)\".\n>> \n>> Hrm, I think you're confusing the worldview your series here is\n>> advocating for, and what I'm suggesting as an alternative.\n>> \n>> There isn't any way on \"master\" to have \"an empty list\", that's a\n>> worldview you're proposing. In particular your 1/5 here removes:\n>> \n>> \tassert(values->nr > 0);\n>\n> Yes, I think the lack of a key should be the same to an empty list\n> because it is logically equivalent and an empty list is safer to use\n> than a possibly-NULL list. That's what started this whole investigation.\n>\n> By no longer returning NULL, we can eliminate an entire class of bugs\n> from happening.\n\nThis saga was started because this code had a segfault:\n\n\tconst struct string_list *list = git_config_get_value_multi(key);\n\tfor_each_string_list_item(item, list) { ... found = 1 ... }\n\nIn general I completely agree with you that APIs should be safe by\ndefault, especially when it comes to NULL. E.g. it's *very handy* that\nthe \"buf\" member of \"struct strbuf\" is never NULL.\n\nBut I just don't see it in this case, i.e. the alternative once we make\n*_multi() act like the rest of the config API is:\n\n\tif (!git_config_get_const_value_multi(key, &list))\n\t\tfor_each_string_list_item(item, list) { ... found = 1 ... }\n\nOr, well, in my version:\n\n\tif (!git_config_get_const_value_multi(key, &list))\n\t\tfound = unsorted_string_list_has_string(list, maintpath);\n\nBut that unsorted_string_list_has_string() pattern could also be used in\nthe former.\n\nI've found this \"if I have this data, then\" behavior of the config API\nto be very handy & safe to use, we just haven't used it for this one API\nentry point.\n\n>> More generally the config format has no notion of \"an empty list\", if\n>> you have a valid key-value pair at all you have a list of \".nr >= 1\".\n>\n> The critical part is that you have a return code that has three modes:\n>\n> 1. Negative: some kind of parsing error happened, such as a malformed\n> 'key' parameter.\n\nCritically not \"some kind of parsing error\", but \"malformed key error\",\nwe've already parsed the config itself at this point.\n\n> 2. Zero: The 'key' was found with multiple values.\n\nWell, found at all, maybe it has just one value.\n\n> 3. Positive: The 'key' was not found. (Are there other innocuous\n> errors that fit in this case?)\n\nIt's just \"not found\", nothing else (there's no \"return 2;\" etc).\n\nBut in terms of simple API use if your key is a constant string\n(i.e. the ones where we hardcode the key in the C source) we just need\nto handle cases #2 and #3, hence why I added that _*const_value_multi()\nwrapper.\n\nOf course it's possible to get an error on\ngit_config_get_const_value_multi(\"mal.formed.key.\", but we'd BUG() out\non that in development, and once we test it once we know the hardcoded\nkey is valid. So as an API it makes sense not to worry about it, but\njust BUG() out.\n\nYou only need to handle this \"malformed key\" case on runtime in\ngit-config(1) itself, and e.g. \"git for-each-repo --config=\", where\nwe get the key from the user\n\n> This \"positive means not found\" is very non-obvious.\n\nIs it? I find it quite idiomatic in C, but yeah, it would be odd in some\nother contexts and/or languages.\n\nIn this case though every other function in the config API works that\nway, i.e.:\n\n\tgit grep 'if \\(!.*config_get_'\n\nSo whatever our desires if it were a clean-room API (where a key\nexisting might return 1), surely consistency would win out at this\npoint.\n\n> Even with your goal of exposing those parsing errors when the 'key' is\n> user-supplied, I still think it would be better to provide a non-NULL\n> empty string_list and only care about these return values:\n>\n> 1. Negative: some kind of parsing error happened.\n>\n> 2. Nonnegative: parsing succeeded. The string_list has all found values\n> (might be empty) and the return value is equal to the string_list's\n> 'nr' member.\n\nCan you show me how this sort of thing would look in your proposal:\n\n\tint bool;\n const struct string_list *list;\n int found = 0;\n\n\tif (!git_config_get_const_value_multi(\"a.list\", &list))\n\t\tfound = unsorted_string_list_has_string(list, \"needle\");\n\tif (!git_config_get_bool(\"a.bool\", &bool))\n\t\tif (bool) ...;\n\nAFAICT that would be:\n\n\tint bool;\n const struct string_list *list = git_config_get_value_multi(\"a.list\");\n int found = unsorted_string_list_has_string(list, \"needle\");\n\n\tif (!git_config_get_bool(\"a.bool\", &bool))\n\t\tif (bool) ...;\n\nI find it unintuitive to make it act differently just because it's a\ncollection, so we can get away with basically stuffing the \"existed?\" in\nthe value itself (as .nr == 0).\n\n> In these cases, I see two modes of use.\n>\n> First, check for an error and exit early (empty list no-ops naturally):\n>\n> \tif (git_config_get_const_value_multi(key, &list) < 0)\n> \t\treturn -1;\n\nNo, the _*const_value_multi() never returns < 0, that's a BUG(). So we\ndon't worry about that for the common case.\n\nAt the tip of my branch for this gc, log, versioncmp etc. all use that.\n\nWhich means that this right after:\n\n> \tfor_each_string_list_item(item, list) {\n> \t\t...\n> \t}\n\nWould be an error, we'd have a NULL list if we got a return value of 1,\nbut the common case is:\n\n\tif (!git_config_get_const_value_multi(key, &list) {\n\t\tstruct string_list_item *item;\n\n\t\tfor_each_string_list_item(item, list) ...\n\t}\n\n> Second, ignore errors. Care about non-empty list:\n>\n> \tif (git_config_get_const_value_multi(\"known.key\", &list) > 0) {\n> \t\t...\n> \t}\n\nSince that's the *_const_value_multi() not > 0, just \"!fn()\" will do,\nsince it's 0 or 1.\n\n> But this is just a matter of taste at this point, and I'm getting the\n> feeling that my taste for reducing the chances of NULL references is\n> not very popular.\n\nIt's very popular with me :)\n\nI just don't see how it's needed in *this case*, since we can entirely\nsidestep the need for a dangerous NULL value by making it act like the\nrest of the API.\n\n think the reason the bug in [1] happened is exactly because\ngit_config_get_value_multi(key) currently acts in a way where you need\nto explicitly remember to handle the NULL, *and* if you forget to do so\nit's easy to have it work.\n\nI.e. in your (initial buggy) version:\n\n\tconst struct string_list *list = git_config_get_value_multi(key);\n\n\tfor_each_string_list_item(item, list) { /* boom, \"list\" can be null */\n\nThe problem being that you can easily write that idiomatically, have it\nwork in development (because you test with that in your config), and\nthen it blows up if you don't have that config.\n\nBut that's exactly why I think the rest of the config API is well\ndesigned, because as soon as you need to write that as:\n\n\tconst struct string_list *list;\n\n\tif (!git_config_get_const_value_multi(key, &list))\n\t\tfor_each_string_list_item(item, list) { /* \"list\" can't be null /*\n\nYou're forced to structure your code in such a way that you can't really\nforget the implicit NULL check, because to make use of the API at all\nyou need an \"if I have the data, then...\" block.\n\n1. https://lore.kernel.org/git/8a8bffaec89e55da0c5bcac2f04331e0d4e69790.1664218087.git.gitgitgadget@gmail.com/\n"}]}