[PATCH 0/3] add: use advise_if_enabled

[PATCH 0/3] add: use advise_if_enabled

[Rubén Justo]

This series is a simple change, in builtin/add.c, from:

	if (advice_enabled(XXX))
		advise(MMM)

to the newer:

	advise_if_enabled(XXX, MMM)


Rubén Justo (3):
  add: use advise_if_enabled for ADVICE_ADD_IGNORED_FILE
  add: use advise_if_enabled for ADVICE_ADD_EMPTY_PATHSPEC
  add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO

 builtin/add.c              | 18 ++++++---------
 t/t3700-add.sh             | 45 ++++++++++++++++++++++++++++++++++++--
 t/t7400-submodule-basic.sh |  3 +--
 3 files changed, 51 insertions(+), 15 deletions(-)

-- 
2.44.0.371.gf9813d4ed5

[PATCH 1/3] add: use advise_if_enabled for ADVICE_ADD_IGNORED_FILE

[Rubén Justo]

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

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/add.c              | 6 ++----
 t/t3700-add.sh             | 3 +--
 t/t7400-submodule-basic.sh | 3 +--
 3 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 393c10cbcf..8f148987f7 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -328,10 +328,8 @@ static int add_files(struct dir_struct *dir, int flags)
 		fprintf(stderr, _(ignore_error));
 		for (i = 0; i < dir->ignored_nr; i++)
 			fprintf(stderr, "%s\n", dir->ignored[i]->name);
-		if (advice_enabled(ADVICE_ADD_IGNORED_FILE))
-			advise(_("Use -f if you really want to add them.\n"
-				"Turn this message off by running\n"
-				"\"git config advice.addIgnoredFile false\""));
+		advise_if_enabled(ADVICE_ADD_IGNORED_FILE,
+				  _("Use -f if you really want to add them."));
 		exit_status = 1;
 	}
 
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index f23d39f0d5..76c2c9e7b0 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -370,8 +370,7 @@ cat >expect.err <<\EOF
 The following paths are ignored by one of your .gitignore files:
 ignored-file
 hint: Use -f if you really want to add them.
-hint: Turn this message off by running
-hint: "git config advice.addIgnoredFile false"
+hint: Disable this message with "git config advice.addIgnoredFile false"
 EOF
 cat >expect.out <<\EOF
 add 'track-this'
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 00c1f1aab1..5c4a89df5c 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -212,8 +212,7 @@ test_expect_success 'submodule add to .gitignored path fails' '
 		The following paths are ignored by one of your .gitignore files:
 		submod
 		hint: Use -f if you really want to add them.
-		hint: Turn this message off by running
-		hint: "git config advice.addIgnoredFile false"
+		hint: Disable this message with "git config advice.addIgnoredFile false"
 		EOF
 		# Does not use test_commit due to the ignore
 		echo "*" > .gitignore &&
-- 
2.44.0.371.gf9813d4ed5

Re: [PATCH 1/3] add: use advise_if_enabled for ADVICE_ADD_IGNORED_FILE

[Junio C Hamano]

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

> -		if (advice_enabled(ADVICE_ADD_IGNORED_FILE))
> -			advise(_("Use -f if you really want to add them.\n"
> -				"Turn this message off by running\n"
> -				"\"git config advice.addIgnoredFile false\""));
> +		advise_if_enabled(ADVICE_ADD_IGNORED_FILE,
> +				  _("Use -f if you really want to add them."));

Good.

>  		exit_status = 1;
>  	}
>  
> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index f23d39f0d5..76c2c9e7b0 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -370,8 +370,7 @@ cat >expect.err <<\EOF
>  The following paths are ignored by one of your .gitignore files:
>  ignored-file
>  hint: Use -f if you really want to add them.
> -hint: Turn this message off by running
> -hint: "git config advice.addIgnoredFile false"
> +hint: Disable this message with "git config advice.addIgnoredFile false"

Funny that we weremanually crafting the hint to turn it off.  Nice
to see that code go.

