| Message ID | patch-v4-09.27-2f4e65fb534-20220331T005325Z-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. > > To do this add a stub "REV_INFO_INIT" macro, ideally macro would be > able to fully initialize a "struct rev_info", but all it does is the > equivalent of assigning "{ 0 }" to the struct, the API user will still > need to use repo_init_revisions(). In some future follow-up work we'll > hopefully make REV_INFO_INIT be a "stand-alone" init likke STRBUF_INIT > and other similar macros. I do not think we want to leave such a misleading paragraph to future developers. Yes, We may want to move some of what init_revisions() does to REV_INFO_INIT(), and for that, it helps to start using greppable string REV_INFO_INT early rather than { 0 } to ease such transition, and that is what we should be stressing, instead of ranting "it does not do anything, so why are we stupidly introducing a name, instead of writing { 0 }, which is what amounts to it anyway?" without explicitly saying so but hinting with words like "stub", "all it does is", and "will still need to". Is that some kind of passive-aggressive thing? You cannot use get_revision() even after calling init_revisions(), and still need to use setup_revisions() before hand, but that does not mean init_revisions() is not doing its job. It may be implemented as the zero-initialization right now, but it misses the point to put a stress on the fact that it doesn't do much now.
On Thu, Mar 31 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. >> >> To do this add a stub "REV_INFO_INIT" macro, ideally macro would be >> able to fully initialize a "struct rev_info", but all it does is the >> equivalent of assigning "{ 0 }" to the struct, the API user will still >> need to use repo_init_revisions(). In some future follow-up work we'll >> hopefully make REV_INFO_INIT be a "stand-alone" init likke STRBUF_INIT >> and other similar macros. > > I do not think we want to leave such a misleading paragraph to > future developers. > > Yes, We may want to move some of what init_revisions() does to > REV_INFO_INIT(), and for that, it helps to start using greppable > string REV_INFO_INT early rather than { 0 } to ease such transition, > and that is what we should be stressing, instead of ranting "it does > not do anything, so why are we stupidly introducing a name, instead > of writing { 0 }, which is what amounts to it anyway?" without > explicitly saying so but hinting with words like "stub", "all it > does is", and "will still need to". > > Is that some kind of passive-aggressive thing? No, I'm not trying to be a dick. I'm just confused, sorry :) I.e. I didn't grok what you really wanted from that REV_INFO_INIT pattern in the beginning, and I largely draft my commit messages as a way to adequately explain things to myself. The "stub" and "not sufficient (yet!)" part of the docs is a (probably too leaky) artifact of having (after the whole thread about REV_INFO_INIT started) gotten that mostly working as a "real" initializer. Which I figured I'd submit at some point after this lands. The grep.c case was quite tricky, but the rest look pretty easy, I just didn't finish them. I can re-roll & omit this entirely from the commit message, and the API docs. I just thought, and still think, that it's worth drawing the API user's attention to the fact that unlike most stand-alone *_INIT macros this one isn't sufficient to be up & running with a "struct rev_info". Which isn't the case with {STRBUF,STRVEC}_INIT, CHILD_PROCESS_INIT, STRING_LIST_INIT_* etc. etc., i.e. if you've used any of these: git grep -h -o '\S+_INIT;' | sort | uniq -c | sort -nr You're likely to expect them to "flow" a certain way when it comes to initializing the struct on the stack, but this one will be a bit of an odd case while repo_init_revisions() is still required, which seems to me to be worth explaining. But not enough to argue about it, so whatever you'd prefer... > You cannot use get_revision() even after calling init_revisions(), > and still need to use setup_revisions() before hand, but that does > not mean init_revisions() is not doing its job. It may be > implemented as the zero-initialization right now, but it misses the > point to put a stress on the fact that it doesn't do much now. I don't think it's broken, having gotten most of the way towards rewriting it to be macro-init'd there are some tricky edge cases where we might always need to call a function to fully initialize it. But per the above I thought it made sense to explain that this particular *_INIT is a bit of an odd case if you've gotten used to other such macros.
diff --git a/bisect.c b/bisect.c index cc6b8b6230d..b63669cc9d7 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 = REV_INFO_INIT; 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 24980863f68..cda33ee4d2b 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -766,7 +766,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 = REV_INFO_INIT; int diff_files_result; struct strbuf buf = STRBUF_INIT; const char *git_dir; @@ -853,6 +853,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 d50cfb5aa7e..6a870a6edb7 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 = REV_INFO_INIT; 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); @@ -284,6 +286,8 @@ int verify_bundle(struct repository *r, printf_ln("The bundle uses this filter: %s", list_objects_filter_spec(&header->filter)); } +cleanup: + release_revisions(&revs); return ret; } diff --git a/revision.h b/revision.h index 19f886472aa..c920753e8b9 100644 --- a/revision.h +++ b/revision.h @@ -329,6 +329,17 @@ struct rev_info { struct tmp_objdir *remerge_objdir; }; +/** + * Initialize the "struct rev_info" structure with a macro. + * + * This is not sufficient (yet!) to initialize a "struct rev_info", + * but it's OK (but redundant) to use it before a call to + * repo_init_revisions(), which does the real initialization. By using + * this it's safe to call release_revisions() on the "struct rev_info" + * without having called repo_init_revisions(). + */ +#define REV_INFO_INIT { 0 } + /** * Initialize a rev_info structure with default values. The third parameter may * be NULL or can be prefix path, and then the `.prefix` variable will be set @@ -363,7 +374,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, /** * Free data allocated in a "struct rev_info" after it's been - * initialized with repo_init_revisions(). + * initialized with repo_init_revisions() or REV_INFO_INIT. */ void release_revisions(struct rev_info *revs); diff --git a/submodule.c b/submodule.c index 4df04ae14e4..e796151ac6f 100644 --- a/submodule.c +++ b/submodule.c @@ -619,7 +619,7 @@ void show_submodule_diff_summary(struct diff_options *o, const char *path, struct object_id *one, struct object_id *two, unsigned dirty_submodule) { - struct rev_info rev; + struct rev_info rev = REV_INFO_INIT; struct commit *left = NULL, *right = NULL; struct commit_list *merge_bases = NULL; struct repository *sub; @@ -645,6 +645,7 @@ void show_submodule_diff_summary(struct diff_options *o, const char *path, print_submodule_diff_summary(sub, &rev, o); out: + release_revisions(&rev); if (merge_bases) free_commit_list(merge_bases); clear_commit_marks(left, ~0);
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. To do this add a stub "REV_INFO_INIT" macro, ideally macro would be able to fully initialize a "struct rev_info", but all it does is the equivalent of assigning "{ 0 }" to the struct, the API user will still need to use repo_init_revisions(). In some future follow-up work we'll hopefully make REV_INFO_INIT be a "stand-alone" init likke STRBUF_INIT and other similar macros. 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). For the submodule.c code see the "goto" on "(!left || !right || !sub)" added in 8e6df65015f (submodule: refactor show_submodule_summary with helper function, 2016-08-31). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- bisect.c | 18 ++++++++++++------ builtin/submodule--helper.c | 3 ++- bundle.c | 12 ++++++++---- revision.h | 13 ++++++++++++- submodule.c | 3 ++- 5 files changed, 36 insertions(+), 13 deletions(-)