Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