>  EOF
>  cat >expect.out <<\EOF
>  add 'track-this'
> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
> index 00c1f1aab1..5c4a89df5c 100755
> --- a/t/t7400-submodule-basic.sh
> +++ b/t/t7400-submodule-basic.sh
> @@ -212,8 +212,7 @@ test_expect_success 'submodule add to .gitignored path fails' '
>  		The following paths are ignored by one of your .gitignore files:
>  		submod
>  		hint: Use -f if you really want to add them.
> -		hint: Turn this message off by running
> -		hint: "git config advice.addIgnoredFile false"
> +		hint: Disable this message with "git config advice.addIgnoredFile false"
>  		EOF
>  		# Does not use test_commit due to the ignore
>  		echo "*" > .gitignore &&

[PATCH 2/3] add: use advise_if_enabled for ADVICE_ADD_EMPTY_PATHSPEC

[Rubén Justo]

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

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 | 10 ++++++++++
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 8f148987f7..289adaaecf 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -438,10 +438,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (require_pathspec && pathspec.nr == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
-		if (advice_enabled(ADVICE_ADD_EMPTY_PATHSPEC))
-			advise( _("Maybe you wanted to say 'git add .'?\n"
-				"Turn this message off by running\n"
-				"\"git config advice.addEmptyPathspec false\""));
+		advise_if_enabled(ADVICE_ADD_EMPTY_PATHSPEC,
+				  _("Maybe you wanted to say 'git add .'?"));
 		return 0;
 	}
 
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 76c2c9e7b0..681081e0d5 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -28,6 +28,16 @@ test_expect_success 'Test of git add' '
 	touch foo && git add foo
 '
 
+test_expect_success 'Test with no pathspecs' '
+	cat >expect <<-EOF &&
+	Nothing specified, nothing added.
+	hint: Maybe you wanted to say ${SQ}git add .${SQ}?
+	hint: Disable this message with "git config advice.addEmptyPathspec false"
+	EOF
+	git add 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'Post-check that foo is in the index' '
 	git ls-files foo | grep foo
 '
-- 
2.44.0.371.gf9813d4ed5

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

[Rubén Justo]

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

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;
 	}
 
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' '
+	rm -fr empty &&
+	git init empty &&
+	(
+		cd empty &&
+		git init empty &&
+		(
+			cd empty &&
+			git commit --allow-empty -m "foo"
+		) &&
+		git add empty 2>actual &&
+		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 &&
-- 
2.44.0.371.gf9813d4ed5

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

[Junio C Hamano]

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

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

[Rubén Justo]

On Fri, Mar 29, 2024 at 10:55:56AM -0700, Junio C Hamano wrote:
> 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.

May I ask what you would find a good justification?

Perhaps "newer" -> "now preferred"?

> > +test_expect_success '"git add" a nested repository' '
> 
> "nested" -> "embedded", as the warning, advice_type and the message
> contents all use "embedded" consistently.

Makes sense.

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

Good point about the naming.  I'm not so sure about the "only once"
part, but I do not have any strong objection.

Thanks.

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

[Junio C Hamano]

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

>> This one also needs a similar justification, but with a twist.
>
> May I ask what you would find a good justification?
>
> Perhaps "newer" -> "now preferred"?

That is merely shifting justification around.  You'd now need to
answer: Why do you consider it preferred?

A few obvious advantages of using if_enabled() form are:

 - you do not have to give "here is how to disable this piece of advice"
   yourself.

 - the code can become shorter.

and they may make it preferred to use it, when appropriate.

> Good point about the naming.  I'm not so sure about the "only once"
> part, but I do not have any strong objection.

I am not sure what you are not sure about ;-).

If you are adding a test for a feature because it is not covered by
existing tests, and if that feature consists of two parts, it is
naturally expected of you to cover both parts with the new test,
unless there is a strong reason not to.  No?

I would understand if you said because of such and such reasons, we
should not cover the "two instances of violation, only one advice"
half of the feature, though.

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

