From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mon, 04 Apr 2022 09:19:57 GMT
Subject: Re: [PATCH v5 17/27] revisions API: have release_revisions() release "mailmap"
Message-ID: <ce2c72e5-f172-b017-e62d-2eebc5ba1631@gmail.com>
In-Reply-To: <220403.86ilrqmflb.gmgdl@evledraar.gmail.com>

Hi Ævar

On 03/04/2022 14:57, Ævar Arnfjörð Bjarmason wrote:
> [...]
>>>    	av[++ac] = buf.buf;
>>>    	av[++ac] = NULL;
>>>    	setup_revisions(ac, av, &revs, NULL);
>>> -	revs.mailmap = &mailmap;
>>> +	revs.mailmap = xmalloc(sizeof(struct string_list));
>>> +	string_list_init_nodup(revs.mailmap);
>>
>> This is a common pattern in one of the previous patches, is it worth
>> adding helpers to allocate and initialize a struct string_list? Maybe
>> string_list_new_nodup() and string_list_new_dup().
> 
> Maybe, but generally in the git codebase things malloc and then init(),
> if we're going to add something like this *_new() that would be a change
> for a lot more APIs than just mailmap.
> 
> And if it's just for mailmap I don't see how the inconsistency with
> other code would be worth it.

It's for struct string_list not for mailmap and could be used for all 
the conversions in patch 3. In general the split between allocation and 
initialization is useful because it allows us to allocate structures on 
the stack where possible or embed them in other structures. However if 
there is a structure that is often allocated on the heap then I don't 
think there is anything wrong with having a combined 
allocate-and-initialize helper function which makes the code shorter and 
eliminates the possibility of passing the wrong size to malloc().

>>> diff --git a/revision.c b/revision.c
>>> index 553f7de8250..622f0faecc4 100644
>>> --- a/revision.c
>>> +++ b/revision.c
>>> @@ -2926,10 +2926,19 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>>>    	return left;
>>>    }
>>>    +static void release_revisions_mailmap(struct string_list
>>> *mailmap)
>>> +{
>>> +	if (!mailmap)
>>> +		return;
>>> +	clear_mailmap(mailmap);
>>> +	free(mailmap);
>>> +}
>>
>> It's not a big issue but if there are no other users of this then it
>> could just go inside release_revisions, my impression is that this
>> series builds a collection of very small functions whose only caller
>> is release_revisions()
> 
> Yes, these are just trivial static helpers so that each line in
> release_revisions() corresponds to a member of the struct, without
> loops, indentation for "don't free this" etc.

Fair enough

> To the machine code it makes no difference at higher optimization
> levels.

Indeed, not that these functions are particularly performance sensitive 
in the first place.

Best Wishes

Phillip