{"thread":{"id":"44790","subject":"[PATCH] pathspec: give better message for submodule related pathspec error","startedAt":"2016-12-29T19:29:18Z","lastAt":"2017-01-04T01:24:03Z","messageCount":4,"participants":["Stefan Beller","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"308497","messageId":"20161229192908.32633-1-sbeller@google.com","threadId":"44790","inReplyTo":null,"subject":"[PATCH] pathspec: give better message for submodule related pathspec error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-29T19:29:08Z","receivedAt":"2016-12-29T19:29:18Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Every once in a while someone complains to the mailing list to have\nrun into this weird assertion[1].\n\nThe usual response from the mailing list is link to old discussions[2],\nand acknowledging the problem stating it is known.\n\nFor now just improve the user visible error message.\n\n[1] https://www.google.com/search?q=item-%3Enowildcard_len\n[2] http://git.661346.n2.nabble.com/assert-failed-in-submodule-edge-case-td7628687.html\n    https://www.spinics.net/lists/git/msg249473.html\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n * a more defensive check and message\n * with tests!\n \n pathspec.c                       | 19 +++++++++++++++++--\n t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 50 insertions(+), 2 deletions(-)\n create mode 100755 t/t6134-pathspec-in-submodule.sh\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 22ca74a126..b446d79615 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -313,8 +313,23 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n \t}\n \n \t/* sanity checks, pathspec matchers assume these are sane */\n-\tassert(item->nowildcard_len <= item->len &&\n-\t       item->prefix         <= item->len);\n+\tif (item->nowildcard_len > item->len ||\n+\t    item->prefix         > item->len) {\n+\t\t/* Historically this always was a submodule issue */\n+\t\tfor (i = 0; i < active_nr; i++) {\n+\t\t\tstruct cache_entry *ce = active_cache[i];\n+\t\t\tint ce_len = ce_namelen(ce);\n+\t\t\tint len = ce_len < item->len ? ce_len : item->len;\n+\t\t\tif (!S_ISGITLINK(ce->ce_mode))\n+\t\t\t\tcontinue;\n+\t\t\tif (!memcmp(ce->name, item->match, len))\n+\t\t\t\tdie (_(\"Pathspec '%s' is in submodule '%.*s'\"),\n+\t\t\t\t\titem->original, ce_len, ce->name);\n+\t\t}\n+\t\t/* The error is a new unknown bug */\n+\t\tdie (\"BUG: item->nowildcard_len > item->len || item->prefix > item->len)\");\n+\t}\n+\n \treturn magic;\n }\n \ndiff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh\nnew file mode 100755\nindex 0000000000..e62dbb7327\n--- /dev/null\n+++ b/t/t6134-pathspec-in-submodule.sh\n@@ -0,0 +1,33 @@\n+#!/bin/sh\n+\n+test_description='test case exclude pathspec'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup a submodule' '\n+\ttest_commit 1 &&\n+\tgit submodule add ./ sub &&\n+\tgit commit -a -m \"add submodule\" &&\n+\tgit submodule deinit --all\n+'\n+\n+cat <<EOF >expect\n+fatal: Pathspec 'sub/a' is in submodule 'sub'\n+EOF\n+\n+test_expect_success 'error message for path inside submodule' '\n+\techo a >sub/a &&\n+\ttest_must_fail git add sub/a 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat <<EOF >expect\n+fatal: Pathspec '.' is in submodule 'sub'\n+EOF\n+\n+test_expect_success 'error message for path inside submodule from within submodule' '\n+\ttest_must_fail git -C sub add . 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.11.0.259.ga95e92af08.dirty\n\n"},{"id":"308558","messageId":"xmqqh95j60uh.fsf@gitster.mtv.corp.google.com","threadId":"44790","inReplyTo":"20161229192908.32633-1-sbeller@google.com","subject":"Re: [PATCH] pathspec: give better message for submodule related pathspec error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-01T01:11:18Z","receivedAt":"2017-01-01T01:11:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> Every once in a while someone complains to the mailing list to have\n> run into this weird assertion[1].\n>\n> The usual response from the mailing list is link to old discussions[2],\n> and acknowledging the problem stating it is known.\n>\n> For now just improve the user visible error message.\n\nThans. judging from the date: header I take this is meant as v3 that\nsupersedes v2 done on Wednesday.\n\nIt is not clear in the above that what this thing is.  Given that we\nare de-asserting it, is the early part of the new code diagnosing an\nend-user error (i.e. you gave me a pathspec but that extends into a\nsubmodule which is a no-no)?  The \"was a submodule issue\" comment\nadded is \"this is an end-user mistake, there is nothing to fix in\nthe code\"?\n\nI take that the new \"BUG\" thing tells the Git developers that no\nsane codepath should throw an pathspec_item that satisfies the\ncondition of the if() statement for non-submodules?\n\n> diff --git a/pathspec.c b/pathspec.c\n> index 22ca74a126..b446d79615 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -313,8 +313,23 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n>  \t}\n>  \n>  \t/* sanity checks, pathspec matchers assume these are sane */\n> -\tassert(item->nowildcard_len <= item->len &&\n> -\t       item->prefix         <= item->len);\n> +\tif (item->nowildcard_len > item->len ||\n> +\t    item->prefix         > item->len) {\n> +\t\t/* Historically this always was a submodule issue */\n> +\t\tfor (i = 0; i < active_nr; i++) {\n> +\t\t\tstruct cache_entry *ce = active_cache[i];\n> +\t\t\tint ce_len = ce_namelen(ce);\n> +\t\t\tint len = ce_len < item->len ? ce_len : item->len;\n> +\t\t\tif (!S_ISGITLINK(ce->ce_mode))\n> +\t\t\t\tcontinue;\n\nComputation of ce_len and len are better done after this check, no?\n\n> +\t\t\tif (!memcmp(ce->name, item->match, len))\n> +\t\t\t\tdie (_(\"Pathspec '%s' is in submodule '%.*s'\"),\n> +\t\t\t\t\titem->original, ce_len, ce->name);\n> +\t\t}\n> +\t\t/* The error is a new unknown bug */\n> +\t\tdie (\"BUG: item->nowildcard_len > item->len || item->prefix > item->len)\");\n> +\t}\n> +\n>  \treturn magic;\n>  }\n>  \n> diff --git a/t/t6134-pathspec-in-submodule.sh b/t/t6134-pathspec-in-submodule.sh\n> new file mode 100755\n> index 0000000000..e62dbb7327\n> --- /dev/null\n> +++ b/t/t6134-pathspec-in-submodule.sh\n> @@ -0,0 +1,33 @@\n> +#!/bin/sh\n> +\n> +test_description='test case exclude pathspec'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup a submodule' '\n> +\ttest_commit 1 &&\n> +\tgit submodule add ./ sub &&\n\nIs this adding our own project as its submodule?  \n\nIt MIGHT be a handy hack when writing a test, but let's stop doing\nthat insanity.  No sane project does that in real life, doesn't it?\n\nCreate a subdirectory, make it a repository, have a commit there and\nbind that as our own submodule.  That would be a more normal way to\nstart your own superproject and its submodule pair if they originate\ntogether at the same place.\n\nBetter yet create a separate repository, have a commit there, and\nthen pull it in with \"git submodule add && git submodule init\" into\nour repository.  That would be the normal way to borrow somebody\nelse's project as a part of your own superproject.\n\n> +\tgit commit -a -m \"add submodule\" &&\n> +\tgit submodule deinit --all\n> +'\n> +\n> +cat <<EOF >expect\n> +fatal: Pathspec 'sub/a' is in submodule 'sub'\n> +EOF\n> +\n> +test_expect_success 'error message for path inside submodule' '\n> +\techo a >sub/a &&\n> +\ttest_must_fail git add sub/a 2>actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +cat <<EOF >expect\n> +fatal: Pathspec '.' is in submodule 'sub'\n> +EOF\n> +\n> +test_expect_success 'error message for path inside submodule from within submodule' '\n> +\ttest_must_fail git -C sub add . 2>actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_done\n"},{"id":"308710","messageId":"CAGZ79kZ5F46UzeNeVYQV1EKEEa6+az-Pe_jrT+wZF9X6b34CGA@mail.gmail.com","threadId":"44790","inReplyTo":"xmqqh95j60uh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] pathspec: give better message for submodule related pathspec error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-03T18:15:38Z","receivedAt":"2017-01-03T18:15:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Dec 31, 2016 at 5:11 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> Every once in a while someone complains to the mailing list to have\n>> run into this weird assertion[1].\n>>\n>> The usual response from the mailing list is link to old discussions[2],\n>> and acknowledging the problem stating it is known.\n>>\n>> For now just improve the user visible error message.\n>\n> Thans. judging from the date: header I take this is meant as v3 that\n> supersedes v2 done on Wednesday.\n\nYes, that is correct. Sorry for being sloppy not numbering the\npatches correctly.\n\n>\n> It is not clear in the above that what this thing is.  Given that we\n> are de-asserting it, is the early part of the new code diagnosing an\n> end-user error (i.e. you gave me a pathspec but that extends into a\n> submodule which is a no-no)?  The \"was a submodule issue\" comment\n> added is \"this is an end-user mistake, there is nothing to fix in\n> the code\"?\n\nThis is not a fix in the code, but purely improving an error message.\nSo far anytime someone run into this assert, it was related to submodules.\nI do not know the pathspec code well enough to claim this condition\ncan be produced via submodules *only*, though.\n\nSo I proposed a more defensive patch, which diagnoses if it is the\n\"no-no, pathspec extends into a submodule\" first and then throws\na generic error afterwards in case it is not the submodule issue.\n\n> I take that the new \"BUG\" thing tells the Git developers that no\n> sane codepath should throw an pathspec_item that satisfies the\n> condition of the if() statement for non-submodules?\n\nIf we want to keep the semantics of the assert around, then we\nhave to have a blank statement if the submodule error message\nis not triggered.\n\nI assume if we print this BUG, then there is an actual bug.\n\n>\n>> diff --git a/pathspec.c b/pathspec.c\n>> index 22ca74a126..b446d79615 100644\n>> --- a/pathspec.c\n>> +++ b/pathspec.c\n>> @@ -313,8 +313,23 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n>>       }\n>>\n>>       /* sanity checks, pathspec matchers assume these are sane */\n>> -     assert(item->nowildcard_len <= item->len &&\n>> -            item->prefix         <= item->len);\n>> +     if (item->nowildcard_len > item->len ||\n>> +         item->prefix         > item->len) {\n>> +             /* Historically this always was a submodule issue */\n>> +             for (i = 0; i < active_nr; i++) {\n>> +                     struct cache_entry *ce = active_cache[i];\n>> +                     int ce_len = ce_namelen(ce);\n>> +                     int len = ce_len < item->len ? ce_len : item->len;\n>> +                     if (!S_ISGITLINK(ce->ce_mode))\n>> +                             continue;\n>\n> Computation of ce_len and len are better done after this check, no?\n\nYes, though I trusted the modern-day-compilers to get it right. Will\nfix in a reroll.\n\n>> +test_expect_success 'setup a submodule' '\n>> +     test_commit 1 &&\n>> +     git submodule add ./ sub &&\n>\n> Is this adding our own project as its submodule?\n\nYes it is.\n\n>\n> It MIGHT be a handy hack when writing a test, but let's stop doing\n> that insanity.\n\nI agree that this is not a good idea.\n\n>  No sane project does that in real life, doesn't it?\n\nIf such a project was cloned with submodules, it would recurse endlessly. :)\n\n> Create a subdirectory, make it a repository, have a commit there and\n> bind that as our own submodule.  That would be a more normal way to\n> start your own superproject and its submodule pair if they originate\n> together at the same place.\n\nI wonder if we want to have a helper function in test-lib.sh to be used\nfor that. This use case (have a repository and a submodule) happens in\na lot of tests, so we could make life easier by providing a function\nin the library so it is even easier than this HACK.\n\n> Better yet create a separate repository, have a commit there, and\n> then pull it in with \"git submodule add && git submodule init\" into\n> our repository.  That would be the normal way to borrow somebody\n> else's project as a part of your own superproject.\n\nThe library function could do that, yes.\n\nThanks,\nStefan\n"},{"id":"308756","messageId":"20170104012318.262o5743ne2u7rwc@sigill.intra.peff.net","threadId":"44790","inReplyTo":"CAGZ79kZ5F46UzeNeVYQV1EKEEa6+az-Pe_jrT+wZF9X6b34CGA@mail.gmail.com","subject":"Re: [PATCH] pathspec: give better message for submodule related pathspec error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-04T01:23:18Z","receivedAt":"2017-01-04T01:24:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 03, 2017 at 10:15:38AM -0800, Stefan Beller wrote:\n\n> > I take that the new \"BUG\" thing tells the Git developers that no\n> > sane codepath should throw an pathspec_item that satisfies the\n> > condition of the if() statement for non-submodules?\n> \n> If we want to keep the semantics of the assert around, then we\n> have to have a blank statement if the submodule error message\n> is not triggered.\n> \n> I assume if we print this BUG, then there is an actual bug.\n\nRight. I think this isn't a new \"BUG\", but rather a loosening of an\nexisting one. IOW, two things are going on here:\n\n  1. Switch assert() to die(\"BUG\") to give a more readable message.\n\n  2. Take one of the cases where we hit a BUG and turn it into a normal\n     \"there was something wrong with the input\" message.\n\nIf I understand the submodule case correctly, then (2) is reasonable.\nThe user gave a bogus pathspec that crosses the submodule boundary.\n\nI've no idea if there are other cases that could ever hit the remaining\nBUG, but it seems like a good defensive measure to leave it in. If\nsomebody hits it, then we can investigate their case.\n\n-Peff\n"}]}