[Rubén Justo]

On Fri, Mar 29, 2024 at 12:31:30PM -0700, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> >> This one also needs a similar justification, but with a twist.
> >
> > May I ask what you would find a good justification?
> >
> > Perhaps "newer" -> "now preferred"?
> 
> That is merely shifting justification around.  You'd now need to
> answer: Why do you consider it preferred?

Because it's newer ;-D

Maybe I'll point to the commit where advise_if_enabled() was introduced,
b3b18d1621 (advice: revamp advise API, 2020-03-02). We have some
arguments there.  I'll sleep on it.

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

[Junio C Hamano]

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

> On Fri, Mar 29, 2024 at 12:31:30PM -0700, Junio C Hamano wrote:
>> Rubén Justo <rjusto@gmail.com> writes:
>> 
>> >> This one also needs a similar justification, but with a twist.
>> >
>> > May I ask what you would find a good justification?
>> >
>> > Perhaps "newer" -> "now preferred"?
>> 
>> That is merely shifting justification around.  You'd now need to
>> answer: Why do you consider it preferred?
>
> Because it's newer ;-D

A newer thing is not necessarily better, though.

> Maybe I'll point to the commit where advise_if_enabled() was introduced,
> b3b18d1621 (advice: revamp advise API, 2020-03-02). We have some
> arguments there.  I'll sleep on it.

I think I've already given my verison of justification in the
message you are responding to.  I'll stop spending time on this
topic while you are sleeping on it ;-)

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

[Rubén Justo]

On Fri, Mar 29, 2024 at 12:31:30PM -0700, Junio C Hamano wrote:

> > Good point about the naming.  I'm not so sure about the "only once"
> > part, but I do not have any strong objection.
> 
> I am not sure what you are not sure about ;-).
> 
> If you are adding a test for a feature because it is not covered by
> existing tests, and if that feature consists of two parts, it is
> naturally expected of you to cover both parts with the new test,
> unless there is a strong reason not to.  No?

Sure.  However, I was not seeing that a whole.  Only testing the advice
message seemed sensible to me.

However, your proposed test covers the whole feature, and it shouldn't
be too challenging to modify the advice's text if we need to.  

So, it is better.

Thanks.

Re: [PATCH 0/3] add: use advise_if_enabled

[Junio C Hamano]

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

> This series is a simple change, in builtin/add.c, from:
>
> 	if (advice_enabled(XXX))
> 		advise(MMM)

I wonder if a coccinelle rule can automatically identify and rewrite
these ...

>
> to the newer:
>
> 	advise_if_enabled(XXX, MMM)

... to this form automatically.

Re: [PATCH 0/3] add: use advise_if_enabled

[Rubén Justo]

On Fri, Mar 29, 2024 at 10:28:34AM -0700, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > This series is a simple change, in builtin/add.c, from:
> >
> > 	if (advice_enabled(XXX))
> > 		advise(MMM)
> 
> I wonder if a coccinelle rule can automatically identify and rewrite
> these ...
> 
> >
> > to the newer:
> >
> > 	advise_if_enabled(XXX, MMM)
> 
> ... to this form automatically.

I don't have a solid opinion on that but, after a cursory review, I have
doubts a simple rule could catch enough cases to make it worth the
effort.

And, let's hope that the new code will utilize the new API for its
convenience.

[PATCH v2 0/3] add: use advise_if_enabled

[Rubén Justo]

This series is a simple change, in builtin/add.c, from:

	if (advice_enabled(XXX))
		advise(MMM)

to the newer:

	advise_if_enabled(XXX, MMM)

Rubén Justo (3):
  add: use advise_if_enabled for ADVICE_ADD_IGNORED_FILE
  add: use advise_if_enabled for ADVICE_ADD_EMPTY_PATHSPEC
  add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO

 builtin/add.c              | 18 ++++++---------
 t/t3700-add.sh             | 47 ++++++++++++++++++++++++++++++++++++--
 t/t7400-submodule-basic.sh |  3 +--
 3 files changed, 53 insertions(+), 15 deletions(-)

