Re: [PATCH v3 1/3] maintenance: add 'unregister --force'

From: Derrick Stolee <derrickstolee@github.com>
Date: Sep 27, 2022, 13:55 UTC
Message-ID: <30b43cee-e7fb-067f-8a84-ff1fef5444db@github.com>
In-Reply-To: <220927.86a66lylk5.gmgdl@evledraar.gmail.com>

On 9/27/2022 9:36 AM, Ævar Arnfjörð Bjarmason wrote:

Show 16 quoted lines

> 
> On Tue, Sep 27 2022, Derrick Stolee wrote:
>> Of course, there is a reason why we don't check for NULL here,
>> and it's because -Werror=address complains when we use a non-pointer
>> value in the macro:
>>
>> string-list.h:146:28: error: the address of ‘friendly_ref_names’ will always evaluate as ‘true’ [-Werror=address]
>>   146 |         for (item = (list) ? (list)->items : NULL;      \
>>       |
>>
>> I tried searching for a way to suppress this error in a particular
>> case like this (perhaps using something like an attribute?), but I
>> couldn't find anything.
> 
> We discussed this exact issue just a few months ago, see:
> https://lore.kernel.org/git/220614.86czfcytlz.gmgdl@evledraar.gmail.com/

Thanks for finding this thread. I knew it was vaguely familiar.

Show 17 quoted lines

> In general I don't think we should be teaching
> for_each_string_list_item() to handle NULL.
> 
> Instead most callers that need to deal with a "NULL" list should
> probably just use a list that's never NULL. See:
> https://lore.kernel.org/git/220616.86bkuswuh5.gmgdl@evledraar.gmail.com/
> 
> In this case however it seems perfectly reasonable to return a valid
> pointer or NULL, and the function documents as much:
> 	
> 	/**
> 	 * Finds and returns the value list, sorted in order of increasing priority
> 	 * for the configuration variable `key`. When the configuration variable
> 	 * `key` is not found, returns NULL. The caller should not free or modify
> 	 * the returned pointer, as it is owned by the cache.
> 	 */
> 	const struct string_list *git_config_get_value_multi(const char *key);

It documents that it will never return an empty list, and instead will return NULL. There are several places that check that condition explicitly. Converting them is not terribly hard, though, and I'll send an RFC soon that performs that conversion.

> This also gives the reader & compiler more information to e.g. eliminate
> dead code. You're calling maintpath() unconditionally, but if you have
> no config & the user provided --force we'll never end up using it, so we
> can avoid allocating it in the first place.

While you're correct that we could avoid that allocation, it makes the code look terrible and hard to reason about, so I won't make that change.

Thanks,
-Stolee