| Message ID | patch-v2-05.24-a4672aa9c94-20220719T204458Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | submodule--helper: fix memory leaks | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > The module_update() function calls module_list_compute() twice, which > in turn will reset the "struct pathspec" passed to it. Let's instead > track two of them, and clear them both. Looks correct. I wish there were an easy way to limit the scope of this new pathspec2 to the block guarded by "if (opt.init)", but with the "jump to error-exit" idiom, there unfortunately isn't. Thanks. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/submodule--helper.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 28c5fdb8954..7466e781e97 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2602,6 +2602,7 @@ static int update_submodules(struct update_data *update_data) > static int module_update(int argc, const char **argv, const char *prefix) > { > struct pathspec pathspec = { 0 }; > + struct pathspec pathspec2 = { 0 }; > struct update_data opt = UPDATE_DATA_INIT; > struct list_objects_filter_options filter_options = { 0 }; > int ret; > @@ -2692,7 +2693,7 @@ static int module_update(int argc, const char **argv, const char *prefix) > struct init_cb info = INIT_CB_INIT; > > if (module_list_compute(argc, argv, opt.prefix, > - &pathspec, &list) < 0) { > + &pathspec2, &list) < 0) { > ret = 1; > goto cleanup; > } > @@ -2715,6 +2716,7 @@ static int module_update(int argc, const char **argv, const char *prefix) > cleanup: > list_objects_filter_release(&filter_options); > clear_pathspec(&pathspec); > + clear_pathspec(&pathspec2); > return ret; > }
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > The module_update() function calls module_list_compute() twice, which > in turn will reset the "struct pathspec" passed to it. Let's instead > track two of them, and clear them both. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/submodule--helper.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 28c5fdb8954..7466e781e97 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2602,6 +2602,7 @@ static int update_submodules(struct update_data *update_data) > static int module_update(int argc, const char **argv, const char *prefix) > { > struct pathspec pathspec = { 0 }; > + struct pathspec pathspec2 = { 0 }; > struct update_data opt = UPDATE_DATA_INIT; > struct list_objects_filter_options filter_options = { 0 }; > int ret; > @@ -2692,7 +2693,7 @@ static int module_update(int argc, const char **argv, const char *prefix) > struct init_cb info = INIT_CB_INIT; > > if (module_list_compute(argc, argv, opt.prefix, > - &pathspec, &list) < 0) { > + &pathspec2, &list) < 0) { > ret = 1; > goto cleanup; > } This change looks good, but we should do more refactoring in the future. This bit of code inside "if (opt.init)" was copied over from module_init() in 29a5e9e1ff (submodule--helper update-clone: learn --init, 2022-03-04). Prior to that commit, we used to just invoke "git submodule init" in git-submodule.sh. What I wished I had done instead is to create a helper function that can be used by both module_init() and module_update(). Something like: ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ---- diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 28c5fdb895..2040cde4ba 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -481,14 +481,14 @@ static int starts_with_dot_dot_slash(const char *const path) PATH_MATCH_XPLATFORM); } -struct init_cb { +struct init_opts { const char *prefix; - unsigned int flags; + int quiet; }; #define INIT_CB_INIT { 0 } static void init_submodule(const char *path, const char *prefix, - unsigned int flags) + int quiet) { const struct submodule *sub; struct strbuf sb = STRBUF_INIT; @@ -538,7 +538,7 @@ static void init_submodule(const char *path, const char *prefix, if (git_config_set_gently(sb.buf, url)) die(_("Failed to register url for submodule path '%s'"), displaypath); - if (!(flags & OPT_QUIET)) + if (!quiet) fprintf(stderr, _("Submodule '%s' (%s) registered for path '%s'\n"), sub->name, url, displaypath); @@ -567,15 +567,39 @@ static void init_submodule(const char *path, const char *prefix, static void init_submodule_cb(const struct cache_entry *list_item, void *cb_data) { - struct init_cb *info = cb_data; - init_submodule(list_item->name, info->prefix, info->flags); + struct init_opts *info = cb_data; + init_submodule(list_item->name, info->prefix, info->quiet); } -static int module_init(int argc, const char **argv, const char *prefix) +static int init_submodules(int argc, const char **argv, struct init_opts *opts) { - struct init_cb info = INIT_CB_INIT; - struct pathspec pathspec = { 0 }; struct module_list list = MODULE_LIST_INIT; + struct pathspec pathspec = { 0 }; + int ret = 0; + + if (module_list_compute(argc, argv, opts->prefix, + &pathspec, &list) < 0) { + ret = 1; + goto cleanup; + } + + /* + * If there are no path args and submodule.active is set then, + * by default, only initialize 'active' modules. + */ + if (!argc && git_config_get_value_multi("submodule.active")) + module_list_active(&list); + + for_each_listed_submodule(&list, init_submodule_cb, opts); + + cleanup: + clear_pathspec(&pathspec); + return ret; +} + +static int module_init(int argc, const char **argv, const char *prefix) +{ + struct init_opts info = INIT_CB_INIT; int quiet = 0; struct option module_init_options[] = { @@ -587,33 +611,14 @@ static int module_init(int argc, const char **argv, const char *prefix) N_("git submodule init [<options>] [<path>]"), NULL }; - int ret; argc = parse_options(argc, argv, prefix, module_init_options, git_submodule_helper_usage, 0); - if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0) { - ret = 1; - goto cleanup; - } - - /* - * If there are no path args and submodule.active is set then, - * by default, only initialize 'active' modules. - */ - if (!argc && git_config_get_value_multi("submodule.active")) - module_list_active(&list); - info.prefix = prefix; - if (quiet) - info.flags |= OPT_QUIET; - - for_each_listed_submodule(&list, init_submodule_cb, &info); + info.quiet = quiet; - ret = 0; -cleanup: - clear_pathspec(&pathspec); - return ret; + return init_submodules(argc, argv, &info); } struct status_cb { @@ -2688,27 +2693,13 @@ static int module_update(int argc, const char **argv, const char *prefix) opt.warn_if_uninitialized = 1; if (opt.init) { - struct module_list list = MODULE_LIST_INIT; - struct init_cb info = INIT_CB_INIT; - - if (module_list_compute(argc, argv, opt.prefix, - &pathspec, &list) < 0) { - ret = 1; + struct init_opts init_opts = { + .quiet = opt.quiet, + .prefix = opt.prefix, + }; + ret = init_submodules(argc, argv, &init_opts); + if (ret) goto cleanup; - } - - /* - * If there are no path args and submodule.active is set then, - * by default, only initialize 'active' modules. - */ - if (!argc && git_config_get_value_multi("submodule.active")) - module_list_active(&list); - - info.prefix = opt.prefix; - if (opt.quiet) - info.flags |= OPT_QUIET; - - for_each_listed_submodule(&list, init_submodule_cb, &info); } ret = update_submodules(&opt); ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ---- I don't think we need to do this now; it's already pretty noisy for just a "leak cleanup" series, and it isn't even complete (IIRC you mentioned that we could probably drop init_submodule_cb(), but I can't find the message any more). Hopefully someone will pick this up. I doubt I'll have the time to do it, but maybe. > @@ -2715,6 +2716,7 @@ static int module_update(int argc, const char **argv, const char *prefix) > cleanup: > list_objects_filter_release(&filter_options); > clear_pathspec(&pathspec); > + clear_pathspec(&pathspec2); > return ret; > } > > -- > 2.37.1.1062.g385eac7fccf
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 28c5fdb8954..7466e781e97 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2602,6 +2602,7 @@ static int update_submodules(struct update_data *update_data) static int module_update(int argc, const char **argv, const char *prefix) { struct pathspec pathspec = { 0 }; + struct pathspec pathspec2 = { 0 }; struct update_data opt = UPDATE_DATA_INIT; struct list_objects_filter_options filter_options = { 0 }; int ret; @@ -2692,7 +2693,7 @@ static int module_update(int argc, const char **argv, const char *prefix) struct init_cb info = INIT_CB_INIT; if (module_list_compute(argc, argv, opt.prefix, - &pathspec, &list) < 0) { + &pathspec2, &list) < 0) { ret = 1; goto cleanup; } @@ -2715,6 +2716,7 @@ static int module_update(int argc, const char **argv, const char *prefix) cleanup: list_objects_filter_release(&filter_options); clear_pathspec(&pathspec); + clear_pathspec(&pathspec2); return ret; }
The module_update() function calls module_list_compute() twice, which in turn will reset the "struct pathspec" passed to it. Let's instead track two of them, and clear them both. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/submodule--helper.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)