Re: [PATCH v8 2/3] introduce submodule.hasSuperproject record

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

Emily Shaffer <emilyshaffer@google.com> writes:

Show 6 quoted lines

> +	/*
> +	 * Note location of superproject's gitdir. Because the submodule already
> +	 * has a gitdir and local config, we can store this pointer from
> +	 * worktree config to worktree config, if the submodule has
> +	 * extensions.worktreeConfig set.
> +	 */

Probably the comment is a bit stale. There is no longer a pointer or location of superproject's gitdir recorded anywhere.

Show 24 quoted lines

> +	strbuf_addf(&config_path, "%s/config", real_new_git_dir);
> +	git_configset_init(&sub_cs);
> +	git_configset_add_file(&sub_cs, config_path.buf);
> +
> +	git_config_set_in_file(config_path.buf, "submodule.hasSuperproject",
> +			       "true");
> +
> +	git_configset_clear(&sub_cs);
> +	strbuf_release(&config_path);
> +	strbuf_release(&sb);
>  	free(old_git_dir);
>  	free(real_old_git_dir);
>  	free(real_new_git_dir);
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 40cf8d89aa..833fa01961 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -115,6 +115,10 @@ inspect() {
>  	git -C "$sub_dir" rev-parse HEAD >head-sha1 &&
>  	git -C "$sub_dir" update-index --refresh &&
>  	git -C "$sub_dir" diff-files --exit-code &&
> +
> +	# Ensure that submodule.hasSuperproject is set.
> +	git -C "$sub_dir" config "submodule.hasSuperproject"

Are we sufficiently happy to see the variable is set to anything, or do we require it to be set to boolean true?

If the former, the above is fine, with trailing && added.

If the latter, then something like

	val=$(git config --type=bool "submodule.hasSuperproject") &&
	test "$val" = true &&

would be more appropriate, but I wonder something like

test_config_is () {
	local var expect val
	var="$1" expect="$2"
	shift 2
        val=$(git "$@" config --type=bool "$var") &&
	test "$val" = "$expect"
}

would be in order.  That would allow us to write

	test_config_is submodule.hasSuperproject true -C "$sub_dir" &&

Show 14 quoted lines

>  	git -C "$sub_dir" clean -n -d -x >untracked
>  }
>  
> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
> index 11cccbb333..422c3cc343 100755
> --- a/t/t7406-submodule-update.sh
> +++ b/t/t7406-submodule-update.sh
> @@ -1061,4 +1061,12 @@ test_expect_success 'submodule update --quiet passes quietness to fetch with a s
>  	)
>  '
>  
> +test_expect_success 'submodule update adds submodule.hasSuperproject to older repos' '
> +	(cd super &&
> +	 git -C submodule config --unset submodule.hasSuperproject &&

Are we testing that submodule.hasSuperproject is set, and that it can successfully be unset? "config --unset no.such.var" will exit with non-zero status.

> +	 git submodule update &&
> +	 git -C submodule config submodule.hasSuperproject

Ditto.