Range-diff against v1:
1:  c599ff8b98 ! 1:  9462b7045d add: use advise_if_enabled for ADVICE_ADD_IGNORED_FILE
    @@ Metadata
      ## Commit message ##
         add: use advise_if_enabled for ADVICE_ADD_IGNORED_FILE
     
    -    Use the newer advise_if_enabled() machinery to show the advice.
    +    Since b3b18d1621 (advice: revamp advise API, 2020-03-02), we can use
    +    advise_if_enabled() to display an advice.  This API encapsulates three
    +    actions:
    +            1.- checking the visibility of the advice
    +
    +            2.- displaying the advice when appropriate
    +
    +            3.- displaying instructions on how to disable the advice, when
    +                appropriate
    +
    +    The code we have in builtin/add.c to display the ADVICE_ADD_IGNORED_FILE
    +    advice, is doing these three things.  However, the instructions
    +    displayed on how to disable the hint are not shown in the normalized way
    +    that advise_if_enabled() introduced.  This may cause distraction.
    +
    +    There is no reason not to use the new API here.  On the contrary, by
    +    using it we gain simplicity in the code and avoid possible distractions.
    +
    +    For these reasons, use the newer advise_if_enabled() machinery to show
    +    the ADVICE_ADD_IGNORED_FILE advice, and don't bother checking the
    +    visibility or displaying the instruction on how to disable the advice.
     
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
     
2:  76550e01e1 ! 2:  f892013059 add: use advise_if_enabled for ADVICE_ADD_EMPTY_PATHSPEC
    @@ Metadata
      ## Commit message ##
         add: use advise_if_enabled for ADVICE_ADD_EMPTY_PATHSPEC
     
    -    Use the newer advise_if_enabled() machinery to show the advice.
    +    Since 93b0d86aaf (git-add: error out when given no arguments.,
    +    2006-12-20) we display a message when no arguments are given to "git
    +    add".
     
    -    We don't have a test for this.  Add one.
    +    Part of that message was converted to advice in bf66db37f1 (add: use
    +    advise function to display hints, 2020-01-07).
    +
    +    Following the same line of reasoning as in the previous commit, it is
    +    sensible to use advise_if_enabled() here.
    +
    +    Therefore, use advise_if_enabled() in builtin/add.c to show the
    +    ADVICE_ADD_EMPTY_PATHSPEC advice, and don't bother checking there the
    +    visibility of the advice or displaying the instruction on how to disable
    +    it.
    +
    +    Also add a test for these messages, in order to detect a possible
    +    change in them.
     
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
     
3:  3017dd2188 ! 3:  254ece0ee4 add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO
    @@ Metadata
      ## Commit message ##
         add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO
     
    -    Use the newer advise_if_enabled() machinery to show the advice.
    +    By following a similar reasoning as in previous commits, there are no
    +    reason why we should not use the advise_if_enabled() API to display the
    +    ADVICE_ADD_EMBEDDED_REPO advice.
     
    -    We don't have a test for this.  Add one.
    +    This advice was introduced in 532139940c (add: warn when adding an
    +    embedded repository, 2017-06-14).  Some tests were included in the
    +    commit, but none is testing this advice.  Which, note, we only want to
    +    display once per run.
     
    +    So, use the advise_if_enabled() machinery to show the
    +    ADVICE_ADD_EMBEDDED_REPO advice and include a test to notice any
    +    possible breakage.
    +
    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
     
      ## builtin/add.c ##
    @@ t/t3700-add.sh: test_expect_success '"git add ." in empty repo' '
      	)
      '
      
    -+test_expect_success '"git add" a nested repository' '
    -+	rm -fr empty &&
    -+	git init empty &&
    ++test_expect_success '"git add" a embedded repository' '
    ++	rm -fr outer && git init outer &&
     +	(
    -+		cd empty &&
    -+		git init empty &&
    -+		(
    -+			cd empty &&
    -+			git commit --allow-empty -m "foo"
    -+		) &&
    -+		git add empty 2>actual &&
    ++		cd outer &&
    ++		for i in 1 2
    ++		do
    ++			name=inner$i &&
    ++			git init $name &&
    ++			git -C $name commit --allow-empty -m $name ||
    ++				return 1
    ++		done &&
    ++		git add . 2>actual &&
     +		cat >expect <<-EOF &&
    -+		warning: adding embedded git repository: empty
    ++		warning: adding embedded git repository: inner1
     +		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: 	git submodule add <url> inner1
     +		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: 	git rm --cached inner1
     +		hint:
     +		hint: See "git help submodule" for more information.
     +		hint: Disable this message with "git config advice.addEmbeddedRepo false"
    ++		warning: adding embedded git repository: inner2
     +		EOF
     +		test_cmp expect actual
     +	)

