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