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

On Tue, Sep 27 2022, Derrick Stolee wrote:

quoted

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/ (local)

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

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/ (local)

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