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...
>> run_diff_index(&rev, 1);
>> - object_array_clear(&rev.pending);
>> - return (rev.diffopt.flags.has_changes != 0);
>> + has_changes = rev.diffopt.flags.has_changes;
>> + release_revisions(&rev);
>> + return (has_changes != 0);
>
> This is necessary because release_revisions(&rev) is a way to
> release all resources held by rev, and .has_changes is stored
> as a part of a resource that will be cleared.
Yes, but...
>> diff --git a/revision.c b/revision.c
>> index 5824fdeddfd..831f2cf977b 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -2926,6 +2926,13 @@ static void release_revisions_commit_list(struct rev_info *revs)
>> revs->commits = NULL;
>> }
>>
>> +void release_revisions(struct rev_info *revs)
>> +{
>> + if (!revs)
>> + return;
>> + object_array_clear(&revs->pending);
>> +}
>
> Whoa.
>
> Earlier, we saw a code that indicates that a call to this function
> will invalidate the revs->diffopt.flags.has_changes but that is not
> the case, at least at this point in the series. Is this a result of
> an incorrect "rebase -i"?
>
> Regarding the clear_pathspec() earlier we saw, it is OK to leave the
> call there without any extra comment, if the plan is to first start
> by introducing release_revisions() that does nothing but .pending
> field. But then the diffopt.flags.has_changes thing we saw earlier
> should be postponed to a step where release_revisions() clears the
> diffopt structure that is embedded in rev_info.
>
>> @@ -2568,8 +2568,9 @@ int has_uncommitted_changes(struct repository *r,
>>
>> diff_setup_done(&rev_info.diffopt);
>> result = run_diff_index(&rev_info, 1);
>> - object_array_clear(&rev_info.pending);
>> - return diff_result_code(&rev_info.diffopt, result);
>> + result = diff_result_code(&rev_info.diffopt, result);
>> + release_revisions(&rev_info);
>> + return result;
>
> Ditto.
...indeed, those don't do anything yet.
I thought it better (and will explain so in a re-rolled commit message)
to start adding release_revisions() to existing API users, and not to
have those individidual users assume anything about what is and isn't
cleared yet.
Some users that end up with the release_revisions() don't need it at
all, because in those cases we just so happen not to actually allocate
anything.
But just like with strbuf_release() et al we generally just add it at
the end, and just have the destructor worry about the NOOP-ing.
So I'd prefer to keep this part of the general structure as-is,
i.e. even if we do nothing with "diffopt" *yet* we can assert that the
mere addition of the nascent release_revisions() isn't breaking anything
by merely existing as we add it to all "struct rev_info" users.
Then as we add more things to release_revisions() we get a nicely tested
before/after where we can see if/how adding new free()'s there breaks or
doesn't break.
