| Message ID | patch-v2-08.27-42ad1208934-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: > Use release_revisions() to various users of "struct rev_list" which > need to have their "struct rev_info" zero-initialized before we can > start using it. In all of these cases we might "goto cleanup" (or equivalent), I didn't look at the bisect code, but the bundle one looks iffy from the point of view of API cleanliness. If we have not yet called repo_init_revisions() on a revs, we should refrain from calling release_revisions() on it in the first place, no?
On Wed, Mar 23 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> Use release_revisions() to various users of "struct rev_list" which >> need to have their "struct rev_info" zero-initialized before we can >> start using it. In all of these cases we might "goto cleanup" (or equivalent), > > I didn't look at the bisect code, but the bundle one looks iffy from > the point of view of API cleanliness. If we have not yet called > repo_init_revisions() on a revs, we should refrain from calling > release_revisions() on it in the first place, no? It could be avoided, but I'd really prefer not to for this series. repo_init_revisions() is a non-trivial function, and changing the various bits in this series that can easily have a "goto" pattern because we assume that { 0 }-init'd is safe to pass to release_revisions() would be a larger change... We assume that in a lot of other destructors throughout the codebase, I figured we could leave this for later. Is that OK with you?
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > On Wed, Mar 23 2022, Junio C Hamano wrote: > >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> >>> Use release_revisions() to various users of "struct rev_list" which >>> need to have their "struct rev_info" zero-initialized before we can >>> start using it. In all of these cases we might "goto cleanup" (or equivalent), >> >> I didn't look at the bisect code, but the bundle one looks iffy from >> the point of view of API cleanliness. If we have not yet called >> repo_init_revisions() on a revs, we should refrain from calling >> release_revisions() on it in the first place, no? > > It could be avoided, but I'd really prefer not to for this series. > > repo_init_revisions() is a non-trivial function, and changing the > various bits in this series that can easily have a "goto" pattern > because we assume that { 0 }-init'd is safe to pass to > release_revisions() would be a larger change... > > We assume that in a lot of other destructors throughout the codebase, I > figured we could leave this for later. > > Is that OK with you? Not really. If you introduce "#define REV_INFO_INIT { 0 }", perhaps, though. Without such a mechanism to clearly say "here is what we initialize a rev_info", the first call to repo_init_revisions() looks like the place that initializes a rev_info, and call to release_revisions() on a rev_info that did not go through repo_init_revisions() looks like a call to free() of a pointer that hasn't been assigned the result from an allocation from the heap. That is where my "iffy from the API cleanliness POV" comes from.
Junio C Hamano <gitster@pobox.com> writes: > Without such a mechanism to clearly say "here is what we initialize "what" -> "where and how". > a rev_info", the first call to repo_init_revisions() looks like the > place that initializes a rev_info, and call to release_revisions() > on a rev_info that did not go through repo_init_revisions() looks > like a call to free() of a pointer that hasn't been assigned the > result from an allocation from the heap. That is where my "iffy > from the API cleanliness POV" comes from.
diff --git a/bisect.c b/bisect.c index cc6b8b6230d..159a4b644df 100644 --- a/bisect.c +++ b/bisect.c @@ -1010,7 +1010,7 @@ void read_bisect_terms(const char **read_bad, const char **read_good) */ enum bisect_error bisect_next_all(struct repository *r, const char *prefix) { - struct rev_info revs; + struct rev_info revs = { 0 }; struct commit_list *tried; int reaches = 0, all = 0, nr, steps; enum bisect_error res = BISECT_OK; @@ -1035,7 +1035,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix) res = check_good_are_ancestors_of_bad(r, prefix, no_checkout); if (res) - return res; + goto cleanup; bisect_rev_setup(r, &revs, prefix, "%s", "^%s", 1); @@ -1060,14 +1060,16 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix) term_good, term_bad); - return BISECT_FAILED; + res = BISECT_FAILED; + goto cleanup; } if (!all) { fprintf(stderr, _("No testable commit found.\n" "Maybe you started with bad path arguments?\n")); - return BISECT_NO_TESTABLE_COMMIT; + res = BISECT_NO_TESTABLE_COMMIT; + goto cleanup; } bisect_rev = &revs.commits->item->object.oid; @@ -1087,7 +1089,8 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix) * for negative return values for early returns up * until the cmd_bisect__helper() caller. */ - return BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND; + res = BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND; + goto cleanup; } nr = all - reaches - 1; @@ -1106,7 +1109,10 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix) /* Clean up objects used, as they will be reused. */ repo_clear_commit_marks(r, ALL_REV_FLAGS); - return bisect_checkout(bisect_rev, no_checkout); + res = bisect_checkout(bisect_rev, no_checkout); +cleanup: + release_revisions(&revs); + return res; } static inline int log2i(int n) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 0eeced8a10d..e8e20cdcc53 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -756,7 +756,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid, { char *displaypath; struct strvec diff_files_args = STRVEC_INIT; - struct rev_info rev; + struct rev_info rev = { 0 }; int diff_files_result; struct strbuf buf = STRBUF_INIT; const char *git_dir; @@ -843,6 +843,7 @@ static void status_submodule(const char *path, const struct object_id *ce_oid, cleanup: strvec_clear(&diff_files_args); free(displaypath); + release_revisions(&rev); } static void status_submodule_cb(const struct cache_entry *list_item, diff --git a/bundle.c b/bundle.c index e359370cfcd..a83d253a815 100644 --- a/bundle.c +++ b/bundle.c @@ -196,14 +196,16 @@ int verify_bundle(struct repository *r, * to be verbose about the errors */ struct string_list *p = &header->prerequisites; - struct rev_info revs; + struct rev_info revs = { 0 }; const char *argv[] = {NULL, "--all", NULL}; struct commit *commit; int i, ret = 0, req_nr; const char *message = _("Repository lacks these prerequisite commits:"); - if (!r || !r->objects || !r->objects->odb) - return error(_("need a repository to verify a bundle")); + if (!r || !r->objects || !r->objects->odb) { + ret = error(_("need a repository to verify a bundle")); + goto cleanup; + } repo_init_revisions(r, &revs, NULL); for (i = 0; i < p->nr; i++) { @@ -221,7 +223,7 @@ int verify_bundle(struct repository *r, error("%s %s", oid_to_hex(oid), name); } if (revs.pending.nr != p->nr) - return ret; + goto cleanup; req_nr = revs.pending.nr; setup_revisions(2, argv, &revs, NULL); @@ -283,6 +285,8 @@ int verify_bundle(struct repository *r, list_refs(r, 0, NULL); } } +cleanup: + release_revisions(&revs); return ret; }
Use release_revisions() to various users of "struct rev_list" which need to have their "struct rev_info" zero-initialized before we can start using it. In all of these cases we might "goto cleanup" (or equivalent), For the bundle.c code see the early exit case added in 3bbbe467f29 (bundle verify: error out if called without an object database, 2019-05-27). For the relevant bisect.c code see 45b6370812c (bisect: libify `check_good_are_ancestors_of_bad` and its dependents, 2020-02-17). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- bisect.c | 18 ++++++++++++------ builtin/submodule--helper.c | 3 ++- bundle.c | 12 ++++++++---- 3 files changed, 22 insertions(+), 11 deletions(-)