| Message ID | patch-v2-05.27-4c0718b43d7-20220323T203149Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | revision.[ch]: add and use release_revisions() | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > +static void release_revisions_commit_list(struct rev_info *revs) > +{ > + struct commit_list *commits = revs->commits; > + > + if (!commits) > + return; > + free_commit_list(commits); > + revs->commits = NULL; > +} It makes sense to have this as a separate helper, but the original implementation this was lifted from is much easier to follow than this version with an unnecessary rewrite, I would think. > @@ -4080,10 +4090,7 @@ static void create_boundary_commit_list(struct rev_info *revs) > * boundary commits anyway. (This is what the code has always > * done.) > */ > - if (revs->commits) { > - free_commit_list(revs->commits); > - revs->commits = NULL; > - } > + release_revisions_commit_list(revs); IOW static void release_revisions_commit_list(struct rev_info *revs) { if (revs->commits) { free_commit_list(revs->commits); revs->commits = NULL; } }
On Wed, Mar 23 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> +static void release_revisions_commit_list(struct rev_info *revs) >> +{ >> + struct commit_list *commits = revs->commits; >> + >> + if (!commits) >> + return; >> + free_commit_list(commits); >> + revs->commits = NULL; >> +} > > It makes sense to have this as a separate helper, but the original > implementation this was lifted from is much easier to follow than > this version with an unnecessary rewrite, I would think. > >> @@ -4080,10 +4090,7 @@ static void create_boundary_commit_list(struct rev_info *revs) >> * boundary commits anyway. (This is what the code has always >> * done.) >> */ >> - if (revs->commits) { >> - free_commit_list(revs->commits); >> - revs->commits = NULL; >> - } >> + release_revisions_commit_list(revs); > > IOW > > static void release_revisions_commit_list(struct rev_info *revs) > { > if (revs->commits) { > free_commit_list(revs->commits); > revs->commits = NULL; > } > } Sure, will change it. I think the pre-image is preferrable in context, i.e. to have them all use the same early abort pattern, but not enough to argue the point :)
diff --git a/revision.c b/revision.c index 2646b78990e..303d1188207 100644 --- a/revision.c +++ b/revision.c @@ -2923,6 +2923,16 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s return left; } +static void release_revisions_commit_list(struct rev_info *revs) +{ + struct commit_list *commits = revs->commits; + + if (!commits) + return; + free_commit_list(commits); + revs->commits = NULL; +} + static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child) { struct commit_list *l = xcalloc(1, sizeof(*l)); @@ -4080,10 +4090,7 @@ static void create_boundary_commit_list(struct rev_info *revs) * boundary commits anyway. (This is what the code has always * done.) */ - if (revs->commits) { - free_commit_list(revs->commits); - revs->commits = NULL; - } + release_revisions_commit_list(revs); /* * Put all of the actual boundary commits from revs->boundary_commits
Move the existing code for invoking free_commit_list() and setting revs->commits to NULL into a new release_revisions_commit_list() function. This will be used as part of a general free()-ing mechanism for "struct rev_info". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- revision.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)