On 9/26/2022 5:55 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> @@ -1538,11 +1546,23 @@ static int maintenance_unregister(int argc, const char **argv, const char *prefi
>> usage_with_options(builtin_maintenance_unregister_usage,
>> options);
>>
>> - config_unset.git_cmd = 1;
>> - strvec_pushl(&config_unset.args, "config", "--global", "--unset",
>> - "--fixed-value", "maintenance.repo", maintpath, NULL);
>> + for_each_string_list_item(item, list) {
>> + if (!strcmp(maintpath, item->string)) {
>> + found = 1;
>> + break;
>> + }
>> + }
>
> Just out of curiosity, I ran this in a fresh repository and got a
> segfault.
Yikes! Thanks for catching. I think there was another instance in
the 'register' code that I caught by tests, but I appreciate you
catching this one.
> An attached patch obviously fixes it, but I am wondering
> if a better "fix" is to teach for_each_string_list_item() that it is
> perfectly reasonable to see a NULL passed to it as the list, which
> is a mere special case that the caller has a string list with 0
> items on it.
>
> Thoughts?
I agree that for_each_string_list_item() could handle NULL lists
better, especially because it looks like a method and hides some
details. Plus, wrapping the for-ish loop with an if statement is
particularly ugly.
I think this might be more confusing that
git_configset_get_value_multi() can return a NULL list instead of
an empty list. It boils down to this diff:
diff --git a/config.c b/config.c
index cbb5a3bab74..39cc0534170 100644
--- a/config.c
+++ b/config.c
@@ -2416,8 +2416,9 @@ int git_configset_get_value(struct config_set *cs, const char *key, const char *
const struct string_list *git_configset_get_value_multi(struct config_set *cs, const char *key)
{
+ static struct string_list empty_list = STRING_LIST_INIT_NODUP;
struct config_set_element *e = configset_find_element(cs, key);
- return e ? &e->value_list : NULL;
+ return e ? &e->value_list : &empty_list;
}
int git_configset_get_string(struct config_set *cs, const char *key, char **dest)
However, this change causes a lot of cascading failures that
are probably not worth tracking down.
I'll get a patch put together that changes the behavior of
for_each_string_list_item() and adds the missing 'unregister' test
so we can avoid this problem.
Thanks,
-Stolee
