Re: [PATCH 3/3] add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 29, 2024, 17:55 UTC
Message-ID: <xmqqcyrczzv7.fsf@gitster.g>
In-Reply-To: <0e38da05-efd6-451e-bd8a-b2b3457c0c75@gmail.com>

Rubén Justo <rjusto@gmail.com> writes:

> Use the newer advise_if_enabled() machinery to show the advice.

Common to the other two patches, but "Newer" is not a good enough excuse if the existing code is working well for us and not being maintenance burden. The previous two patches were helped by use of advise_if_enabled() in a concrete way (or perhaps two ways), and that should be explained when selling them.

This one also needs a similar justification, but with a twist.

Show 24 quoted lines

> We don't have a test for this.  Add one.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
>  builtin/add.c  |  6 +++---
>  t/t3700-add.sh | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 289adaaecf..e97699d6b9 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -310,9 +310,9 @@ static void check_embedded_repo(const char *path)
>  	strbuf_strip_suffix(&name, "/");
>  
>  	warning(_("adding embedded git repository: %s"), name.buf);
> -	if (!adviced_on_embedded_repo &&
> -	    advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) {
> -		advise(embedded_advice, name.buf, name.buf);
> +	if (!adviced_on_embedded_repo) {
> +		advise_if_enabled(ADVICE_ADD_EMBEDDED_REPO,
> +				  embedded_advice, name.buf, name.buf);
>  		adviced_on_embedded_repo = 1;
>  	}

This uses a static variable "adviced_on_embedded_repo" to skip giving the advice messages over and over. The patch preserves that feature of this code while updating it to use the "if_enabled" variant.

Show 9 quoted lines

> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index 681081e0d5..2b92f3eb5b 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -349,6 +349,38 @@ test_expect_success '"git add ." in empty repo' '
>  	)
>  '
>  
> +test_expect_success '"git add" a nested repository' '

"nested" -> "embedded", as the warning, advice_type and the message contents all use "embedded" consistently.

Show 10 quoted lines

> +	rm -fr empty &&
> +	git init empty &&
> +	(
> +		cd empty &&
> +		git init empty &&
> +		(
> +			cd empty &&
> +			git commit --allow-empty -m "foo"
> +		) &&
> +		git add empty 2>actual &&

It is very good to add a test for a feature that we failed to cover so far. But the feature, as we seen above, is twofold. We see an advice, and we it see only once even when we have multiple.

So we should add two such embedded repositories for the test, no? Also, the shell repository is not meant to stay empty as the user will make a mistaken attempt to "add" something to it.

Perhaps the above part would become more like:

	rm -rf outer && git init outer &&
	(
		cd outer &&
		for i in 1 2
		do
			name=inner$i &&
			git init $name &&
                        git -C $name --allow-empty -m $name ||
				return 1
		done &&
                git add . 2>actual &&

to use a more descriptive name that shows the point of the test (it is not interesting that they are empty---they are in "outer contains innner repositories" relationship and that is what the test wants to make), and ensure "only once" part of the feature we are testing.

Show 24 quoted lines

> +		cat >expect <<-EOF &&
> +		warning: adding embedded git repository: empty
> +		hint: You${SQ}ve added another git repository inside your current repository.
> +		hint: Clones of the outer repository will not contain the contents of
> +		hint: the embedded repository and will not know how to obtain it.
> +		hint: If you meant to add a submodule, use:
> +		hint: 
> +		hint: 	git submodule add <url> empty
> +		hint: 
> +		hint: If you added this path by mistake, you can remove it from the
> +		hint: index with:
> +		hint: 
> +		hint: 	git rm --cached empty
> +		hint: 
> +		hint: See "git help submodule" for more information.
> +		hint: Disable this message with "git config advice.addEmbeddedRepo false"
> +		EOF
> +		test_cmp expect actual
> +	)
> +'
> +
>  test_expect_success 'error on a repository with no commits' '
>  	rm -fr empty &&
>  	git init empty &&