Re: [PATCH v8 3/3] rev-parse: short-circuit superproject worktree when config unset

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 1, 2022, 07:06 UTC
Message-ID: <xmqq7d9eup7k.fsf@gitster.g>
In-Reply-To: <20220301002613.1459916-4-emilyshaffer@google.com>

Emily Shaffer <emilyshaffer@google.com> writes:

Show 18 quoted lines

> diff --git a/submodule.c b/submodule.c
> index 741104af8a..463e7f0c48 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -2237,6 +2237,7 @@ int get_superproject_working_tree(struct strbuf *buf)
>  	struct strbuf sb = STRBUF_INIT;
>  	struct strbuf one_up = STRBUF_INIT;
>  	const char *cwd = xgetcwd();
> +	int has_superproject_cfg = 0;
>  	int ret = 0;
>  	const char *subpath;
>  	int code;
> @@ -2250,6 +2251,17 @@ int get_superproject_working_tree(struct strbuf *buf)
>  		 */
>  		return 0;
>  
> +	if (git_config_get_bool("submodule.hassuperproject", &has_superproject_cfg)
> +	    || !has_superproject_cfg) {

git_config_get_bool() returns 0 when it successfully finds the variable, so the above says "If submodule.hasSuperproject is not set at all, or if it is set to false, then..."

Show 7 quoted lines

> +		/*
> +		 * If we don't have a superproject, then we're probably not a
> +		 * submodule. If this is failing and shouldn't be, investigate
> +		 * why the config was never set.
> +		 */
> +		error(_("Asked to find a superproject, but submodule.hasSuperproject != true"));
> +		return 0;

But given that this thing is new, I am not sure if that is a sensible guard to use here. Shouldn't we say

 - If submodule.hasSuperproject is EXPLICITLY set to false then ...

instead?  I.e.

	if (!git_config_get_bool("submodule.hassuperproject", &value) &&
	    !value) {
		error(_("asked to ..."));
		return 0;
	}