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

From: Junio C Hamano <[email-masked]>
Date: Wed, 09 Mar 2022 14:57:52 -0800

Ævar Arnfjörð Bjarmason  <[email-masked]> 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.

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

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