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

From: Junio C Hamano <gitster@pobox.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, "Jeff King" <peff@peff.net>,
	"Martin Ågren" <martin.agren@gmail.com>,
	"Phillip Wood" <phillip.wood123@gmail.com>
Subject: Re: [PATCH v2 4/5] builtin/remote.c: add and use a REF_STATES_INIT
Date: Mon, 27 Sep 2021 16:56:44 -0700	[thread overview]
Message-ID: <xmqqv92lmv2b.fsf@gitster.g> (raw)
In-Reply-To: <87v92lzihd.fsf@evledraar.gmail.com> (=?utf-8?B?IsOGdmFyIEFy?= =?utf-8?B?bmZqw7Zyw7A=?= Bjarmason"'s message of "Tue, 28 Sep 2021 01:38:16 +0200")

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> 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;

No, I am not talking about ownership of "foo" itself.  What changes
is ownership of some of the string_list embedded in "foo" that used
NOT to have strdup_strings member set now have the bit set, so the
string list owns the string.

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

OK, get_remotes_ref_states() marked all string list to the dup
variant, then we won't have a problem.  I was trying to make sure
that was something you looked at when preparing these patches.

Thanks.