{"thread":{"id":"44819","subject":"[PATCHv6 2/2] pathspec: give better message for submodule related pathspec error","startedAt":"2017-01-05T19:33:08Z","lastAt":"2017-01-09T20:10:55Z","messageCount":7,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"308829","messageId":"20170105192904.1107-3-sbeller@google.com","threadId":"44819","inReplyTo":"20170105192904.1107-1-sbeller@google.com","subject":"[PATCHv6 2/2] pathspec: give better message for submodule related pathspec error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-05T19:29:04Z","receivedAt":"2017-01-05T19:33:08Z","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\nNote: There is a case (e.g. \"git -C submodule add .\") in which we call\nstrip_submodule_slash_expensive, as git-add requests it via the flag\nPATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE, but the assert used to\ntrigger nevertheless, because the flag PATHSPEC_LITERAL was not set,\nsuch that we executed\n\n\tif (item->nowildcard_len < prefixlen)\n\t\titem->nowildcard_len = prefixlen;\n\nand prefixlen was not adapted (e.g. it was computed from \"submodule/\")\nSo in the die_inside_submodule_path function we also need handle paths,\nthat were stripped before, i.e. are the exact submodule path. This\nis why the conditions in die_inside_submodule_path are slightly\ndifferent than in strip_submodule_slash_expensive.\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 \nHelped-by: Junio C Hamano \nSigned-off-by: Stefan Beller \n---\n pathspec.c | 35 +++++++++++++++++++++++++++++++++--\n t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 66 insertions(+), 2 deletions(-)\n create mode 100755 t/t6134-pathspec-in-submodule.sh\n\ndiff --git a/pathspec.c b/pathspec.c\nindex d4efcf6662..42cd83c235 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -296,6 +296,27 @@ static void strip_submodule_slash_expensive(struct pathspec_item *item)\n \t}\n }\n \n+static void die_inside_submodule_path(struct pathspec_item *item)\n+{\n+\tint i;\n+\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 !(item->match[ce_len] == '/' || item->match[ce_len] == '\\0') ||\n+\t\t memcmp(ce->name, item->match, ce_len))\n+\t\t\tcontinue;\n+\n+\t\tdie(_(\"Pathspec '%s' is in submodule '%.*s'\"),\n+\t\t item->original, ce_len, ce->name);\n+\t}\n+}\n+\n /*\n * Perform the initialization of a pathspec_item based on a pathspec element.\n */\n@@ -391,8 +412,18 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\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 * This case can be triggered by the user pointing us to a\n+\t\t * pathspec inside a submodule, which is an input error.\n+\t\t * Detect that here and complain, but fallback in the\n+\t\t * non-submodule case to a BUG, as we have no idea what\n+\t\t * would trigger that.\n+\t\t */\n+\t\tdie_inside_submodule_path(item);\n+\t\tdie (\"BUG: item->nowildcard_len > item->len || item->prefix > item->len)\");\n+\t}\n }\n \n static int pathspec_item_cmp(const void *a_, const void *b_)\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 <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 <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.31.g919a8d0.dirty\n\n"},{"id":"308830","messageId":"20170105192904.1107-2-sbeller@google.com","threadId":"44819","inReplyTo":"20170105192904.1107-1-sbeller@google.com","subject":"[PATCHv6 1/2] submodule tests: don't use itself as a submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-05T19:29:03Z","receivedAt":"2017-01-05T19:33:12Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In reality nobody would run \"git submodule add ./. \"\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 \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.31.g919a8d0.dirty\n\n"},{"id":"308831","messageId":"20170105192904.1107-1-sbeller@google.com","threadId":"44819","inReplyTo":null,"subject":"[PATCHv6 0/2] pathspec: give better message for submodule related pathspec error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-05T19:29:02Z","receivedAt":"2017-01-05T19:33:16Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v6:\n* rebased on top of origin/bw/pathspec-cleanup, resolving conflicts.\n (Additionally needs merging with origin/sb/submodule-embed-gitdir to have \n 6f94351b0, test-lib-functions.sh: teach test_commit -C )\n* reworded comments and commit message\n* do not reuse the strip_submodule_slash_expensive function, but have\n a dedicated die_inside_submodule_path function.\n\nv5:\n* was just resending the latest patch, which turns out to be in conflict with\n origin/bw/pathspec-cleanup\n\nv4:\n> 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.\nThe actual fixing patch was reworded borrowing some words from Jeff.\nAs this makes use of \"test_commit -C\", it goes on top of sb/submodule-embed-gitdir\n\nv3:\nmore defensive and with tests.\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 | 31 ++++++++++++++++++++++---------\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(+), 13 deletions(-)\n create mode 100755 t/t6134-pathspec-in-submodule.sh\n\n-- \n2.11.0.31.g919a8d0.dirty\n\n"},{"id":"308976","messageId":"xmqqh959ynb4.fsf@gitster.mtv.corp.google.com","threadId":"44819","inReplyTo":"20170105192904.1107-2-sbeller@google.com","subject":"Re: [PATCHv6 1/2] submodule tests: don't use itself as a submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-09T02:33:35Z","receivedAt":"2017-01-09T02:33:44Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This provides an easier way to have submodules in tests, by just setting\n> TEST_CREATE_SUBMODULE to a non empty string, similar to\n> TEST_NO_CREATE_REPO.\n\nYuck. \n\nI find it doubtful that it is a good idea to create two submodule\nrepositories by merely dot-including the test-lib.sh; I find it\ndoubly doubtful that it is a good idea to make test_create_repo\npay attention to the special variable to implement that.\n\nI am OK with a solution where callers that set TEST_CREATE_SUBMODULE\nvariable in this patch to instead have an explicit call\n\n\ttest_create_repo --submodule pretzel\n\nThat would be a lot more obvious.\n\nThe primary reason why I hate the implementation in this patch is\nthat it is very easy for a test that says TEST_CREATE_SUBMODULE\nupfront, only to get the initial test repository (which everybody\nelse gets) with two test submodules, to later gain a test that wants\nto use a separate repository and call \"test_create_repo\". It will\nalways get the pretzel submodules, which may or may not match what\nthe test writer who adds a new test needs.\n\n> Make use of it in those tests that add a submodule from ./. except for\n> the occurrence in create_lib_submodule_repo as there it seems we craft\n> a repository deliberately for both inside as well as outside use.\n\nBut isn't the point of this change that use of ./. cannot be\nmimicking any real-world use, hence pointless for the purpose of\nreally testing the components of the system? If \"we craft\ndeliberately for both inside and outside use\" indeed _IS_ a good\nthing, then perhaps use of ./. has practical real-world use---if\nnot, wouldn't we want to fix the scripts that include the\nlib-submodule-repo helper not to test such an unrealistic layout?\n\n"},{"id":"308977","messageId":"xmqqbmvhyn1m.fsf@gitster.mtv.corp.google.com","threadId":"44819","inReplyTo":"20170105192904.1107-3-sbeller@google.com","subject":"Re: [PATCHv6 2/2] pathspec: give better message for submodule related pathspec error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-09T02:39:17Z","receivedAt":"2017-01-09T02:39:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\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>\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> Note: There is a case (e.g. \"git -C submodule add .\") in which we call\n> strip_submodule_slash_expensive, as git-add requests it via the flag\n> PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE, but the assert used to\n> trigger nevertheless, because the flag PATHSPEC_LITERAL was not set,\n> such that we executed\n>\n> \tif (item->nowildcard_len < prefixlen)\n> \t\titem->nowildcard_len = prefixlen;\n>\n> and prefixlen was not adapted (e.g. it was computed from \"submodule/\")\n> So in the die_inside_submodule_path function we also need handle paths,\n> that were stripped before, i.e. are the exact submodule path. This\n> is why the conditions in die_inside_submodule_path are slightly\n> different than in strip_submodule_slash_expensive.\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 \n> Helped-by: Junio C Hamano \n> Signed-off-by: Stefan Beller \n> ---\n\nFor future reference, do not bury a useful fix behind unproven new\nthings. The main purpose of this two-patch series is this change,\nand it does not have to wait for the change to allow test_commit to\nnotice \"-C\" you have in another series.\n\nJust write it in longhand, and when both topics graduate, send in\nanother patch to update \"(cd && test_commit )\" to use\nthe new \"test_commit -C \".\n\nThanks.\n\n> pathspec.c | 35 +++++++++++++++++++++++++++++++++--\n> t/t6134-pathspec-in-submodule.sh | 33 +++++++++++++++++++++++++++++++++\n> 2 files changed, 66 insertions(+), 2 deletions(-)\n> create mode 100755 t/t6134-pathspec-in-submodule.sh\n>\n> diff --git a/pathspec.c b/pathspec.c\n> index d4efcf6662..42cd83c235 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -296,6 +296,27 @@ static void strip_submodule_slash_expensive(struct pathspec_item *item)\n> \t}\n> }\n> \n> +static void die_inside_submodule_path(struct pathspec_item *item)\n> +{\n> +\tint i;\n> +\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 !(item->match[ce_len] == '/' || item->match[ce_len] == '\\0') ||\n> +\t\t memcmp(ce->name, item->match, ce_len))\n> +\t\t\tcontinue;\n> +\n> +\t\tdie(_(\"Pathspec '%s' is in submodule '%.*s'\"),\n> +\t\t item->original, ce_len, ce->name);\n> +\t}\n> +}\n> +\n> /*\n> * Perform the initialization of a pathspec_item based on a pathspec element.\n> */\n> @@ -391,8 +412,18 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\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 * This case can be triggered by the user pointing us to a\n> +\t\t * pathspec inside a submodule, which is an input error.\n> +\t\t * Detect that here and complain, but fallback in the\n> +\t\t * non-submodule case to a BUG, as we have no idea what\n> +\t\t * would trigger that.\n> +\t\t */\n> +\t\tdie_inside_submodule_path(item);\n> +\t\tdie (\"BUG: item->nowildcard_len > item->len || item->prefix > item->len)\");\n> +\t}\n> }\n> \n> static int pathspec_item_cmp(const void *a_, const void *b_)\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 <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 <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":"309065","messageId":"CAGZ79kbkTf5mT39p6mznGuCwptdVx7hc8N8P=mKo4aoV2=-JVw@mail.gmail.com","threadId":"44819","inReplyTo":"xmqqbmvhyn1m.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv6 2/2] pathspec: give better message for submodule related pathspec error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-09T20:03:59Z","receivedAt":"2017-01-09T20:04:05Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Jan 8, 2017 at 6:39 PM, Junio C Hamano wrote:\n> For future reference, do not bury a useful fix behind unproven new\n> things. The main purpose of this two-patch series is this change,\n> and it does not have to wait for the change to allow test_commit to\n> notice \"-C\" you have in another series.\n\nnoted.\n\nThe bug fixed here doesn't need to be fast-tracked IMO, because\nof its history. (It's well documented on the web already)\n\nI think this series can wait until the\n\"test_commit -C\" is available.\n\nIn a reroll I'll drop the dependency then.\n"},{"id":"309066","messageId":"CAGZ79kak8qzUG5G1mM8uDqxW8tVBNmuLcvMhopby_U8PvUOjJg@mail.gmail.com","threadId":"44819","inReplyTo":"xmqqh959ynb4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv6 1/2] submodule tests: don't use itself as a submodule","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-09T20:10:49Z","receivedAt":"2017-01-09T20:10:55Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Jan 8, 2017 at 6:33 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> This provides an easier way to have submodules in tests, by just setting\n>> TEST_CREATE_SUBMODULE to a non empty string, similar to\n>> TEST_NO_CREATE_REPO.\n>\n> Yuck.\n>\n> I find it doubtful that it is a good idea to create two submodule\n> repositories by merely dot-including the test-lib.sh; I find it\n> doubly doubtful that it is a good idea to make test_create_repo\n> pay attention to the special variable to implement that.\n>\n> I am OK with a solution where callers that set TEST_CREATE_SUBMODULE\n> variable in this patch to instead have an explicit call\n>\n> test_create_repo --submodule pretzel\n>\n> That would be a lot more obvious.\n\nagreed.\n\n>\n> The primary reason why I hate the implementation in this patch is\n> that it is very easy for a test that says TEST_CREATE_SUBMODULE\n> upfront, only to get the initial test repository (which everybody\n> else gets) with two test submodules, to later gain a test that wants\n> to use a separate repository and call \"test_create_repo\". It will\n> always get the pretzel submodules, which may or may not match what\n> the test writer who adds a new test needs.\n\nI agree. At the time of writing the patch series, I was anticipating writing\nway more submodule tests, but now I have these future tests integrated into\nlib-submodule-update.sh.\n\n>\n>> Make use of it in those tests that add a submodule from ./. except for\n>> the occurrence in create_lib_submodule_repo as there it seems we craft\n>> a repository deliberately for both inside as well as outside use.\n>\n> But isn't the point of this change that use of ./. cannot be\n> mimicking any real-world use, hence pointless for the purpose of\n> really testing the components of the system? If \"we craft\n> deliberately for both inside and outside use\" indeed _IS_ a good\n> thing, then perhaps use of ./. has practical real-world use---if\n> not, wouldn't we want to fix the scripts that include the\n> lib-submodule-repo helper not to test such an unrealistic layout?\n>\n\nMakes sense; I tried to fix it up to avoid the ./. clone in\ncreate_lib_submodule_repo, but the issue is there are too many\nimplicit assumption of these two repos, such that a faithful conversion\nwould just duplicate code for the submodule, (e.g. create the same\namount of commits, containing the same diffs, etc.), which then\ncan be argued to just slow down the test suite as the clone from ./.\nis actually reducing the needed work by a factor of 2.\n"}]}