Re: [PATCH v2 4/5] builtin/remote.c: add and use a REF_STATES_INIT

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Sep 27 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Use a new REF_STATES_INIT designated initializer instead of assigning
>> to the "strdup_strings" member of the previously memzero()'d version
>> of this struct.
>>
>> The pattern of assigning to "strdup_strings" dates back to
>> 211c89682ee (Make git-remote a builtin, 2008-02-29) (when it was
>> "strdup_paths"), i.e. long before we used anything like our current
>> established *_INIT patterns consistently.
>>
>> Then in e61e0cc6b70 (builtin-remote: teach show to display remote
>> HEAD, 2009-02-25) and e5dcbfd9ab7 (builtin-remote: new show output
>> style for push refspecs, 2009-02-25) we added some more of these.
>>
>> As it turns out we only initialized this struct three times, all the
>> other uses were of pointers to those initialized structs. So let's
>> initialize it in those three places, skip the memset(), and pass those
>> structs down appropriately.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  builtin/remote.c | 23 +++++++++++------------
>>  1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/builtin/remote.c b/builtin/remote.c
>> index 7f88e6ce9de..160dd954f74 100644
>> --- a/builtin/remote.c
>> +++ b/builtin/remote.c
>> @@ -344,6 +344,14 @@ struct ref_states {
>>  	int queried;
>>  };
>>  
>> +#define REF_STATES_INIT { \
>> +	.new_refs = STRING_LIST_INIT_DUP, \
>> +	.stale = STRING_LIST_INIT_DUP, \
>> +	.tracked = STRING_LIST_INIT_DUP, \
>> +	.heads = STRING_LIST_INIT_DUP, \
>> +	.push = STRING_LIST_INIT_DUP, \
>> +}
>
> So, now everybody owns the string, but ...
>
>>  static int get_ref_states(const struct ref *remote_refs, struct ref_states *states)
>>  {
>>  	struct ref *fetch_map = NULL, **tail = &fetch_map;
>> @@ -355,9 +363,6 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
>>  			die(_("Could not get fetch map for refspec %s"),
>>  				states->remote->fetch.raw[i]);
>>  
>> -	states->new_refs.strdup_strings = 1;
>> -	states->tracked.strdup_strings = 1;
>> -	states->stale.strdup_strings = 1;
>
> ... we used to set up selectively to own.
>
> How would we make sure after this change we are not adding leaks?
> Is there a way to do so mechanically?

I manual run with SANITIZE=leak shows the same amount of memory leakage
before & after this change for me.

We should fix those leaks, but until my "tests: add a test mode for
SANITIZE=leak, run it in CI" lands on master I'm still waiting on fixing
those, and this series will help make it easier.

The ownership of the "states" struct or its lifetime isn't different
after this change. It's only that we're doing:

    struct foo = FOO_INIT;
    /* use &foo */

Instead of:

    struct foo;
    memset(&foo, 0, sizeof(foo));
    foo->some_list.strdup_strings = 1;

What is different is that that "strdup_strings" member in the contained
"some_list" will be set to "1" earlier. E.g. in the case of this in the
pre-image:

    states->new_refs.strdup_strings = 1;

We'd do that in get_ref_states(), which was called by
get_remote_ref_states(), which was e.g. called by show(). So before we
wouldn't flip that particular value until we got all the way down into
get_remote_ref_states(), now we initialize the struct like that on the
stack right away.

But it doesn't result in any new allocations, string_list like strbuf
etc. won't allocate until something is pushed onto the list, using the
common "nr/alloc" & malloc on demand pattern.

>> -	struct ref_states states;
>> +	struct ref_states states = REF_STATES_INIT;
>>  	struct string_list refs_to_prune = STRING_LIST_INIT_NODUP;
>>  	struct string_list_item *item;
>>  	const char *dangling_msg = dry_run
>>  		? _(" %s will become dangling!")
>>  		: _(" %s has become dangling!");
>>  
>> -	memset(&states, 0, sizeof(states));
>>  	get_remote_ref_states(remote, &states, GET_REF_STATES);
>
> Like this one, get_remote_ref_states() used to receive states that
> are set to borrow strings, but now we get duplicated strings, right?
> Are we leaking whatever strings we push to these string lists now?

Ah, yes it *could* happen as a side-effect of this sotr of change that
that memset() was implicitly flipping some string_list structs to the
equivalent of strdup_strings=0.

But that's not the case here, it was just memset() boilerplate, then in
get_remote_ref_states() we'd set all the string lists we'd use to "dup".