Re: [PATCH v3 1/3] maintenance: add 'unregister --force'
- From
Derrick Stolee <derrickstolee@github.com>
- Date
- Sep 27, 2022, 11:38 UTC
- Message-ID
- <b1e6f118-046c-42f8-01a8-2543e792685a@github.com>
- In-Reply-To
- <xmqqa66lby1w.fsf@gitster.g>
On 9/26/2022 5:55 PM, Junio C Hamano wrote:
Show 18 quoted lines
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> 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.
Show 7 quoted lines
> 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