| Message ID | patch-09.10-aa2bec76f6e-20211004T002226Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | unpack-trees & dir APIs: fix memory leaks | expand |
On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > Refactor code added in 1c41d2805e4 (unpack_trees_options: free > messages when done, 2018-05-21) to use a ret/goto pattern, rather than > duplicating the end cleanup in the function. > > This control flow will be matched in subsequent commits by various > other similar code, which often needs to do more than just call > unpack_trees_options_release(). Let's change this to consistently use > the same pattern everywhere. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > merge.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/merge.c b/merge.c > index 2f618425aff..2e3714ccaa0 100644 > --- a/merge.c > +++ b/merge.c > @@ -54,6 +54,7 @@ int checkout_fast_forward(struct repository *r, > struct tree_desc t[MAX_UNPACK_TREES]; > int i, nr_trees = 0; > struct lock_file lock_file = LOCK_INIT; > + int ret = 0; > > refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL); > > @@ -95,12 +96,14 @@ int checkout_fast_forward(struct repository *r, > > if (unpack_trees(nr_trees, t, &opts)) { > rollback_lock_file(&lock_file); > - unpack_trees_options_release(&opts); > - return -1; > + ret = -1; > + goto cleanup; > } > - unpack_trees_options_release(&opts); > > if (write_locked_index(r->index, &lock_file, COMMIT_LOCK)) > - return error(_("unable to write new index file")); > - return 0; > + ret = error(_("unable to write new index file")); > + > +cleanup: > + unpack_trees_options_release(&opts); > + return ret; > } > -- > 2.33.0.1404.g83021034c5d I would say 'meh'...but the commit message is downright confusing. It suggests that subsequent commits, plural, will be modifying this code further. There is only one more commit in this series, and looking ahead, it doesn't even touch this file. So, there actually isn't any reason for these changes beyond what we see in this file, at least not for this series. And as far as this series is concerned, I think it's actually less readable. If there's a subsequent series that will use this and make further changes I could be convinced, but then I'd say this commit belongs as part of the later series, not this one.
On Mon, Oct 04 2021, Elijah Newren wrote: > On Sun, Oct 3, 2021 at 5:46 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: >> >> Refactor code added in 1c41d2805e4 (unpack_trees_options: free >> messages when done, 2018-05-21) to use a ret/goto pattern, rather than >> duplicating the end cleanup in the function. >> >> This control flow will be matched in subsequent commits by various >> other similar code, which often needs to do more than just call >> unpack_trees_options_release(). Let's change this to consistently use >> the same pattern everywhere. >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- >> merge.c | 13 ++++++++----- >> 1 file changed, 8 insertions(+), 5 deletions(-) >> >> diff --git a/merge.c b/merge.c >> index 2f618425aff..2e3714ccaa0 100644 >> --- a/merge.c >> +++ b/merge.c >> @@ -54,6 +54,7 @@ int checkout_fast_forward(struct repository *r, >> struct tree_desc t[MAX_UNPACK_TREES]; >> int i, nr_trees = 0; >> struct lock_file lock_file = LOCK_INIT; >> + int ret = 0; >> >> refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL); >> >> @@ -95,12 +96,14 @@ int checkout_fast_forward(struct repository *r, >> >> if (unpack_trees(nr_trees, t, &opts)) { >> rollback_lock_file(&lock_file); >> - unpack_trees_options_release(&opts); >> - return -1; >> + ret = -1; >> + goto cleanup; >> } >> - unpack_trees_options_release(&opts); >> >> if (write_locked_index(r->index, &lock_file, COMMIT_LOCK)) >> - return error(_("unable to write new index file")); >> - return 0; >> + ret = error(_("unable to write new index file")); >> + >> +cleanup: >> + unpack_trees_options_release(&opts); >> + return ret; >> } >> -- >> 2.33.0.1404.g83021034c5d > > I would say 'meh'...but the commit message is downright confusing. It > suggests that subsequent commits, plural, will be modifying this code > further. There is only one more commit in this series, and looking > ahead, it doesn't even touch this file. So, there actually isn't any > reason for these changes beyond what we see in this file, at least not > for this series. And as far as this series is concerned, I think it's > actually less readable. If there's a subsequent series that will use > this and make further changes I could be convinced, but then I'd say > this commit belongs as part of the later series, not this one. Will fix, initially had these built-in fixes split into one commit per built-in, but decided it was too verbose and squashed them all, and didn't copyedit the commit message properly.
diff --git a/merge.c b/merge.c index 2f618425aff..2e3714ccaa0 100644 --- a/merge.c +++ b/merge.c @@ -54,6 +54,7 @@ int checkout_fast_forward(struct repository *r, struct tree_desc t[MAX_UNPACK_TREES]; int i, nr_trees = 0; struct lock_file lock_file = LOCK_INIT; + int ret = 0; refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL); @@ -95,12 +96,14 @@ int checkout_fast_forward(struct repository *r, if (unpack_trees(nr_trees, t, &opts)) { rollback_lock_file(&lock_file); - unpack_trees_options_release(&opts); - return -1; + ret = -1; + goto cleanup; } - unpack_trees_options_release(&opts); if (write_locked_index(r->index, &lock_file, COMMIT_LOCK)) - return error(_("unable to write new index file")); - return 0; + ret = error(_("unable to write new index file")); + +cleanup: + unpack_trees_options_release(&opts); + return ret; }
Refactor code added in 1c41d2805e4 (unpack_trees_options: free messages when done, 2018-05-21) to use a ret/goto pattern, rather than duplicating the end cleanup in the function. This control flow will be matched in subsequent commits by various other similar code, which often needs to do more than just call unpack_trees_options_release(). Let's change this to consistently use the same pattern everywhere. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- merge.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)