{"thread":{"id":"44842","subject":"[PATCHv7] pathspec: give better message for submodule related pathspec error","startedAt":"2017-01-09T22:43:46Z","lastAt":"2017-01-10T18:14:43Z","messageCount":5,"participants":["Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"309073","messageId":"20170109224330.28405-1-sbeller@google.com","threadId":"44842","inReplyTo":null,"subject":"[PATCHv7] pathspec: give better message for submodule related pathspec error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-09T22:43:30Z","receivedAt":"2017-01-09T22:43: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\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\n This comes as a single patch again, replacing sb/pathspec-errors.\n It goes directly on top of bw/pathspec-cleanup.\n \n v7:\n do not rely on \"test_commit -C\" being there, nor the infrastructure\n to request a \"good\" submodule upstream. Just create a submodule outselves\n to test in.\n \n Thanks,\n Stefan\n\n pathspec.c | 35 +++++++++++++++++++++++++++++++++--\n t/t6134-pathspec-in-submodule.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 73 insertions(+), 2 deletions(-)\n create mode 100755 t/t6134-pathspec-in-submodule.sh\n\ndiff --git a/pathspec.c b/pathspec.c\nindex ff2509ddd1..7ababb3159 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..d952ae2cae\n--- /dev/null\n+++ b/t/t6134-pathspec-in-submodule.sh\n@@ -0,0 +1,40 @@\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+\ttest_create_repo pretzel &&\n+\t(\n+\t\tcd pretzel &&\n+\t\ttouch a &&\n+\t\tgit add a &&\n+\t\tgit commit -m \"add a file\" -- a\n+\t) &&\n+\tgit submodule add ./pretzel 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.rc2.30.g7c4be45.dirty\n\n"},{"id":"309074","messageId":"xmqqeg0bu9oq.fsf@gitster.mtv.corp.google.com","threadId":"44842","inReplyTo":"20170109224330.28405-1-sbeller@google.com","subject":"Re: [PATCHv7] pathspec: give better message for submodule related pathspec error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-09T22:53:41Z","receivedAt":"2017-01-09T22:53:49Z","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\nIs it only me who sees funny black rectangles in front of these four\nlines instead of blanks, by the way?\n\n> This comes as a single patch again, replacing sb/pathspec-errors.\n> It goes directly on top of bw/pathspec-cleanup.\n> \n> v7:\n> do not rely on \"test_commit -C\" being there, nor the infrastructure\n> to request a \"good\" submodule upstream. Just create a submodule outselves\n> to test in.\n> \n\nThanks.\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..d952ae2cae\n> --- /dev/null\n> +++ b/t/t6134-pathspec-in-submodule.sh\n> @@ -0,0 +1,40 @@\n> +#!/bin/sh\n> +\n> +test_description='test case exclude pathspec'\n> +\n> +TEST_CREATE_SUBMODULE=yes\n\nDid you mean to keep this?\n\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'setup a submodule' '\n> +\ttest_create_repo pretzel &&\n> +\t(\n> +\t\tcd pretzel &&\n> +\t\ttouch a &&\n\nThis is better spelled as\n\n\t\t: >a &&\n\nbecause use of touch, when you do not care about the file timestamp,\nis misleading.\n\nThanks.\n"},{"id":"309077","messageId":"CAGZ79kZVKFvijfwEfmiaahz2VYgaa=m2m-ty2tbOXh2toO_muQ@mail.gmail.com","threadId":"44842","inReplyTo":"xmqqeg0bu9oq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv7] pathspec: give better message for submodule related pathspec error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-09T23:03:55Z","receivedAt":"2017-01-09T23:04:01Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jan 9, 2017 at 2:53 PM, Junio C Hamano wrote:\n\n> Is it only me who sees funny black rectangles in front of these four\n> lines instead of blanks, by the way?\n\nI copied this part literally from Jeff. My mail display-client (gmail\nat the time)\ndid mangle it apparently, (it inserted some non-breaking white space characters\nas well as a character that I can not really identify, as it is no\nvalid UTF8 and bogus\nASCII)\n\n\n>> +TEST_CREATE_SUBMODULE=yes\n>\n> Did you mean to keep this?\n\nNo.\n\n\n> because use of touch, when you do not care about the file timestamp,\n> is misleading.\n\nmakes sense.\n\nI'll reroll again.\n\n>\n> Thanks.\n"},{"id":"309078","messageId":"20170109231650.9043-1-sbeller@google.com","threadId":"44842","inReplyTo":"CAGZ79kZVKFvijfwEfmiaahz2VYgaa=m2m-ty2tbOXh2toO_muQ@mail.gmail.com","subject":"[PATCHv8] pathspec: give better message for submodule related pathspec error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-01-09T23:16:50Z","receivedAt":"2017-01-09T23:17:07Z","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\n This comes as a single patch again, replacing sb/pathspec-errors.\n It goes directly on top of bw/pathspec-cleanup.\n\n v8:\n cleaned white spaces in commit message\n removed TEST_CREATE_SUBMODULE=yes\n : > instead of touch.\n\n v7:\n do not rely on \"test_commit -C\" being there, nor the infrastructure\n to request a \"good\" submodule upstream. Just create a submodule outselves\n to test in.\n\n Thanks,\n Stefan\n\n pathspec.c | 35 +++++++++++++++++++++++++++++++++--\n t/t6134-pathspec-in-submodule.sh | 36 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 69 insertions(+), 2 deletions(-)\n create mode 100755 t/t6134-pathspec-in-submodule.sh\n\ndiff --git a/pathspec.c b/pathspec.c\nindex ff2509ddd1..7ababb3159 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..fd401ca605\n--- /dev/null\n+++ b/t/t6134-pathspec-in-submodule.sh\n@@ -0,0 +1,36 @@\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_create_repo pretzel &&\n+\t: >pretzel/a &&\n+\tgit -C pretzel add a &&\n+\tgit -C pretzel commit -m \"add a file\" -- a &&\n+\tgit submodule add ./pretzel 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.rc2.30.g7c4be45.dirty\n\n"},{"id":"309148","messageId":"xmqqr34asrzv.fsf@gitster.mtv.corp.google.com","threadId":"44842","inReplyTo":"20170109231650.9043-1-sbeller@google.com","subject":"Re: [PATCHv8] pathspec: give better message for submodule related pathspec error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-10T18:13:24Z","receivedAt":"2017-01-10T18:14:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This comes as a single patch again, replacing sb/pathspec-errors.\n> It goes directly on top of bw/pathspec-cleanup.\n>\n> v8:\n> cleaned white spaces in commit message\n> removed TEST_CREATE_SUBMODULE=yes\n> : > instead of touch.\n>\n> v7:\n> do not rely on \"test_commit -C\" being there, nor the infrastructure\n> to request a \"good\" submodule upstream. Just create a submodule outselves\n> to test in.\n>\n> Thanks,\n> Stefan\n\nThanks, queued.\n"}]}