Re: [PATCH 05/24] revision.[ch]: provide and start using a release_revisions()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 9, 2022, 22:57 UTC
- Message-ID
- <xmqqmthylonj.fsf@gitster.g>
- In-Reply-To
- <patch-05.24-b89dcadcc22-20220309T123321Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 11 quoted lines
> 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.
Show 6 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.
Show 14 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.
Show 9 quoted lines
> @@ -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.