base-commit: 2d8cf94b28de9da683ddd40961a3a572f2741cf3
-- 
2.44.0.417.g254ece0ee4

[PATCH v2 1/3] add: use advise_if_enabled for ADVICE_ADD_IGNORED_FILE

[Rubén Justo]

Since b3b18d1621 (advice: revamp advise API, 2020-03-02), we can use
advise_if_enabled() to display an advice.  This API encapsulates three
actions:
	1.- checking the visibility of the advice

	2.- displaying the advice when appropriate

	3.- displaying instructions on how to disable the advice, when
	    appropriate

The code we have in builtin/add.c to display the ADVICE_ADD_IGNORED_FILE
advice, is doing these three things.  However, the instructions
displayed on how to disable the hint are not shown in the normalized way
that advise_if_enabled() introduced.  This may cause distraction.

There is no reason not to use the new API here.  On the contrary, by
using it we gain simplicity in the code and avoid possible distractions.

For these reasons, use the newer advise_if_enabled() machinery to show
the ADVICE_ADD_IGNORED_FILE advice, and don't bother checking the
visibility or displaying the instruction on how to disable the advice.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/add.c              | 6 ++----
 t/t3700-add.sh             | 3 +--
 t/t7400-submodule-basic.sh | 3 +--
 3 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 393c10cbcf..8f148987f7 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -328,10 +328,8 @@ static int add_files(struct dir_struct *dir, int flags)
 		fprintf(stderr, _(ignore_error));
 		for (i = 0; i < dir->ignored_nr; i++)
 			fprintf(stderr, "%s\n", dir->ignored[i]->name);
-		if (advice_enabled(ADVICE_ADD_IGNORED_FILE))
-			advise(_("Use -f if you really want to add them.\n"
-				"Turn this message off by running\n"
-				"\"git config advice.addIgnoredFile false\""));
+		advise_if_enabled(ADVICE_ADD_IGNORED_FILE,
+				  _("Use -f if you really want to add them."));
 		exit_status = 1;
 	}
 
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index f23d39f0d5..76c2c9e7b0 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -370,8 +370,7 @@ cat >expect.err <<\EOF
 The following paths are ignored by one of your .gitignore files:
 ignored-file
 hint: Use -f if you really want to add them.
-hint: Turn this message off by running
-hint: "git config advice.addIgnoredFile false"
+hint: Disable this message with "git config advice.addIgnoredFile false"
 EOF
 cat >expect.out <<\EOF
 add 'track-this'
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 00c1f1aab1..5c4a89df5c 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -212,8 +212,7 @@ test_expect_success 'submodule add to .gitignored path fails' '
 		The following paths are ignored by one of your .gitignore files:
 		submod
 		hint: Use -f if you really want to add them.
-		hint: Turn this message off by running
-		hint: "git config advice.addIgnoredFile false"
+		hint: Disable this message with "git config advice.addIgnoredFile false"
 		EOF
 		# Does not use test_commit due to the ignore
 		echo "*" > .gitignore &&
-- 
2.44.0.417.g254ece0ee4

