Re: [PATCH 1/6] submodule--helper: use submodule_from_path in set-{url,branch}

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 4, 2023, 01:10 UTC
Message-ID: <xmqqmswztcoc.fsf@gitster.g>
In-Reply-To: <20231003185047.2697995-1-heftig@archlinux.org>

"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>

Show 23 quoted lines

> 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.

Show 26 quoted lines

> @@ -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.