| Message ID | 20231003185047.2697995-1-heftig@archlinux.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] submodule--helper: use submodule_from_path in set-{url,branch} | expand |
"Jan Alexander Steffens (heftig)" <heftig@archlinux.org> writes: > The commands need a path to a submodule but treated it as the name when > modifying the .gitmodules file, leading to confusion when a submodule's > name does not match its path. Thanks for noticing and fixing this common mix-up. > Because calling submodule_from_path initializes the submodule cache, we > need to manually trigger a reread before syncing, as the cache is > missing the config change we just made. > Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org> > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index f6871efd95..f376466a5e 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2902,19 +2902,26 @@ static int module_set_url(int argc, const char **argv, const char *prefix) > N_("git submodule set-url [--quiet] <path> <newurl>"), > NULL > }; > + const struct submodule *sub; > > argc = parse_options(argc, argv, prefix, options, usage, 0); > > if (argc != 2 || !(path = argv[0]) || !(newurl = argv[1])) > usage_with_options(usage, options); > > - config_name = xstrfmt("submodule.%s.url", path); > + sub = submodule_from_path(the_repository, null_oid(), path); > > + if (!sub) > + die(_("no submodule mapping found in .gitmodules for path '%s'"), > + path); > + > + config_name = xstrfmt("submodule.%s.url", sub->name); Looks correct. > config_set_in_gitmodules_file_gently(config_name, newurl); > - sync_submodule(path, prefix, NULL, quiet ? OPT_QUIET : 0); > + > + repo_read_gitmodules (the_repository, 0); Style. No extra space between function name and "(". But more importantly, is this sufficient? repo_read_gitmodules() does not seem to clear the cache and build its contents from scratch (as submodule_cache_check_init() bypasses itself upon second call). The code is correct because submodule-config.c::config_from() does set .overwrite to true, so submodule.name.url would be overwritten to the new value, I think, but somebody else who is more familiar with the recent submodule code may want to sanity check my analysis. > + sync_submodule(sub->path, prefix, NULL, quiet ? OPT_QUIET : 0); Is the use of "sub" still safe here? I think it is safe as repo_read_gitmodules() did not rebuild the in-core cache from scratch but did selective overwrite, so the in-core instance "sub" is still valid, but again somebody else who is more familiar with the recent submodule code may want to sanity check. > @@ -2942,19 +2949,26 @@ static int module_set_branch(int argc, const char **argv, const char *prefix) > N_("git submodule set-branch [-q|--quiet] (-b|--branch) <branch> <path>"), > NULL > }; > + const struct submodule *sub; > > argc = parse_options(argc, argv, prefix, options, usage, 0); > > if (!opt_branch && !opt_default) > die(_("--branch or --default required")); > > if (opt_branch && opt_default) > die(_("options '%s' and '%s' cannot be used together"), "--branch", "--default"); > > if (argc != 1 || !(path = argv[0])) > usage_with_options(usage, options); > > - config_name = xstrfmt("submodule.%s.branch", path); > + sub = submodule_from_path(the_repository, null_oid(), path); > + > + if (!sub) > + die(_("no submodule mapping found in .gitmodules for path '%s'"), > + path); > + > + config_name = xstrfmt("submodule.%s.branch", sub->name); > ret = config_set_in_gitmodules_file_gently(config_name, opt_branch); This side happens not to require re-reading of gitmodules file, because, unlike the URL helper, we do not care what we have in the in-core cache is stale. It is correct but feels a bit brittle.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index f6871efd95..f376466a5e 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2902,19 +2902,26 @@ static int module_set_url(int argc, const char **argv, const char *prefix) N_("git submodule set-url [--quiet] <path> <newurl>"), NULL }; + const struct submodule *sub; argc = parse_options(argc, argv, prefix, options, usage, 0); if (argc != 2 || !(path = argv[0]) || !(newurl = argv[1])) usage_with_options(usage, options); - config_name = xstrfmt("submodule.%s.url", path); + sub = submodule_from_path(the_repository, null_oid(), path); + if (!sub) + die(_("no submodule mapping found in .gitmodules for path '%s'"), + path); + + config_name = xstrfmt("submodule.%s.url", sub->name); config_set_in_gitmodules_file_gently(config_name, newurl); - sync_submodule(path, prefix, NULL, quiet ? OPT_QUIET : 0); + + repo_read_gitmodules (the_repository, 0); + sync_submodule(sub->path, prefix, NULL, quiet ? OPT_QUIET : 0); free(config_name); - return 0; } @@ -2942,19 +2949,26 @@ static int module_set_branch(int argc, const char **argv, const char *prefix) N_("git submodule set-branch [-q|--quiet] (-b|--branch) <branch> <path>"), NULL }; + const struct submodule *sub; argc = parse_options(argc, argv, prefix, options, usage, 0); if (!opt_branch && !opt_default) die(_("--branch or --default required")); if (opt_branch && opt_default) die(_("options '%s' and '%s' cannot be used together"), "--branch", "--default"); if (argc != 1 || !(path = argv[0])) usage_with_options(usage, options); - config_name = xstrfmt("submodule.%s.branch", path); + sub = submodule_from_path(the_repository, null_oid(), path); + + if (!sub) + die(_("no submodule mapping found in .gitmodules for path '%s'"), + path); + + config_name = xstrfmt("submodule.%s.branch", sub->name); ret = config_set_in_gitmodules_file_gently(config_name, opt_branch); free(config_name);
The commands need a path to a submodule but treated it as the name when modifying the .gitmodules file, leading to confusion when a submodule's name does not match its path. Because calling submodule_from_path initializes the submodule cache, we need to manually trigger a reread before syncing, as the cache is missing the config change we just made. Signed-off-by: Jan Alexander Steffens (heftig) <heftig@archlinux.org> --- builtin/submodule--helper.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)