On Thu, Mar 10 2022, Ævar Arnfjörð Bjarmason wrote: > On Wed, Mar 09 2022, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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).
