{"thread":{"id":"44812","subject":"[PATCHv4 0/2] pathspec: give better message for submodule related pathspec error","startedAt":"2017-01-04T01:48:52Z","lastAt":"2017-01-04T23:28:49Z","messageCount":10,"participants":["Stefan Beller","Jeff King","Brandon Williams"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"308757","messageId":"20170104014835.22377-1-sbeller@google.com","threadId":"44812","inReplyTo":null,"subject":"[PATCHv4 0/2] pathspec: give better message for submodule related pathspec error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-04T01:48:33Z","receivedAt":"2017-01-04T01:48:52Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> It MIGHT be a handy hack when writing a test, but let's stop doing\n> that insanity.  No sane project does that in real life, doesn't it?\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\nThis comes as an extra patch before the actual fix.\n\nThe actual fixing patch was reworded borrowing some words from Jeff.\n\nAs this makes use of \"test_commit -C\", it goes on top of sb/submodule-embed-gitdir\n\nThanks,\nStefan\n\n\nStefan Beller (2):\n  submodule tests: don't use itself as a submodule\n  pathspec: give better message for submodule related pathspec error\n\n pathspec.c                       | 24 ++++++++++++++++++++++--\n t/lib-submodule-update.sh        |  2 ++\n t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++++++++++++++\n t/t7001-mv.sh                    |  5 +++--\n t/t7507-commit-verbose.sh        |  4 +++-\n t/t7800-difftool.sh              |  4 +++-\n t/test-lib-functions.sh          | 16 ++++++++++++++++\n 7 files changed, 82 insertions(+), 6 deletions(-)\n create mode 100755 t/t6134-pathspec-in-submodule.sh\n\n-- \n2.11.0.rc2.31.g2cc886f\n\n"},{"id":"308758","messageId":"20170104014835.22377-2-sbeller@google.com","threadId":"44812","inReplyTo":"20170104014835.22377-1-sbeller@google.com","subject":"[PATCH 1/2] submodule tests: don't use itself as a submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-04T01:48:34Z","receivedAt":"2017-01-04T01:48:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In reality nobody would run \"git submodule add ./. <submodule-path>\"\nto add the repository to itself as a submodule as this comes with some\nnasty surprises, such as infinite recursion when cloning that repository.\nHowever we do this all the time in the test suite, because most of the\ntime this was the most convenient way to test a very specific thing\nfor submodule behavior.\n\nThis provides an easier way to have submodules in tests, by just setting\nTEST_CREATE_SUBMODULE to a non empty string, similar to\nTEST_NO_CREATE_REPO.\n\nMake use of it in those tests that add a submodule from ./. except for\nthe occurrence in create_lib_submodule_repo as there it seems we craft\na repository deliberately for both inside as well as outside use.\n\nThe name \"pretzel.[non]bare\" was chosen deliberate to not introduce\nmore strings to the test suite containing \"sub[module]\" as searching for\n\"sub\" already yields a lot of hits from different contexts. \"pretzel\"\ndoesn't occur in the test suite yet, so it is a good candidate for\na potential remote for a submodule.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/lib-submodule-update.sh |  2 ++\n t/t7001-mv.sh             |  5 +++--\n t/t7507-commit-verbose.sh |  4 +++-\n t/t7800-difftool.sh       |  4 +++-\n t/test-lib-functions.sh   | 16 ++++++++++++++++\n 5 files changed, 27 insertions(+), 4 deletions(-)\n\ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex 79cdd34a54..58d76d9df8 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -44,6 +44,8 @@ create_lib_submodule_repo () {\n \t\tgit branch \"no_submodule\" &&\n \n \t\tgit checkout -b \"add_sub1\" &&\n+\t\t# Adding the repo itself as a submodule is a hack.\n+\t\t# Do not imitate this.\n \t\tgit submodule add ./. sub1 &&\n \t\tgit config -f .gitmodules submodule.sub1.ignore all &&\n \t\tgit config submodule.sub1.ignore all &&\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex e365d1ff77..6cb32f3a3a 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -1,6 +1,7 @@\n #!/bin/sh\n \n test_description='git mv in subdirs'\n+TEST_CREATE_SUBMODULE=yes\n . ./test-lib.sh\n \n test_expect_success \\\n@@ -288,12 +289,12 @@ rm -f moved symlink\n test_expect_success 'setup submodule' '\n \tgit commit -m initial &&\n \tgit reset --hard &&\n-\tgit submodule add ./. sub &&\n+\tgit submodule add ./pretzel.bare sub &&\n \techo content >file &&\n \tgit add file &&\n \tgit commit -m \"added sub and file\" &&\n \tmkdir -p deep/directory/hierarchy &&\n-\tgit submodule add ./. deep/directory/hierarchy/sub &&\n+\tgit submodule add ./pretzel.bare deep/directory/hierarchy/sub &&\n \tgit commit -m \"added another submodule\" &&\n \tgit branch submodule\n '\ndiff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh\nindex ed2653d46f..d269900afa 100755\n--- a/t/t7507-commit-verbose.sh\n+++ b/t/t7507-commit-verbose.sh\n@@ -1,6 +1,7 @@\n #!/bin/sh\n \n test_description='verbose commit template'\n+TEST_CREATE_SUBMODULE=yes\n . ./test-lib.sh\n \n write_script \"check-for-diff\" <<\\EOF &&\n@@ -74,11 +75,12 @@ test_expect_success 'diff in message is retained with -v' '\n \n test_expect_success 'submodule log is stripped out too with -v' '\n \tgit config diff.submodule log &&\n-\tgit submodule add ./. sub &&\n+\tgit submodule add ./pretzel.bare sub &&\n \tgit commit -m \"sub added\" &&\n \t(\n \t\tcd sub &&\n \t\techo \"more\" >>file &&\n+\t\tgit add file &&\n \t\tgit commit -a -m \"submodule commit\"\n \t) &&\n \t(\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 70a2de461a..d13a5d0453 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -7,6 +7,7 @@ test_description='git-difftool\n \n Testing basic diff tool invocation\n '\n+TEST_CREATE_SUBMODULE=Yes\n \n . ./test-lib.sh\n \n@@ -534,7 +535,8 @@ test_expect_success PERL 'difftool --no-symlinks detects conflict ' '\n '\n \n test_expect_success PERL 'difftool properly honors gitlink and core.worktree' '\n-\tgit submodule add ./. submod/ule &&\n+\tgit submodule add ./pretzel.bare submod/ule &&\n+\ttest_commit -C submod/ule second_commit &&\n \ttest_config -C submod/ule diff.tool checktrees &&\n \ttest_config -C submod/ule difftool.checktrees.cmd '\\''\n \t\ttest -d \"$LOCAL\" && test -d \"$REMOTE\" && echo good\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 579e812506..aa327a7dff 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -800,6 +800,22 @@ test_create_repo () {\n \t\terror \"cannot run git init -- have you built things yet?\"\n \t\tmv .git/hooks .git/hooks-disabled\n \t) || exit\n+\tif test -n \"$TEST_CREATE_SUBMODULE\"\n+\tthen\n+\t\t(\n+\t\t\tcd \"$repo\"\n+\t\t\tTEST_CREATE_SUBMODULE=\n+\t\t\texport TEST_CREATE_SUBMODULE\n+\t\t\ttest_create_repo \"pretzel.nonbare\"\n+\t\t\ttest_commit -C \"pretzel.nonbare\" \\\n+\t\t\t\t\"create submodule\" \"submodule-file\" \\\n+\t\t\t\t\"submodule-content\" \"submodule-tag\" >&3 2>&4 ||\n+\t\t\t\terror \"cannot run test_commit\"\n+\t\t\tgit clone --bare \"pretzel.nonbare\" \\\n+\t\t\t\t  \"pretzel.bare\" >&3 2>&4 ||\n+\t\t\t\t  error \"cannot clone into bare\"\n+\t\t)\n+\tfi\n }\n \n # This function helps on symlink challenged file systems when it is not\n-- \n2.11.0.rc2.31.g2cc886f\n\n"},{"id":"308759","messageId":"20170104014835.22377-3-sbeller@google.com","threadId":"44812","inReplyTo":"20170104014835.22377-1-sbeller@google.com","subject":"[PATCH 2/2] pathspec: give better message for submodule related pathspec error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-04T01:48:35Z","receivedAt":"2017-01-04T01:49:20Z","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]. The usual response from the mailing\nlist is link to old discussions[2], and acknowledging the problem\nstating it is known.\n\nThis patch accomplishes two things:\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\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\nHelped-by: Jeff King <peff@peff.net>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n pathspec.c                       | 24 ++++++++++++++++++++++--\n t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 55 insertions(+), 2 deletions(-)\n create mode 100755 t/t6134-pathspec-in-submodule.sh\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 22ca74a126..574a0bb158 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -313,8 +313,28 @@ 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 len;\n+\n+\t\t\tif (!S_ISGITLINK(ce->ce_mode))\n+\t\t\t\tcontinue;\n+\n+\t\t\tlen = ce_namelen(ce);\n+\t\t\tif (item->len < len)\n+\t\t\t\tlen = item->len;\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_namelen(ce), 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..2900d8d06e\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_CREATE_SUBMODULE=yes\n+. ./test-lib.sh\n+\n+test_expect_success 'setup a submodule' '\n+\tgit submodule add ./pretzel.bare 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.rc2.31.g2cc886f\n\n"},{"id":"308763","messageId":"20170104075506.sa5oa5bheykswkwn@sigill.intra.peff.net","threadId":"44812","inReplyTo":"20170104014835.22377-3-sbeller@google.com","subject":"Re: [PATCH 2/2] pathspec: give better message for submodule related pathspec error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-04T07:55:06Z","receivedAt":"2017-01-04T07:55:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 03, 2017 at 05:48:35PM -0800, Stefan Beller wrote:\n\n> Every once in a while someone complains to the mailing list to have\n> run into this weird assertion[1]. The usual response from the mailing\n> list is link to old discussions[2], and acknowledging the problem\n> stating it is known.\n> \n> This patch accomplishes two things:\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\nAs this last bit is quoted from me, I won't deny that it's brilliant as\nusual.\n\nBut as this commit message needs to stand on its own, rather than as part of a\nlarger discussion thread, it might be worth expanding \"one of the cases\"\nhere. And talking about what's happening to the other cases.\n\nLike:\n\n  This assertion triggered for cases where there wasn't a programming\n  bug, but just bogus input. In particular, if the user asks for a\n  pathspec that is inside a submodule, we shouldn't assert() or\n  die(\"BUG\"); we should tell the user their request is bogus.\n\n  We'll retain the assertion for non-submodule cases, though. We don't\n  know of any cases that would trigger this, but it _would_ be\n  indicative of a programming error, and we should catch it here.\n\nor something. Writing the first paragraph made me wonder if a better\nsolution, though, would be to catch and complain about this case\nearlier. IOW, this _is_ a programming bug, because we're violating some\nassumption of the pathspec code. And whatever is putting that item into\nthe pathspec list is what should be fixed.\n\nI haven't looked closely enough to have a real opinion on that, though.\n\n> diff --git a/pathspec.c b/pathspec.c\n> index 22ca74a126..574a0bb158 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -313,8 +313,28 @@ 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 len;\n\nGiven the discussion, this comment seems funny now. Who cares about\n\"historically\"? It should probably be something like:\n\n  /*\n   * This case can be triggered by the user pointing us to a pathspec\n   * inside a submodule, which is an input error. Detect that here\n   * and complain, but fallback in the non-submodule case to a BUG,\n   * as we have no idea what would trigger that.\n   */\n\n-Peff\n"},{"id":"308797","messageId":"CAGZ79kY3oVfn-xH4RQR9jMoKxoQUtF5HezY9HPUZGbx9KP-S5Q@mail.gmail.com","threadId":"44812","inReplyTo":"20170104075506.sa5oa5bheykswkwn@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] pathspec: give better message for submodule related pathspec error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-04T18:46:27Z","receivedAt":"2017-01-04T18:48:08Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jan 3, 2017 at 11:55 PM, Jeff King <peff@peff.net> wrote:\n> But as this commit message needs to stand on its own, rather than as part of a\n> larger discussion thread, it might be worth expanding \"one of the cases\"\n> here. And talking about what's happening to the other cases.\n>\n> Like:\n>\n>   This assertion triggered for cases where there wasn't a programming\n>   bug, but just bogus input. In particular, if the user asks for a\n>   pathspec that is inside a submodule, we shouldn't assert() or\n>   die(\"BUG\"); we should tell the user their request is bogus.\n>\n>   We'll retain the assertion for non-submodule cases, though. We don't\n>   know of any cases that would trigger this, but it _would_ be\n>   indicative of a programming error, and we should catch it here.\n\nmakes sense.\n\n>\n> or something. Writing the first paragraph made me wonder if a better\n> solution, though, would be to catch and complain about this case\n> earlier. IOW, this _is_ a programming bug, because we're violating some\n> assumption of the pathspec code. And whatever is putting that item into\n> the pathspec list is what should be fixed.\n>\n> I haven't looked closely enough to have a real opinion on that, though.\n\nWell I think you get different behavior with different flags enabled, i.e.\nthe test provided is a cornercase (as \"git add .\" in the submodule should\nnot yell at us IF PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE\nwere set, in my understanding of the code, so maybe the test rather adds\na ./file/with/characters inside the submodule directory)\n\nI think a valid long term vision would be to have\n\n    $ git -C submodule add file\n    $ echo $?\n    0\n\nto behave the same as\n\n    $ git add submodule/file\n    advice/hint: adding file inside of a submodule\n    $ echo $?\n    0\n    $ git -c submodule.iKnowWhatIDo add submodule/anotherfile\n    $ echo $?\n    0\n\nBrandon, who is refactoring the pathspec stuff currently may have\nan opinion if we could catch it earlier and still have beautiful code.\n\nThanks,\nStefan\n\n> Given the discussion, this comment seems funny now. Who cares about\n> \"historically\"? It should probably be something like:\n>\n>   /*\n>    * This case can be triggered by the user pointing us to a pathspec\n>    * inside a submodule, which is an input error. Detect that here\n>    * and complain, but fallback in the non-submodule case to a BUG,\n>    * as we have no idea what would trigger that.\n>    */\n\nMakes sense.\n\n>\n> -Peff\n"},{"id":"308799","messageId":"20170104205346.GE69227@google.com","threadId":"44812","inReplyTo":"CAGZ79kY3oVfn-xH4RQR9jMoKxoQUtF5HezY9HPUZGbx9KP-S5Q@mail.gmail.com","subject":"Re: [PATCH 2/2] pathspec: give better message for submodule related pathspec error","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-01-04T20:53:46Z","receivedAt":"2017-01-04T21:11:20Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 01/04, Stefan Beller wrote:\n> On Tue, Jan 3, 2017 at 11:55 PM, Jeff King <peff@peff.net> wrote:\n> > But as this commit message needs to stand on its own, rather than as part of a\n> > larger discussion thread, it might be worth expanding \"one of the cases\"\n> > here. And talking about what's happening to the other cases.\n> >\n> > Like:\n> >\n> >   This assertion triggered for cases where there wasn't a programming\n> >   bug, but just bogus input. In particular, if the user asks for a\n> >   pathspec that is inside a submodule, we shouldn't assert() or\n> >   die(\"BUG\"); we should tell the user their request is bogus.\n> >\n> >   We'll retain the assertion for non-submodule cases, though. We don't\n> >   know of any cases that would trigger this, but it _would_ be\n> >   indicative of a programming error, and we should catch it here.\n> \n> makes sense.\n> \n> >\n> > or something. Writing the first paragraph made me wonder if a better\n> > solution, though, would be to catch and complain about this case\n> > earlier. IOW, this _is_ a programming bug, because we're violating some\n> > assumption of the pathspec code. And whatever is putting that item into\n> > the pathspec list is what should be fixed.\n> >\n> > I haven't looked closely enough to have a real opinion on that, though.\n> \n> Well I think you get different behavior with different flags enabled, i.e.\n> the test provided is a cornercase (as \"git add .\" in the submodule should\n> not yell at us IF PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE\n> were set, in my understanding of the code, so maybe the test rather adds\n> a ./file/with/characters inside the submodule directory)\n> \n> I think a valid long term vision would be to have\n> \n>     $ git -C submodule add file\n>     $ echo $?\n>     0\n> \n> to behave the same as\n> \n>     $ git add submodule/file\n>     advice/hint: adding file inside of a submodule\n>     $ echo $?\n>     0\n>     $ git -c submodule.iKnowWhatIDo add submodule/anotherfile\n>     $ echo $?\n>     0\n> \n> Brandon, who is refactoring the pathspec stuff currently may have\n> an opinion if we could catch it earlier and still have beautiful code.\n> \n> Thanks,\n> Stefan\n> \n> > Given the discussion, this comment seems funny now. Who cares about\n> > \"historically\"? It should probably be something like:\n> >\n> >   /*\n> >    * This case can be triggered by the user pointing us to a pathspec\n> >    * inside a submodule, which is an input error. Detect that here\n> >    * and complain, but fallback in the non-submodule case to a BUG,\n> >    * as we have no idea what would trigger that.\n> >    */\n> \n> Makes sense.\n> \n> >\n> > -Peff\n\nSo there are really two different things going on in the pathspec code\nwith regards to submodules.\n\nThe case that this series is trying to solve is not because the user\nprovided a pathspec into a submodule, but rather they are executing in\nthe context of the submodule with bogus state.  Typically this bogus\nstate has something to do with the submodule's .gitdir being blown away\n(like in the last test (3/3) added in this patch).  Because the\nsubmodule doesn't have a .gitdir, it searches backward in the directory\nhierarchy for a .gitdir and it happens to find the superproject's gitdir\nand uses that as its own .gitdir.  When this happens test 3/3 catches\nthat assert with the prefix being \"sub/\" and match being \"sub\" (since\nthe submodule slash was removed).  The condition doesn't trigger when\nyou supply a pathspec of \"./b/a\" assuming you have a file 'a' in\ndirectory 'b' inside the submodule, since the prefix would still be\n\"sub/\" while the match string would be \"sub/b/a\".  Coincidentally the\ncheck that PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE does, does in fact\ncatch it (if using say the 'git add' command).\n\nThis leads me into the second case.  If\nPATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE is set, then any pathspec which\ndecends into a submodule will indeed be caught and cause and error (as\nwas happens in test 2/3 in this patch).\n\nSo in my opinion, the assert at the end of constructing a\npathspec object probably isn't the best place for determining if the\nsubmodule's gitdir has been destroyed and instead it has fallen back to\nits parent's gitdir.  A check for something like this should happen much\nsooner.\n\nThere are cases where it is advantages to be able to supply a pathspec\ninto a submodule without it erroring out (git grep --recurse-submodules\nis one example).  So right now the current method for not allowing a\npathspec into a submodule is to pass the STRIP_SUBMODULE_SLASH_EXPENSIVE\nflag when creating the pathspec object.\n\n-- \nBrandon Williams\n"},{"id":"308808","messageId":"20170104231027.7065-1-sbeller@google.com","threadId":"44812","inReplyTo":"20170104205346.GE69227@google.com","subject":"[PATCHv5] pathspec: give better message for submodule related pathspec error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-04T23:10:27Z","receivedAt":"2017-01-04T23:10:46Z","isPatch":false,"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]. The usual response from the mailing\nlist is link to old discussions[2], and acknowledging the problem\nstating it is known.\n\nThis patch accomplishes two things:\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\n     This assertion triggered for cases where there wasn't a programming\n     bug, but just bogus input. In particular, if the user asks for a\n     pathspec that is inside a submodule, we shouldn't assert() or\n     die(\"BUG\"); we should tell the user their request is bogus.\n\n     The only reason we did not check for it, is the expensive nature\n     of such a check, so callers avoid setting the flag\n     PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE. However when we die due\n     to bogus input, the expense of cpu cycles spent outweighs the user\n     wondering what went wrong, so run that check unconditionally before\n     dying with a more generic error message.\n\n     In case we found out that the path points inside a submodule, but the\n     caller did not ask for PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE, we\n     should not silently fix the pathspec to just point at the submodule,\n     as that would confuse callers.\n\nTo make this happen, specifically the second part, move the check for\nbeing inside a submodule into a function and call it either when\nPATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE is set or when we are in the\nbuggy case to give a better error message.\n\nNote: There is this one special case (\"git -C submodule add .\") in which\nwe call check_inside_submodule_expensive two times, once for checking\nPATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE and once in the code path\nhandling the buggy user input. For this to work correctly we need to adapt\nthe conditions in the check for being inside the submodule to account for\nthe prior run to have taken effect.\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\nHelped-by: Jeff King <peff@peff.net>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nThis is just rerolling the second patch of that \"make the assert go away\",\nasking for opinions again.\n\nI agree with Brandon that pathspec code is not the ideal place to\ncheck for issues with submodules. However we should give the best error\nmessage possible for the user, so running this diagnosis is fine by me.\n\nThanks,\nStefan\n\n pathspec.c                       | 63 +++++++++++++++++++++++++++-------------\n t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++\n 2 files changed, 76 insertions(+), 20 deletions(-)\n create mode 100755 t/t6134-pathspec-in-submodule.sh\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 22ca74a126..41e0dac1df 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -88,6 +88,34 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n+static void check_inside_submodule_expensive(struct pathspec_item *item,\n+\t\t\t\t\t     char *match,\n+\t\t\t\t\t     const char *elt, int die_inside)\n+{\n+\tint i;\n+\tfor (i = 0; i < active_nr; i++) {\n+\t\tstruct cache_entry *ce = active_cache[i];\n+\t\tint ce_len = ce_namelen(ce);\n+\n+\t\tif (!S_ISGITLINK(ce->ce_mode))\n+\t\t\tcontinue;\n+\n+\t\tif (item->len < ce_len ||\n+\t\t    !(match[ce_len] == '/' || match[ce_len] == '\\0') ||\n+\t\t    memcmp(ce->name, match, ce_len))\n+\t\t\tcontinue;\n+\n+\t\tif (item->len != ce_len + 1 || die_inside)\n+\t\t\tdie (_(\"Pathspec '%s' is in submodule '%.*s'\"),\n+\t\t\t     elt, ce_len, ce->name);\n+\n+\t\t/* strip trailing slash */\n+\t\titem->len--;\n+\t\tmatch[item->len] = '\\0';\n+\t\tbreak;\n+\t}\n+}\n+\n /*\n  * Take an element of a pathspec and check for magic signatures.\n  * Append the result to the prefix. Return the magic bitmap.\n@@ -273,24 +301,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n \t}\n \n \tif (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)\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-\n-\t\t\tif (!S_ISGITLINK(ce->ce_mode))\n-\t\t\t\tcontinue;\n-\n-\t\t\tif (item->len <= ce_len || match[ce_len] != '/' ||\n-\t\t\t    memcmp(ce->name, match, ce_len))\n-\t\t\t\tcontinue;\n-\t\t\tif (item->len == ce_len + 1) {\n-\t\t\t\t/* strip trailing slash */\n-\t\t\t\titem->len--;\n-\t\t\t\tmatch[item->len] = '\\0';\n-\t\t\t} else\n-\t\t\t\tdie (_(\"Pathspec '%s' is in submodule '%.*s'\"),\n-\t\t\t\t     elt, ce_len, ce->name);\n-\t\t}\n+\t\tcheck_inside_submodule_expensive(item, match, elt, 0);\n \n \tif (magic & PATHSPEC_LITERAL)\n \t\titem->nowildcard_len = item->len;\n@@ -313,8 +324,20 @@ 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/*\n+\t\t * We know something is fishy and we're going to die\n+\t\t * anyway, so we don't care about following operation\n+\t\t * to be expensive, despite the caller not asking for\n+\t\t * an expensive submodule check. The potential expensive\n+\t\t * operation here reduces the users head scratching\n+\t\t * greatly, though.\n+\t\t */\n+\t\tcheck_inside_submodule_expensive(item, match, elt, 1);\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..2900d8d06e\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_CREATE_SUBMODULE=yes\n+. ./test-lib.sh\n+\n+test_expect_success 'setup a submodule' '\n+\tgit submodule add ./pretzel.bare 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.rc2.32.gdde9519.dirty\n\n"},{"id":"308809","messageId":"CAGZ79kagAX-ZjO_k7LxQNHp=Le9C=WzYQxxe-bRFneHjDwnqSw@mail.gmail.com","threadId":"44812","inReplyTo":"20170104075506.sa5oa5bheykswkwn@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] pathspec: give better message for submodule related pathspec error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-04T23:10:14Z","receivedAt":"2017-01-04T23:10:52Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Jan 3, 2017 at 11:55 PM, Jeff King <peff@peff.net> wrote:\n\n> As this last bit is quoted from me, I won't deny that it's brilliant as\n> usual.\n\nobviously. :)\n\n>\n> But as this commit message needs to stand on its own, rather than as part of a\n> larger discussion thread, it might be worth expanding \"one of the cases\"\n> here. And talking about what's happening to the other cases.\n>\n> Like:\n>\n>   This assertion triggered for cases where there wasn't a programming\n>   bug, but just bogus input. In particular, if the user asks for a\n>   pathspec that is inside a submodule, we shouldn't assert() or\n>   die(\"BUG\"); we should tell the user their request is bogus.\n\nalt. cont'd:\n\nWe already would do that if PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE\nis set, but we had to ask for this examination via a flag, because\nit is expensive. At this point in code we know there is bogus input,\nso all we would do is error out. For that case we can assume that the cost\nof the expensive search is negligible compared to the users head scratching\nthat follows.\n\n(This will appear in the patch I am about to send out)\n"},{"id":"308810","messageId":"20170104231605.GB68241@google.com","threadId":"44812","inReplyTo":"20170104231027.7065-1-sbeller@google.com","subject":"Re: [PATCHv5] pathspec: give better message for submodule related pathspec error","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-01-04T23:16:05Z","receivedAt":"2017-01-04T23:16:14Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 01/04, Stefan Beller wrote:\n> Every once in a while someone complains to the mailing list to have\n> run into this weird assertion[1]. The usual response from the mailing\n> list is link to old discussions[2], and acknowledging the problem\n> stating it is known.\n> \n> This patch accomplishes two things:\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> \n>      This assertion triggered for cases where there wasn't a programming\n>      bug, but just bogus input. In particular, if the user asks for a\n>      pathspec that is inside a submodule, we shouldn't assert() or\n>      die(\"BUG\"); we should tell the user their request is bogus.\n> \n>      The only reason we did not check for it, is the expensive nature\n>      of such a check, so callers avoid setting the flag\n>      PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE. However when we die due\n>      to bogus input, the expense of cpu cycles spent outweighs the user\n>      wondering what went wrong, so run that check unconditionally before\n>      dying with a more generic error message.\n> \n>      In case we found out that the path points inside a submodule, but the\n>      caller did not ask for PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE, we\n>      should not silently fix the pathspec to just point at the submodule,\n>      as that would confuse callers.\n> \n> To make this happen, specifically the second part, move the check for\n> being inside a submodule into a function and call it either when\n> PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE is set or when we are in the\n> buggy case to give a better error message.\n> \n> Note: There is this one special case (\"git -C submodule add .\") in which\n> we call check_inside_submodule_expensive two times, once for checking\n> PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE and once in the code path\n> handling the buggy user input. For this to work correctly we need to adapt\n> the conditions in the check for being inside the submodule to account for\n> the prior run to have taken effect.\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> \n> Helped-by: Jeff King <peff@peff.net>\n> Helped-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> \n> This is just rerolling the second patch of that \"make the assert go away\",\n> asking for opinions again.\n> \n> I agree with Brandon that pathspec code is not the ideal place to\n> check for issues with submodules. However we should give the best error\n> message possible for the user, so running this diagnosis is fine by me.\n> \n> Thanks,\n> Stefan\n> \n>  pathspec.c                       | 63 +++++++++++++++++++++++++++-------------\n>  t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++\n>  2 files changed, 76 insertions(+), 20 deletions(-)\n>  create mode 100755 t/t6134-pathspec-in-submodule.sh\n> \n> diff --git a/pathspec.c b/pathspec.c\n> index 22ca74a126..41e0dac1df 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -88,6 +88,34 @@ static void prefix_short_magic(struct strbuf *sb, int prefixlen,\n>  \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n>  }\n>  \n> +static void check_inside_submodule_expensive(struct pathspec_item *item,\n> +\t\t\t\t\t     char *match,\n> +\t\t\t\t\t     const char *elt, int die_inside)\n> +{\n> +\tint i;\n> +\tfor (i = 0; i < active_nr; i++) {\n> +\t\tstruct cache_entry *ce = active_cache[i];\n> +\t\tint ce_len = ce_namelen(ce);\n> +\n> +\t\tif (!S_ISGITLINK(ce->ce_mode))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (item->len < ce_len ||\n> +\t\t    !(match[ce_len] == '/' || match[ce_len] == '\\0') ||\n> +\t\t    memcmp(ce->name, match, ce_len))\n> +\t\t\tcontinue;\n> +\n> +\t\tif (item->len != ce_len + 1 || die_inside)\n> +\t\t\tdie (_(\"Pathspec '%s' is in submodule '%.*s'\"),\n> +\t\t\t     elt, ce_len, ce->name);\n> +\n> +\t\t/* strip trailing slash */\n> +\t\titem->len--;\n> +\t\tmatch[item->len] = '\\0';\n> +\t\tbreak;\n> +\t}\n> +}\n> +\n>  /*\n>   * Take an element of a pathspec and check for magic signatures.\n>   * Append the result to the prefix. Return the magic bitmap.\n> @@ -273,24 +301,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,\n>  \t}\n>  \n>  \tif (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)\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> -\n> -\t\t\tif (!S_ISGITLINK(ce->ce_mode))\n> -\t\t\t\tcontinue;\n> -\n> -\t\t\tif (item->len <= ce_len || match[ce_len] != '/' ||\n> -\t\t\t    memcmp(ce->name, match, ce_len))\n> -\t\t\t\tcontinue;\n> -\t\t\tif (item->len == ce_len + 1) {\n> -\t\t\t\t/* strip trailing slash */\n> -\t\t\t\titem->len--;\n> -\t\t\t\tmatch[item->len] = '\\0';\n> -\t\t\t} else\n> -\t\t\t\tdie (_(\"Pathspec '%s' is in submodule '%.*s'\"),\n> -\t\t\t\t     elt, ce_len, ce->name);\n> -\t\t}\n> +\t\tcheck_inside_submodule_expensive(item, match, elt, 0);\n>  \n>  \tif (magic & PATHSPEC_LITERAL)\n>  \t\titem->nowildcard_len = item->len;\n> @@ -313,8 +324,20 @@ 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/*\n> +\t\t * We know something is fishy and we're going to die\n> +\t\t * anyway, so we don't care about following operation\n> +\t\t * to be expensive, despite the caller not asking for\n> +\t\t * an expensive submodule check. The potential expensive\n> +\t\t * operation here reduces the users head scratching\n> +\t\t * greatly, though.\n> +\t\t */\n> +\t\tcheck_inside_submodule_expensive(item, match, elt, 1);\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..2900d8d06e\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_CREATE_SUBMODULE=yes\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup a submodule' '\n> +\tgit submodule add ./pretzel.bare 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\nI haven't taken a through look at this patch but I think you may want to\nbase it off of 'origin/bw/pathspec-cleanup' series as the changes made in this\npatch now conflict with that series.\n\nAlso I still don't really think this solves the problem of telling the\nuser what is wrong, which is that the submodule's gitdir is gone.\n\n-- \nBrandon Williams\n"},{"id":"308811","messageId":"CAGZ79kbo3uKD5Un_P+EPdO2mMt6WUVjWPqfYEbitzU0zw0ex3A@mail.gmail.com","threadId":"44812","inReplyTo":"20170104231605.GB68241@google.com","subject":"Re: [PATCHv5] pathspec: give better message for submodule related pathspec error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-04T23:28:42Z","receivedAt":"2017-01-04T23:28:49Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> I haven't taken a through look at this patch but I think you may want to\n> base it off of 'origin/bw/pathspec-cleanup' series as the changes made in this\n> patch now conflict with that series.\n\neh right, I forgot to mention this in the notes, it requires\nsb/submodule-embed-gitdir as well, so I'll have to figure that out.\n\n>\n> Also I still don't really think this solves the problem of telling the\n> user what is wrong, which is that the submodule's gitdir is gone.\n>\n\nThe \"git dir gone\" is not a big deal IMHO as a deinitialized submodule\nis perfectly fine (e.g. not initialized). The errors as I tested in Gerrit,\na superproject that contains submodules in plugins/* :\n\n    : gerrit/plugins/cookbook-plugin$ git add .\n    fatal: Pathspec '.' is in submodule 'plugins/cookbook-plugin'\n    : gerrit/plugins/cookbook-plugin$ cd ..\n    : gerrit/plugins$ git add cookbook-plugin/a\n    fatal: Pathspec 'cookbook-plugin/a' is in submodule\n'plugins/cookbook-plugin'\n    : gerrit/plugins$ git add cookbook-plugin/.\n    : gerrit/plugins$ git add cookbook-plugin/./.\n    : gerrit/plugins$\n\nI think that is perfect behavior for now, as it reliably detects\n(a) the submodule being there and (b) if you are in there, no\nmatter if there is a .git dir or not.\n\nThe same error coming up if the submodule is initialized and valid, e.g.\n\n    : gerrit/plugins$ git submodule update --init cookbook-plugin\n    : gerrit/plugins$ git add cookbook-plugin/a/.\n    fatal: Pathspec 'cookbook-plugin/a/.' is in submodule\n'plugins/cookbook-plugin'\n\nSo I think this is pretty much exactly what we want for now:\n* if PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE is set\n   we keep the behavior as is and do the expensive thing\n* if the caller wants to use path inside of a submodule no matter\n  the git dir of the submodule, then set the CHEAP flag instead\n* in case of the assert (that I originally wanted to fix), we fall back to the\n  EXPENSIVE thing reporting the error message that we already reported\n  in such cases.\n\nTL;DR: I was rather asking about the code being a viable;\nby now I am convinced this is the correct behavior. ;)\n"}]}