From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Thu, 10 Mar 2022 14:56:32 GMT
Subject: Re: [PATCH 05/24] revision.[ch]: provide and start using a release_revisions()
Message-ID: <220310.861qz9kg8m.gmgdl@evledraar.gmail.com>
In-Reply-To: <220310.864k46lk7x.gmgdl@evledraar.gmail.com>


On Thu, Mar 10 2022, Ævar Arnfjörð Bjarmason wrote:

> On Wed, Mar 09 2022, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>>
>>> diff --git a/builtin/checkout.c b/builtin/checkout.c
>>> index 9244827ca02..ed993383531 100644
>>> --- a/builtin/checkout.c
>>> +++ b/builtin/checkout.c
>>> @@ -629,7 +629,7 @@ static void show_local_changes(struct object *head,
>>>  	diff_setup_done(&rev.diffopt);
>>>  	add_pending_object(&rev, head, NULL);
>>>  	run_diff_index(&rev, 0);
>>> -	object_array_clear(&rev.pending);
>>> +	release_revisions(&rev);
>>>  }
>>
>> I very much like this.
>>
>>> -	object_array_clear(&rev.pending);
>>>  	clear_pathspec(&rev.prune_data);
>>> +	release_revisions(&rev);
>>
>> But this makes readers wonder why .prune_data still needs a separate
>> call to clear.  We are making it unnecessary to clear .pending
>> separately, which was what made me say I very much like this in the
>> first place.
>>
>> At least surviving clear_pathspec() now deserves an in-code comment?
>> We'll know when we see what release_revisions() actually does.
>
> I'll note it in a commit message. Generally in this series I then loop
> back and remove the clear_pathspec() (or similar) once I start clearing
> "prune_data", but in this case I seem to have missed this specific case
> later in the series...

Just for completeness, I didn't miss a case here, but thought I did
because of the truncated diff mentioning "builtin/checkout.c",
nevermind... (but will fix the other raised issue).