git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Derrick Stolee <derrickstolee@github.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>,
	git@vger.kernel.org, vdye@github.com,
	"SZEDER Gábor" <szeder.dev@gmail.com>
Subject: Re: [PATCH v3 1/3] maintenance: add 'unregister --force'
Date: Tue, 27 Sep 2022 09:55:50 -0400	[thread overview]
Message-ID: <30b43cee-e7fb-067f-8a84-ff1fef5444db@github.com> (raw)
In-Reply-To: <220927.86a66lylk5.gmgdl@evledraar.gmail.com>

On 9/27/2022 9:36 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> 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.
 
> 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
  

  reply	other threads:[~2022-09-27 13:56 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-09-20  1:02 [PATCH] maintenance: make unregister idempotent Derrick Stolee via GitGitGadget
2022-09-21 17:19 ` Junio C Hamano
2022-09-22 12:37   ` Derrick Stolee
2022-09-22 19:31     ` Junio C Hamano
2022-09-22 19:46       ` Derrick Stolee
2022-09-22 20:44         ` Junio C Hamano
2022-09-22 13:37 ` [PATCH v2 0/2] scalar: " Derrick Stolee via GitGitGadget
2022-09-22 13:37   ` [PATCH v2 1/2] maintenance: add 'unregister --force' Derrick Stolee via GitGitGadget
2022-09-23 13:08     ` SZEDER Gábor
2022-09-26 13:32       ` Derrick Stolee
2022-09-26 15:39         ` Ævar Arnfjörð Bjarmason
2022-09-26 17:25           ` Derrick Stolee
2022-09-26 19:17             ` Junio C Hamano
2022-09-22 13:37   ` [PATCH v2 2/2] scalar: make 'unregister' idempotent Derrick Stolee via GitGitGadget
2022-09-26 18:48   ` [PATCH v3 0/3] scalar: make unregister idempotent Derrick Stolee via GitGitGadget
2022-09-26 18:48     ` [PATCH v3 1/3] maintenance: add 'unregister --force' Derrick Stolee via GitGitGadget
2022-09-26 19:23       ` Junio C Hamano
2022-09-26 20:49         ` Derrick Stolee
2022-09-26 21:55       ` Junio C Hamano
2022-09-27 11:38         ` Derrick Stolee
2022-09-27 11:54           ` Derrick Stolee
2022-09-27 13:36             ` Ævar Arnfjörð Bjarmason
2022-09-27 13:55               ` Derrick Stolee [this message]
2022-09-26 18:48     ` [PATCH v3 2/3] scalar: make 'unregister' idempotent Derrick Stolee via GitGitGadget
2022-09-26 18:48     ` [PATCH v3 3/3] gc: replace config subprocesses with API calls Derrick Stolee via GitGitGadget
2022-09-26 19:27       ` Junio C Hamano
2022-09-27 13:56     ` [PATCH v4 0/4] scalar: make unregister idempotent Derrick Stolee via GitGitGadget
2022-09-27 13:56       ` [PATCH v4 1/4] maintenance: add 'unregister --force' Derrick Stolee via GitGitGadget
2022-09-27 13:56       ` [PATCH v4 2/4] scalar: make 'unregister' idempotent Derrick Stolee via GitGitGadget
2022-09-27 13:57       ` [PATCH v4 3/4] gc: replace config subprocesses with API calls Derrick Stolee via GitGitGadget
2022-09-27 13:57       ` [PATCH v4 4/4] string-list: document iterator behavior on NULL input Derrick Stolee via GitGitGadget
2022-09-27 16:39         ` Junio C Hamano
2022-09-27 16:31       ` [PATCH v4 0/4] scalar: make unregister idempotent Junio C Hamano
2022-09-27 16:54         ` Derrick Stolee

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=30b43cee-e7fb-067f-8a84-ff1fef5444db@github.com \
    --to=derrickstolee@github.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=szeder.dev@gmail.com \
    --cc=vdye@github.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).