"Jan Alexander Steffens (heftig)" <heftig@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> 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.
