Re: [PATCH 05/24] revision.[ch]: provide and start using a release_revisions()

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Mar 10, 2022, 00:28 UTC
Message-ID: <220310.864k46lk7x.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqqmthylonj.fsf@gitster.g>

On Wed, Mar 09 2022, Junio C Hamano wrote:

Show 27 quoted lines

> Æ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...

Show 10 quoted lines

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

Show 40 quoted lines

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