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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Sep 27, 2021, 23:38 UTC
Message-ID: <87v92lzihd.fsf@evledraar.gmail.com>
In-Reply-To: <xmqq4ka5oc1k.fsf@gitster.g>

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

Show 58 quoted lines

> Æ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.

Show 14 quoted lines

>> -	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".