{"thread":{"id":"61224","subject":"[PATCH 0/3] add: use advise_if_enabled","startedAt":"2024-03-29T04:15:03Z","lastAt":"2024-03-30T14:09:37Z","messageCount":17,"participants":["Rubén Justo","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"491805","messageId":"06c9b422-b22e-4310-ad5b-1686616ab860@gmail.com","threadId":"61224","inReplyTo":null,"subject":"[PATCH 0/3] add: use advise_if_enabled","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-29T04:14:53Z","receivedAt":"2024-03-29T04:15:03Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"This series is a simple change, in builtin/add.c, from:\n\n\tif (advice_enabled(XXX))\n\t\tadvise(MMM)\n\nto the newer:\n\n\tadvise_if_enabled(XXX, MMM)\n\n\nRubén Justo (3):\n  add: use advise_if_enabled for ADVICE_ADD_IGNORED_FILE\n  add: use advise_if_enabled for ADVICE_ADD_EMPTY_PATHSPEC\n  add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO\n\n builtin/add.c              | 18 ++++++---------\n t/t3700-add.sh             | 45 ++++++++++++++++++++++++++++++++++++--\n t/t7400-submodule-basic.sh |  3 +--\n 3 files changed, 51 insertions(+), 15 deletions(-)\n\n-- \n2.44.0.371.gf9813d4ed5\n"},{"id":"491810","messageId":"37f79186-b849-496a-ae91-f5e05a280dae@gmail.com","threadId":"61224","inReplyTo":"06c9b422-b22e-4310-ad5b-1686616ab860@gmail.com","subject":"[PATCH 1/3] add: use advise_if_enabled for ADVICE_ADD_IGNORED_FILE","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-29T04:19:03Z","receivedAt":"2024-03-29T04:19:20Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Use the newer advise_if_enabled() machinery to show the advice.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n builtin/add.c              | 6 ++----\n t/t3700-add.sh             | 3 +--\n t/t7400-submodule-basic.sh | 3 +--\n 3 files changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 393c10cbcf..8f148987f7 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -328,10 +328,8 @@ static int add_files(struct dir_struct *dir, int flags)\n \t\tfprintf(stderr, _(ignore_error));\n \t\tfor (i = 0; i < dir->ignored_nr; i++)\n \t\t\tfprintf(stderr, \"%s\\n\", dir->ignored[i]->name);\n-\t\tif (advice_enabled(ADVICE_ADD_IGNORED_FILE))\n-\t\t\tadvise(_(\"Use -f if you really want to add them.\\n\"\n-\t\t\t\t\"Turn this message off by running\\n\"\n-\t\t\t\t\"\\\"git config advice.addIgnoredFile false\\\"\"));\n+\t\tadvise_if_enabled(ADVICE_ADD_IGNORED_FILE,\n+\t\t\t\t  _(\"Use -f if you really want to add them.\"));\n \t\texit_status = 1;\n \t}\n \ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex f23d39f0d5..76c2c9e7b0 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -370,8 +370,7 @@ cat >expect.err <<\\EOF\n The following paths are ignored by one of your .gitignore files:\n ignored-file\n hint: Use -f if you really want to add them.\n-hint: Turn this message off by running\n-hint: \"git config advice.addIgnoredFile false\"\n+hint: Disable this message with \"git config advice.addIgnoredFile false\"\n EOF\n cat >expect.out <<\\EOF\n add 'track-this'\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 00c1f1aab1..5c4a89df5c 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -212,8 +212,7 @@ test_expect_success 'submodule add to .gitignored path fails' '\n \t\tThe following paths are ignored by one of your .gitignore files:\n \t\tsubmod\n \t\thint: Use -f if you really want to add them.\n-\t\thint: Turn this message off by running\n-\t\thint: \"git config advice.addIgnoredFile false\"\n+\t\thint: Disable this message with \"git config advice.addIgnoredFile false\"\n \t\tEOF\n \t\t# Does not use test_commit due to the ignore\n \t\techo \"*\" > .gitignore &&\n-- \n2.44.0.371.gf9813d4ed5\n"},{"id":"491811","messageId":"dd1f4753-b82f-47e4-a7f3-0d252579cc36@gmail.com","threadId":"61224","inReplyTo":"06c9b422-b22e-4310-ad5b-1686616ab860@gmail.com","subject":"[PATCH 2/3] add: use advise_if_enabled for ADVICE_ADD_EMPTY_PATHSPEC","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-29T04:19:29Z","receivedAt":"2024-03-29T04:19:49Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Use the newer advise_if_enabled() machinery to show the advice.\n\nWe don't have a test for this.  Add one.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n builtin/add.c  |  6 ++----\n t/t3700-add.sh | 10 ++++++++++\n 2 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 8f148987f7..289adaaecf 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -438,10 +438,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \n \tif (require_pathspec && pathspec.nr == 0) {\n \t\tfprintf(stderr, _(\"Nothing specified, nothing added.\\n\"));\n-\t\tif (advice_enabled(ADVICE_ADD_EMPTY_PATHSPEC))\n-\t\t\tadvise( _(\"Maybe you wanted to say 'git add .'?\\n\"\n-\t\t\t\t\"Turn this message off by running\\n\"\n-\t\t\t\t\"\\\"git config advice.addEmptyPathspec false\\\"\"));\n+\t\tadvise_if_enabled(ADVICE_ADD_EMPTY_PATHSPEC,\n+\t\t\t\t  _(\"Maybe you wanted to say 'git add .'?\"));\n \t\treturn 0;\n \t}\n \ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex 76c2c9e7b0..681081e0d5 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -28,6 +28,16 @@ test_expect_success 'Test of git add' '\n \ttouch foo && git add foo\n '\n \n+test_expect_success 'Test with no pathspecs' '\n+\tcat >expect <<-EOF &&\n+\tNothing specified, nothing added.\n+\thint: Maybe you wanted to say ${SQ}git add .${SQ}?\n+\thint: Disable this message with \"git config advice.addEmptyPathspec false\"\n+\tEOF\n+\tgit add 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'Post-check that foo is in the index' '\n \tgit ls-files foo | grep foo\n '\n-- \n2.44.0.371.gf9813d4ed5\n"},{"id":"491812","messageId":"0e38da05-efd6-451e-bd8a-b2b3457c0c75@gmail.com","threadId":"61224","inReplyTo":"06c9b422-b22e-4310-ad5b-1686616ab860@gmail.com","subject":"[PATCH 3/3] add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-29T04:19:43Z","receivedAt":"2024-03-29T04:19:56Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Use the newer advise_if_enabled() machinery to show the advice.\n\nWe don't have a test for this.  Add one.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n builtin/add.c  |  6 +++---\n t/t3700-add.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 35 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 289adaaecf..e97699d6b9 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -310,9 +310,9 @@ static void check_embedded_repo(const char *path)\n \tstrbuf_strip_suffix(&name, \"/\");\n \n \twarning(_(\"adding embedded git repository: %s\"), name.buf);\n-\tif (!adviced_on_embedded_repo &&\n-\t    advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) {\n-\t\tadvise(embedded_advice, name.buf, name.buf);\n+\tif (!adviced_on_embedded_repo) {\n+\t\tadvise_if_enabled(ADVICE_ADD_EMBEDDED_REPO,\n+\t\t\t\t  embedded_advice, name.buf, name.buf);\n \t\tadviced_on_embedded_repo = 1;\n \t}\n \ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex 681081e0d5..2b92f3eb5b 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -349,6 +349,38 @@ test_expect_success '\"git add .\" in empty repo' '\n \t)\n '\n \n+test_expect_success '\"git add\" a nested repository' '\n+\trm -fr empty &&\n+\tgit init empty &&\n+\t(\n+\t\tcd empty &&\n+\t\tgit init empty &&\n+\t\t(\n+\t\t\tcd empty &&\n+\t\t\tgit commit --allow-empty -m \"foo\"\n+\t\t) &&\n+\t\tgit add empty 2>actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\twarning: adding embedded git repository: empty\n+\t\thint: You${SQ}ve added another git repository inside your current repository.\n+\t\thint: Clones of the outer repository will not contain the contents of\n+\t\thint: the embedded repository and will not know how to obtain it.\n+\t\thint: If you meant to add a submodule, use:\n+\t\thint: \n+\t\thint: \tgit submodule add <url> empty\n+\t\thint: \n+\t\thint: If you added this path by mistake, you can remove it from the\n+\t\thint: index with:\n+\t\thint: \n+\t\thint: \tgit rm --cached empty\n+\t\thint: \n+\t\thint: See \"git help submodule\" for more information.\n+\t\thint: Disable this message with \"git config advice.addEmbeddedRepo false\"\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'error on a repository with no commits' '\n \trm -fr empty &&\n \tgit init empty &&\n-- \n2.44.0.371.gf9813d4ed5\n\n"},{"id":"491834","messageId":"xmqq1q7t0wxp.fsf@gitster.g","threadId":"61224","inReplyTo":"06c9b422-b22e-4310-ad5b-1686616ab860@gmail.com","subject":"Re: [PATCH 0/3] add: use advise_if_enabled","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-29T17:28:34Z","receivedAt":"2024-03-29T17:28:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> This series is a simple change, in builtin/add.c, from:\n>\n> \tif (advice_enabled(XXX))\n> \t\tadvise(MMM)\n\nI wonder if a coccinelle rule can automatically identify and rewrite\nthese ...\n\n>\n> to the newer:\n>\n> \tadvise_if_enabled(XXX, MMM)\n\n... to this form automatically.\n"},{"id":"491835","messageId":"xmqqv855ym16.fsf@gitster.g","threadId":"61224","inReplyTo":"37f79186-b849-496a-ae91-f5e05a280dae@gmail.com","subject":"Re: [PATCH 1/3] add: use advise_if_enabled for ADVICE_ADD_IGNORED_FILE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-29T17:40:05Z","receivedAt":"2024-03-29T17:40:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> -\t\tif (advice_enabled(ADVICE_ADD_IGNORED_FILE))\n> -\t\t\tadvise(_(\"Use -f if you really want to add them.\\n\"\n> -\t\t\t\t\"Turn this message off by running\\n\"\n> -\t\t\t\t\"\\\"git config advice.addIgnoredFile false\\\"\"));\n> +\t\tadvise_if_enabled(ADVICE_ADD_IGNORED_FILE,\n> +\t\t\t\t  _(\"Use -f if you really want to add them.\"));\n\nGood.\n\n>  \t\texit_status = 1;\n>  \t}\n>  \n> diff --git a/t/t3700-add.sh b/t/t3700-add.sh\n> index f23d39f0d5..76c2c9e7b0 100755\n> --- a/t/t3700-add.sh\n> +++ b/t/t3700-add.sh\n> @@ -370,8 +370,7 @@ cat >expect.err <<\\EOF\n>  The following paths are ignored by one of your .gitignore files:\n>  ignored-file\n>  hint: Use -f if you really want to add them.\n> -hint: Turn this message off by running\n> -hint: \"git config advice.addIgnoredFile false\"\n> +hint: Disable this message with \"git config advice.addIgnoredFile false\"\n\nFunny that we weremanually crafting the hint to turn it off.  Nice\nto see that code go.\n\n>  EOF\n>  cat >expect.out <<\\EOF\n>  add 'track-this'\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 00c1f1aab1..5c4a89df5c 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -212,8 +212,7 @@ test_expect_success 'submodule add to .gitignored path fails' '\n>  \t\tThe following paths are ignored by one of your .gitignore files:\n>  \t\tsubmod\n>  \t\thint: Use -f if you really want to add them.\n> -\t\thint: Turn this message off by running\n> -\t\thint: \"git config advice.addIgnoredFile false\"\n> +\t\thint: Disable this message with \"git config advice.addIgnoredFile false\"\n>  \t\tEOF\n>  \t\t# Does not use test_commit due to the ignore\n>  \t\techo \"*\" > .gitignore &&\n"},{"id":"491837","messageId":"xmqqcyrczzv7.fsf@gitster.g","threadId":"61224","inReplyTo":"0e38da05-efd6-451e-bd8a-b2b3457c0c75@gmail.com","subject":"Re: [PATCH 3/3] add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-29T17:55:56Z","receivedAt":"2024-03-29T17:56:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> Use the newer advise_if_enabled() machinery to show the advice.\n\nCommon to the other two patches, but \"Newer\" is not a good enough\nexcuse if the existing code is working well for us and not being\nmaintenance burden.  The previous two patches were helped by use of\nadvise_if_enabled() in a concrete way (or perhaps two ways), and\nthat should be explained when selling them.\n\nThis one also needs a similar justification, but with a twist.\n\n> We don't have a test for this.  Add one.\n>\n> Signed-off-by: Rubén Justo <rjusto@gmail.com>\n> ---\n>  builtin/add.c  |  6 +++---\n>  t/t3700-add.sh | 32 ++++++++++++++++++++++++++++++++\n>  2 files changed, 35 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/add.c b/builtin/add.c\n> index 289adaaecf..e97699d6b9 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -310,9 +310,9 @@ static void check_embedded_repo(const char *path)\n>  \tstrbuf_strip_suffix(&name, \"/\");\n>  \n>  \twarning(_(\"adding embedded git repository: %s\"), name.buf);\n> -\tif (!adviced_on_embedded_repo &&\n> -\t    advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) {\n> -\t\tadvise(embedded_advice, name.buf, name.buf);\n> +\tif (!adviced_on_embedded_repo) {\n> +\t\tadvise_if_enabled(ADVICE_ADD_EMBEDDED_REPO,\n> +\t\t\t\t  embedded_advice, name.buf, name.buf);\n>  \t\tadviced_on_embedded_repo = 1;\n>  \t}\n\nThis uses a static variable \"adviced_on_embedded_repo\" to skip\ngiving the advice messages over and over.  The patch preserves\nthat feature of this code while updating it to use the \"if_enabled\"\nvariant.\n\n> diff --git a/t/t3700-add.sh b/t/t3700-add.sh\n> index 681081e0d5..2b92f3eb5b 100755\n> --- a/t/t3700-add.sh\n> +++ b/t/t3700-add.sh\n> @@ -349,6 +349,38 @@ test_expect_success '\"git add .\" in empty repo' '\n>  \t)\n>  '\n>  \n> +test_expect_success '\"git add\" a nested repository' '\n\n\"nested\" -> \"embedded\", as the warning, advice_type and the message\ncontents all use \"embedded\" consistently.\n\n> +\trm -fr empty &&\n> +\tgit init empty &&\n> +\t(\n> +\t\tcd empty &&\n> +\t\tgit init empty &&\n> +\t\t(\n> +\t\t\tcd empty &&\n> +\t\t\tgit commit --allow-empty -m \"foo\"\n> +\t\t) &&\n> +\t\tgit add empty 2>actual &&\n\nIt is very good to add a test for a feature that we failed to cover\nso far.  But the feature, as we seen above, is twofold.  We see an\nadvice, and we it see only once even when we have multiple.\n\nSo we should add two such embedded repositories for the test, no?\nAlso, the shell repository is not meant to stay empty as the user\nwill make a mistaken attempt to \"add\" something to it.\n\nPerhaps the above part would become more like:\n\n\trm -rf outer && git init outer &&\n\t(\n\t\tcd outer &&\n\t\tfor i in 1 2\n\t\tdo\n\t\t\tname=inner$i &&\n\t\t\tgit init $name &&\n                        git -C $name --allow-empty -m $name ||\n\t\t\t\treturn 1\n\t\tdone &&\n                git add . 2>actual &&\n\nto use a more descriptive name that shows the point of the test (it\nis not interesting that they are empty---they are in \"outer contains\ninnner repositories\" relationship and that is what the test wants to\nmake), and ensure \"only once\" part of the feature we are testing.\n\n> +\t\tcat >expect <<-EOF &&\n> +\t\twarning: adding embedded git repository: empty\n> +\t\thint: You${SQ}ve added another git repository inside your current repository.\n> +\t\thint: Clones of the outer repository will not contain the contents of\n> +\t\thint: the embedded repository and will not know how to obtain it.\n> +\t\thint: If you meant to add a submodule, use:\n> +\t\thint: \n> +\t\thint: \tgit submodule add <url> empty\n> +\t\thint: \n> +\t\thint: If you added this path by mistake, you can remove it from the\n> +\t\thint: index with:\n> +\t\thint: \n> +\t\thint: \tgit rm --cached empty\n> +\t\thint: \n> +\t\thint: See \"git help submodule\" for more information.\n> +\t\thint: Disable this message with \"git config advice.addEmbeddedRepo false\"\n> +\t\tEOF\n> +\t\ttest_cmp expect actual\n> +\t)\n> +'\n> +\n>  test_expect_success 'error on a repository with no commits' '\n>  \trm -fr empty &&\n>  \tgit init empty &&\n"},{"id":"491843","messageId":"de916779-3c0c-4cf0-b78e-d0536c65af0e@gmail.com","threadId":"61224","inReplyTo":"xmqqcyrczzv7.fsf@gitster.g","subject":"Re: [PATCH 3/3] add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-29T19:04:04Z","receivedAt":"2024-03-29T19:04:10Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Fri, Mar 29, 2024 at 10:55:56AM -0700, Junio C Hamano wrote:\n> Rubén Justo <rjusto@gmail.com> writes:\n> \n> > Use the newer advise_if_enabled() machinery to show the advice.\n> \n> Common to the other two patches, but \"Newer\" is not a good enough\n> excuse if the existing code is working well for us and not being\n> maintenance burden.  The previous two patches were helped by use of\n> advise_if_enabled() in a concrete way (or perhaps two ways), and\n> that should be explained when selling them.\n> \n> This one also needs a similar justification, but with a twist.\n\nMay I ask what you would find a good justification?\n\nPerhaps \"newer\" -> \"now preferred\"?\n\n> > +test_expect_success '\"git add\" a nested repository' '\n> \n> \"nested\" -> \"embedded\", as the warning, advice_type and the message\n> contents all use \"embedded\" consistently.\n\nMakes sense.\n\n> > +\trm -fr empty &&\n> > +\tgit init empty &&\n> > +\t(\n> > +\t\tcd empty &&\n> > +\t\tgit init empty &&\n> > +\t\t(\n> > +\t\t\tcd empty &&\n> > +\t\t\tgit commit --allow-empty -m \"foo\"\n> > +\t\t) &&\n> > +\t\tgit add empty 2>actual &&\n> \n> It is very good to add a test for a feature that we failed to cover\n> so far.  But the feature, as we seen above, is twofold.  We see an\n> advice, and we it see only once even when we have multiple.\n> \n> So we should add two such embedded repositories for the test, no?\n> Also, the shell repository is not meant to stay empty as the user\n> will make a mistaken attempt to \"add\" something to it.\n> \n> Perhaps the above part would become more like:\n> \n> \trm -rf outer && git init outer &&\n> \t(\n> \t\tcd outer &&\n> \t\tfor i in 1 2\n> \t\tdo\n> \t\t\tname=inner$i &&\n> \t\t\tgit init $name &&\n>                         git -C $name --allow-empty -m $name ||\n> \t\t\t\treturn 1\n> \t\tdone &&\n>                 git add . 2>actual &&\n> \n> to use a more descriptive name that shows the point of the test (it\n> is not interesting that they are empty---they are in \"outer contains\n> innner repositories\" relationship and that is what the test wants to\n> make), and ensure \"only once\" part of the feature we are testing.\n\nGood point about the naming.  I'm not so sure about the \"only once\"\npart, but I do not have any strong objection.\n\nThanks.\n"},{"id":"491844","messageId":"1c3bc5ba-0093-4a34-ade2-d6762e9754b3@gmail.com","threadId":"61224","inReplyTo":"xmqq1q7t0wxp.fsf@gitster.g","subject":"Re: [PATCH 0/3] add: use advise_if_enabled","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-29T19:16:27Z","receivedAt":"2024-03-29T19:16:33Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Fri, Mar 29, 2024 at 10:28:34AM -0700, Junio C Hamano wrote:\n> Rubén Justo <rjusto@gmail.com> writes:\n> \n> > This series is a simple change, in builtin/add.c, from:\n> >\n> > \tif (advice_enabled(XXX))\n> > \t\tadvise(MMM)\n> \n> I wonder if a coccinelle rule can automatically identify and rewrite\n> these ...\n> \n> >\n> > to the newer:\n> >\n> > \tadvise_if_enabled(XXX, MMM)\n> \n> ... to this form automatically.\n\nI don't have a solid opinion on that but, after a cursory review, I have\ndoubts a simple rule could catch enough cases to make it worth the\neffort.\n\nAnd, let's hope that the new code will utilize the new API for its\nconvenience.\n"},{"id":"491847","messageId":"xmqqjzlkygvh.fsf@gitster.g","threadId":"61224","inReplyTo":"de916779-3c0c-4cf0-b78e-d0536c65af0e@gmail.com","subject":"Re: [PATCH 3/3] add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-29T19:31:30Z","receivedAt":"2024-03-29T19:31:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n>> This one also needs a similar justification, but with a twist.\n>\n> May I ask what you would find a good justification?\n>\n> Perhaps \"newer\" -> \"now preferred\"?\n\nThat is merely shifting justification around.  You'd now need to\nanswer: Why do you consider it preferred?\n\nA few obvious advantages of using if_enabled() form are:\n\n - you do not have to give \"here is how to disable this piece of advice\"\n   yourself.\n\n - the code can become shorter.\n\nand they may make it preferred to use it, when appropriate.\n\n> Good point about the naming.  I'm not so sure about the \"only once\"\n> part, but I do not have any strong objection.\n\nI am not sure what you are not sure about ;-).\n\nIf you are adding a test for a feature because it is not covered by\nexisting tests, and if that feature consists of two parts, it is\nnaturally expected of you to cover both parts with the new test,\nunless there is a strong reason not to.  No?\n\nI would understand if you said because of such and such reasons, we\nshould not cover the \"two instances of violation, only one advice\"\nhalf of the feature, though.\n"},{"id":"491850","messageId":"a76a0935-78e6-4ad3-9bf9-989f5ed53b84@gmail.com","threadId":"61224","inReplyTo":"xmqqjzlkygvh.fsf@gitster.g","subject":"Re: [PATCH 3/3] add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-29T19:59:27Z","receivedAt":"2024-03-29T19:59:35Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Fri, Mar 29, 2024 at 12:31:30PM -0700, Junio C Hamano wrote:\n> Rubén Justo <rjusto@gmail.com> writes:\n> \n> >> This one also needs a similar justification, but with a twist.\n> >\n> > May I ask what you would find a good justification?\n> >\n> > Perhaps \"newer\" -> \"now preferred\"?\n> \n> That is merely shifting justification around.  You'd now need to\n> answer: Why do you consider it preferred?\n\nBecause it's newer ;-D\n\nMaybe I'll point to the commit where advise_if_enabled() was introduced,\nb3b18d1621 (advice: revamp advise API, 2020-03-02). We have some\narguments there.  I'll sleep on it.\n"},{"id":"491852","messageId":"xmqq8r20yct8.fsf@gitster.g","threadId":"61224","inReplyTo":"a76a0935-78e6-4ad3-9bf9-989f5ed53b84@gmail.com","subject":"Re: [PATCH 3/3] add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-29T20:59:15Z","receivedAt":"2024-03-29T20:59:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rubén Justo <rjusto@gmail.com> writes:\n\n> On Fri, Mar 29, 2024 at 12:31:30PM -0700, Junio C Hamano wrote:\n>> Rubén Justo <rjusto@gmail.com> writes:\n>> \n>> >> This one also needs a similar justification, but with a twist.\n>> >\n>> > May I ask what you would find a good justification?\n>> >\n>> > Perhaps \"newer\" -> \"now preferred\"?\n>> \n>> That is merely shifting justification around.  You'd now need to\n>> answer: Why do you consider it preferred?\n>\n> Because it's newer ;-D\n\nA newer thing is not necessarily better, though.\n\n> Maybe I'll point to the commit where advise_if_enabled() was introduced,\n> b3b18d1621 (advice: revamp advise API, 2020-03-02). We have some\n> arguments there.  I'll sleep on it.\n\nI think I've already given my verison of justification in the\nmessage you are responding to.  I'll stop spending time on this\ntopic while you are sleeping on it ;-)\n\n"},{"id":"491880","messageId":"df826861-2938-4652-85ca-c81254af5b2b@gmail.com","threadId":"61224","inReplyTo":"xmqqjzlkygvh.fsf@gitster.g","subject":"Re: [PATCH 3/3] add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-30T13:35:47Z","receivedAt":"2024-03-30T13:36:00Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"On Fri, Mar 29, 2024 at 12:31:30PM -0700, Junio C Hamano wrote:\n\n> > Good point about the naming.  I'm not so sure about the \"only once\"\n> > part, but I do not have any strong objection.\n> \n> I am not sure what you are not sure about ;-).\n> \n> If you are adding a test for a feature because it is not covered by\n> existing tests, and if that feature consists of two parts, it is\n> naturally expected of you to cover both parts with the new test,\n> unless there is a strong reason not to.  No?\n\nSure.  However, I was not seeing that a whole.  Only testing the advice\nmessage seemed sensible to me.\n\nHowever, your proposed test covers the whole feature, and it shouldn't\nbe too challenging to modify the advice's text if we need to.  \n\nSo, it is better.\n\nThanks.\n"},{"id":"491882","messageId":"46fba030-d7aa-49d2-88fa-e506850f7b6a@gmail.com","threadId":"61224","inReplyTo":"06c9b422-b22e-4310-ad5b-1686616ab860@gmail.com","subject":"[PATCH v2 0/3] add: use advise_if_enabled","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-30T14:00:11Z","receivedAt":"2024-03-30T14:00:18Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"This series is a simple change, in builtin/add.c, from:\n\n\tif (advice_enabled(XXX))\n\t\tadvise(MMM)\n\nto the newer:\n\n\tadvise_if_enabled(XXX, MMM)\n\nRubén Justo (3):\n  add: use advise_if_enabled for ADVICE_ADD_IGNORED_FILE\n  add: use advise_if_enabled for ADVICE_ADD_EMPTY_PATHSPEC\n  add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO\n\n builtin/add.c              | 18 ++++++---------\n t/t3700-add.sh             | 47 ++++++++++++++++++++++++++++++++++++--\n t/t7400-submodule-basic.sh |  3 +--\n 3 files changed, 53 insertions(+), 15 deletions(-)\n\nRange-diff against v1:\n1:  c599ff8b98 ! 1:  9462b7045d add: use advise_if_enabled for ADVICE_ADD_IGNORED_FILE\n    @@ Metadata\n      ## Commit message ##\n         add: use advise_if_enabled for ADVICE_ADD_IGNORED_FILE\n     \n    -    Use the newer advise_if_enabled() machinery to show the advice.\n    +    Since b3b18d1621 (advice: revamp advise API, 2020-03-02), we can use\n    +    advise_if_enabled() to display an advice.  This API encapsulates three\n    +    actions:\n    +            1.- checking the visibility of the advice\n    +\n    +            2.- displaying the advice when appropriate\n    +\n    +            3.- displaying instructions on how to disable the advice, when\n    +                appropriate\n    +\n    +    The code we have in builtin/add.c to display the ADVICE_ADD_IGNORED_FILE\n    +    advice, is doing these three things.  However, the instructions\n    +    displayed on how to disable the hint are not shown in the normalized way\n    +    that advise_if_enabled() introduced.  This may cause distraction.\n    +\n    +    There is no reason not to use the new API here.  On the contrary, by\n    +    using it we gain simplicity in the code and avoid possible distractions.\n    +\n    +    For these reasons, use the newer advise_if_enabled() machinery to show\n    +    the ADVICE_ADD_IGNORED_FILE advice, and don't bother checking the\n    +    visibility or displaying the instruction on how to disable the advice.\n     \n         Signed-off-by: Rubén Justo <rjusto@gmail.com>\n     \n2:  76550e01e1 ! 2:  f892013059 add: use advise_if_enabled for ADVICE_ADD_EMPTY_PATHSPEC\n    @@ Metadata\n      ## Commit message ##\n         add: use advise_if_enabled for ADVICE_ADD_EMPTY_PATHSPEC\n     \n    -    Use the newer advise_if_enabled() machinery to show the advice.\n    +    Since 93b0d86aaf (git-add: error out when given no arguments.,\n    +    2006-12-20) we display a message when no arguments are given to \"git\n    +    add\".\n     \n    -    We don't have a test for this.  Add one.\n    +    Part of that message was converted to advice in bf66db37f1 (add: use\n    +    advise function to display hints, 2020-01-07).\n    +\n    +    Following the same line of reasoning as in the previous commit, it is\n    +    sensible to use advise_if_enabled() here.\n    +\n    +    Therefore, use advise_if_enabled() in builtin/add.c to show the\n    +    ADVICE_ADD_EMPTY_PATHSPEC advice, and don't bother checking there the\n    +    visibility of the advice or displaying the instruction on how to disable\n    +    it.\n    +\n    +    Also add a test for these messages, in order to detect a possible\n    +    change in them.\n     \n         Signed-off-by: Rubén Justo <rjusto@gmail.com>\n     \n3:  3017dd2188 ! 3:  254ece0ee4 add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO\n    @@ Metadata\n      ## Commit message ##\n         add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO\n     \n    -    Use the newer advise_if_enabled() machinery to show the advice.\n    +    By following a similar reasoning as in previous commits, there are no\n    +    reason why we should not use the advise_if_enabled() API to display the\n    +    ADVICE_ADD_EMBEDDED_REPO advice.\n     \n    -    We don't have a test for this.  Add one.\n    +    This advice was introduced in 532139940c (add: warn when adding an\n    +    embedded repository, 2017-06-14).  Some tests were included in the\n    +    commit, but none is testing this advice.  Which, note, we only want to\n    +    display once per run.\n     \n    +    So, use the advise_if_enabled() machinery to show the\n    +    ADVICE_ADD_EMBEDDED_REPO advice and include a test to notice any\n    +    possible breakage.\n    +\n    +    Signed-off-by: Junio C Hamano <gitster@pobox.com>\n         Signed-off-by: Rubén Justo <rjusto@gmail.com>\n     \n      ## builtin/add.c ##\n    @@ t/t3700-add.sh: test_expect_success '\"git add .\" in empty repo' '\n      \t)\n      '\n      \n    -+test_expect_success '\"git add\" a nested repository' '\n    -+\trm -fr empty &&\n    -+\tgit init empty &&\n    ++test_expect_success '\"git add\" a embedded repository' '\n    ++\trm -fr outer && git init outer &&\n     +\t(\n    -+\t\tcd empty &&\n    -+\t\tgit init empty &&\n    -+\t\t(\n    -+\t\t\tcd empty &&\n    -+\t\t\tgit commit --allow-empty -m \"foo\"\n    -+\t\t) &&\n    -+\t\tgit add empty 2>actual &&\n    ++\t\tcd outer &&\n    ++\t\tfor i in 1 2\n    ++\t\tdo\n    ++\t\t\tname=inner$i &&\n    ++\t\t\tgit init $name &&\n    ++\t\t\tgit -C $name commit --allow-empty -m $name ||\n    ++\t\t\t\treturn 1\n    ++\t\tdone &&\n    ++\t\tgit add . 2>actual &&\n     +\t\tcat >expect <<-EOF &&\n    -+\t\twarning: adding embedded git repository: empty\n    ++\t\twarning: adding embedded git repository: inner1\n     +\t\thint: You${SQ}ve added another git repository inside your current repository.\n     +\t\thint: Clones of the outer repository will not contain the contents of\n     +\t\thint: the embedded repository and will not know how to obtain it.\n     +\t\thint: If you meant to add a submodule, use:\n     +\t\thint:\n    -+\t\thint: \tgit submodule add <url> empty\n    ++\t\thint: \tgit submodule add <url> inner1\n     +\t\thint:\n     +\t\thint: If you added this path by mistake, you can remove it from the\n     +\t\thint: index with:\n     +\t\thint:\n    -+\t\thint: \tgit rm --cached empty\n    ++\t\thint: \tgit rm --cached inner1\n     +\t\thint:\n     +\t\thint: See \"git help submodule\" for more information.\n     +\t\thint: Disable this message with \"git config advice.addEmbeddedRepo false\"\n    ++\t\twarning: adding embedded git repository: inner2\n     +\t\tEOF\n     +\t\ttest_cmp expect actual\n     +\t)\n\nbase-commit: 2d8cf94b28de9da683ddd40961a3a572f2741cf3\n-- \n2.44.0.417.g254ece0ee4\n"},{"id":"491883","messageId":"57bbf2a2-1596-4b7c-a467-82b7b5b22a20@gmail.com","threadId":"61224","inReplyTo":"46fba030-d7aa-49d2-88fa-e506850f7b6a@gmail.com","subject":"[PATCH v2 1/3] add: use advise_if_enabled for ADVICE_ADD_IGNORED_FILE","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-30T14:07:27Z","receivedAt":"2024-03-30T14:07:34Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Since b3b18d1621 (advice: revamp advise API, 2020-03-02), we can use\nadvise_if_enabled() to display an advice.  This API encapsulates three\nactions:\n\t1.- checking the visibility of the advice\n\n\t2.- displaying the advice when appropriate\n\n\t3.- displaying instructions on how to disable the advice, when\n\t    appropriate\n\nThe code we have in builtin/add.c to display the ADVICE_ADD_IGNORED_FILE\nadvice, is doing these three things.  However, the instructions\ndisplayed on how to disable the hint are not shown in the normalized way\nthat advise_if_enabled() introduced.  This may cause distraction.\n\nThere is no reason not to use the new API here.  On the contrary, by\nusing it we gain simplicity in the code and avoid possible distractions.\n\nFor these reasons, use the newer advise_if_enabled() machinery to show\nthe ADVICE_ADD_IGNORED_FILE advice, and don't bother checking the\nvisibility or displaying the instruction on how to disable the advice.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n builtin/add.c              | 6 ++----\n t/t3700-add.sh             | 3 +--\n t/t7400-submodule-basic.sh | 3 +--\n 3 files changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 393c10cbcf..8f148987f7 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -328,10 +328,8 @@ static int add_files(struct dir_struct *dir, int flags)\n \t\tfprintf(stderr, _(ignore_error));\n \t\tfor (i = 0; i < dir->ignored_nr; i++)\n \t\t\tfprintf(stderr, \"%s\\n\", dir->ignored[i]->name);\n-\t\tif (advice_enabled(ADVICE_ADD_IGNORED_FILE))\n-\t\t\tadvise(_(\"Use -f if you really want to add them.\\n\"\n-\t\t\t\t\"Turn this message off by running\\n\"\n-\t\t\t\t\"\\\"git config advice.addIgnoredFile false\\\"\"));\n+\t\tadvise_if_enabled(ADVICE_ADD_IGNORED_FILE,\n+\t\t\t\t  _(\"Use -f if you really want to add them.\"));\n \t\texit_status = 1;\n \t}\n \ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex f23d39f0d5..76c2c9e7b0 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -370,8 +370,7 @@ cat >expect.err <<\\EOF\n The following paths are ignored by one of your .gitignore files:\n ignored-file\n hint: Use -f if you really want to add them.\n-hint: Turn this message off by running\n-hint: \"git config advice.addIgnoredFile false\"\n+hint: Disable this message with \"git config advice.addIgnoredFile false\"\n EOF\n cat >expect.out <<\\EOF\n add 'track-this'\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 00c1f1aab1..5c4a89df5c 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -212,8 +212,7 @@ test_expect_success 'submodule add to .gitignored path fails' '\n \t\tThe following paths are ignored by one of your .gitignore files:\n \t\tsubmod\n \t\thint: Use -f if you really want to add them.\n-\t\thint: Turn this message off by running\n-\t\thint: \"git config advice.addIgnoredFile false\"\n+\t\thint: Disable this message with \"git config advice.addIgnoredFile false\"\n \t\tEOF\n \t\t# Does not use test_commit due to the ignore\n \t\techo \"*\" > .gitignore &&\n-- \n2.44.0.417.g254ece0ee4\n"},{"id":"491884","messageId":"444bb0b2-781a-4eb7-a251-a1bbee2d87ce@gmail.com","threadId":"61224","inReplyTo":"46fba030-d7aa-49d2-88fa-e506850f7b6a@gmail.com","subject":"[PATCH v2 2/3] add: use advise_if_enabled for ADVICE_ADD_EMPTY_PATHSPEC","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-30T14:08:59Z","receivedAt":"2024-03-30T14:09:08Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"Since 93b0d86aaf (git-add: error out when given no arguments.,\n2006-12-20) we display a message when no arguments are given to \"git\nadd\".\n\nPart of that message was converted to advice in bf66db37f1 (add: use\nadvise function to display hints, 2020-01-07).\n\nFollowing the same line of reasoning as in the previous commit, it is\nsensible to use advise_if_enabled() here.\n\nTherefore, use advise_if_enabled() in builtin/add.c to show the\nADVICE_ADD_EMPTY_PATHSPEC advice, and don't bother checking there the\nvisibility of the advice or displaying the instruction on how to disable\nit.\n\nAlso add a test for these messages, in order to detect a possible\nchange in them.\n\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n builtin/add.c  |  6 ++----\n t/t3700-add.sh | 10 ++++++++++\n 2 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 8f148987f7..289adaaecf 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -438,10 +438,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \n \tif (require_pathspec && pathspec.nr == 0) {\n \t\tfprintf(stderr, _(\"Nothing specified, nothing added.\\n\"));\n-\t\tif (advice_enabled(ADVICE_ADD_EMPTY_PATHSPEC))\n-\t\t\tadvise( _(\"Maybe you wanted to say 'git add .'?\\n\"\n-\t\t\t\t\"Turn this message off by running\\n\"\n-\t\t\t\t\"\\\"git config advice.addEmptyPathspec false\\\"\"));\n+\t\tadvise_if_enabled(ADVICE_ADD_EMPTY_PATHSPEC,\n+\t\t\t\t  _(\"Maybe you wanted to say 'git add .'?\"));\n \t\treturn 0;\n \t}\n \ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex 76c2c9e7b0..681081e0d5 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -28,6 +28,16 @@ test_expect_success 'Test of git add' '\n \ttouch foo && git add foo\n '\n \n+test_expect_success 'Test with no pathspecs' '\n+\tcat >expect <<-EOF &&\n+\tNothing specified, nothing added.\n+\thint: Maybe you wanted to say ${SQ}git add .${SQ}?\n+\thint: Disable this message with \"git config advice.addEmptyPathspec false\"\n+\tEOF\n+\tgit add 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'Post-check that foo is in the index' '\n \tgit ls-files foo | grep foo\n '\n-- \n2.44.0.417.g254ece0ee4\n"},{"id":"491885","messageId":"aade9864-660c-4238-92ee-4a67758bedc6@gmail.com","threadId":"61224","inReplyTo":"46fba030-d7aa-49d2-88fa-e506850f7b6a@gmail.com","subject":"[PATCH v2 3/3] add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO","fromName":"Rubén Justo","fromEmail":"rjusto@gmail.com","sentAt":"2024-03-30T14:09:29Z","receivedAt":"2024-03-30T14:09:37Z","isPatch":true,"sender":{"key":"rjusto@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5685487?v=4"},"body":"By following a similar reasoning as in previous commits, there are no\nreason why we should not use the advise_if_enabled() API to display the\nADVICE_ADD_EMBEDDED_REPO advice.\n\nThis advice was introduced in 532139940c (add: warn when adding an\nembedded repository, 2017-06-14).  Some tests were included in the\ncommit, but none is testing this advice.  Which, note, we only want to\ndisplay once per run.\n\nSo, use the advise_if_enabled() machinery to show the\nADVICE_ADD_EMBEDDED_REPO advice and include a test to notice any\npossible breakage.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Rubén Justo <rjusto@gmail.com>\n---\n builtin/add.c  |  6 +++---\n t/t3700-add.sh | 34 ++++++++++++++++++++++++++++++++++\n 2 files changed, 37 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 289adaaecf..e97699d6b9 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -310,9 +310,9 @@ static void check_embedded_repo(const char *path)\n \tstrbuf_strip_suffix(&name, \"/\");\n \n \twarning(_(\"adding embedded git repository: %s\"), name.buf);\n-\tif (!adviced_on_embedded_repo &&\n-\t    advice_enabled(ADVICE_ADD_EMBEDDED_REPO)) {\n-\t\tadvise(embedded_advice, name.buf, name.buf);\n+\tif (!adviced_on_embedded_repo) {\n+\t\tadvise_if_enabled(ADVICE_ADD_EMBEDDED_REPO,\n+\t\t\t\t  embedded_advice, name.buf, name.buf);\n \t\tadviced_on_embedded_repo = 1;\n \t}\n \ndiff --git a/t/t3700-add.sh b/t/t3700-add.sh\nindex 681081e0d5..839c904745 100755\n--- a/t/t3700-add.sh\n+++ b/t/t3700-add.sh\n@@ -349,6 +349,40 @@ test_expect_success '\"git add .\" in empty repo' '\n \t)\n '\n \n+test_expect_success '\"git add\" a embedded repository' '\n+\trm -fr outer && git init outer &&\n+\t(\n+\t\tcd outer &&\n+\t\tfor i in 1 2\n+\t\tdo\n+\t\t\tname=inner$i &&\n+\t\t\tgit init $name &&\n+\t\t\tgit -C $name commit --allow-empty -m $name ||\n+\t\t\t\treturn 1\n+\t\tdone &&\n+\t\tgit add . 2>actual &&\n+\t\tcat >expect <<-EOF &&\n+\t\twarning: adding embedded git repository: inner1\n+\t\thint: You${SQ}ve added another git repository inside your current repository.\n+\t\thint: Clones of the outer repository will not contain the contents of\n+\t\thint: the embedded repository and will not know how to obtain it.\n+\t\thint: If you meant to add a submodule, use:\n+\t\thint:\n+\t\thint: \tgit submodule add <url> inner1\n+\t\thint:\n+\t\thint: If you added this path by mistake, you can remove it from the\n+\t\thint: index with:\n+\t\thint:\n+\t\thint: \tgit rm --cached inner1\n+\t\thint:\n+\t\thint: See \"git help submodule\" for more information.\n+\t\thint: Disable this message with \"git config advice.addEmbeddedRepo false\"\n+\t\twarning: adding embedded git repository: inner2\n+\t\tEOF\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'error on a repository with no commits' '\n \trm -fr empty &&\n \tgit init empty &&\n-- \n2.44.0.417.g254ece0ee4\n"}]}