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

[Junio C Hamano <gitster@pobox.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.

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

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

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

> +		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 &&