[PATCH v2 2/3] add: use advise_if_enabled for ADVICE_ADD_EMPTY_PATHSPEC

[Rubén Justo]

Since 93b0d86aaf (git-add: error out when given no arguments.,
2006-12-20) we display a message when no arguments are given to "git
add".

Part of that message was converted to advice in bf66db37f1 (add: use
advise function to display hints, 2020-01-07).

Following the same line of reasoning as in the previous commit, it is
sensible to use advise_if_enabled() here.

Therefore, use advise_if_enabled() in builtin/add.c to show the
ADVICE_ADD_EMPTY_PATHSPEC advice, and don't bother checking there the
visibility of the advice or displaying the instruction on how to disable
it.

Also add a test for these messages, in order to detect a possible
change in them.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/add.c  |  6 ++----
 t/t3700-add.sh | 10 ++++++++++
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 8f148987f7..289adaaecf 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -438,10 +438,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
 
 	if (require_pathspec && pathspec.nr == 0) {
 		fprintf(stderr, _("Nothing specified, nothing added.\n"));
-		if (advice_enabled(ADVICE_ADD_EMPTY_PATHSPEC))
-			advise( _("Maybe you wanted to say 'git add .'?\n"
-				"Turn this message off by running\n"
-				"\"git config advice.addEmptyPathspec false\""));
+		advise_if_enabled(ADVICE_ADD_EMPTY_PATHSPEC,
+				  _("Maybe you wanted to say 'git add .'?"));
 		return 0;
 	}
 
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 76c2c9e7b0..681081e0d5 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -28,6 +28,16 @@ test_expect_success 'Test of git add' '
 	touch foo && git add foo
 '
 
+test_expect_success 'Test with no pathspecs' '
+	cat >expect <<-EOF &&
+	Nothing specified, nothing added.
+	hint: Maybe you wanted to say ${SQ}git add .${SQ}?
+	hint: Disable this message with "git config advice.addEmptyPathspec false"
+	EOF
+	git add 2>actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'Post-check that foo is in the index' '
 	git ls-files foo | grep foo
 '
-- 
2.44.0.417.g254ece0ee4

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

[Rubén Justo]

By following a similar reasoning as in previous commits, there are no
reason why we should not use the advise_if_enabled() API to display the
ADVICE_ADD_EMBEDDED_REPO advice.

This advice was introduced in 532139940c (add: warn when adding an
embedded repository, 2017-06-14).  Some tests were included in the
commit, but none is testing this advice.  Which, note, we only want to
display once per run.

So, use the advise_if_enabled() machinery to show the
ADVICE_ADD_EMBEDDED_REPO advice and include a test to notice any
possible breakage.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/add.c  |  6 +++---
 t/t3700-add.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 37 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;
 	}
 
diff --git a/t/t3700-add.sh b/t/t3700-add.sh
index 681081e0d5..839c904745 100755
--- a/t/t3700-add.sh
+++ b/t/t3700-add.sh
@@ -349,6 +349,40 @@ test_expect_success '"git add ." in empty repo' '
 	)
 '
 
+test_expect_success '"git add" a embedded repository' '
+	rm -fr outer && git init outer &&
+	(
+		cd outer &&
+		for i in 1 2
+		do
+			name=inner$i &&
+			git init $name &&
+			git -C $name commit --allow-empty -m $name ||
+				return 1
+		done &&
+		git add . 2>actual &&
+		cat >expect <<-EOF &&
+		warning: adding embedded git repository: inner1
+		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> inner1
+		hint:
+		hint: If you added this path by mistake, you can remove it from the
+		hint: index with:
+		hint:
+		hint: 	git rm --cached inner1
+		hint:
+		hint: See "git help submodule" for more information.
+		hint: Disable this message with "git config advice.addEmbeddedRepo false"
+		warning: adding embedded git repository: inner2
+		EOF
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'error on a repository with no commits' '
 	rm -fr empty &&
 	git init empty &&
-- 
2.44.0.417.g254ece0ee4