{"thread":{"id":"44777","subject":"[PATCH] pathspec: give better message for submodule related pathspec error","startedAt":"2016-12-28T00:06:21Z","lastAt":"2016-12-28T18:15:48Z","messageCount":5,"participants":["Stefan Beller","Jeff King","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"308430","messageId":"20161228000559.17842-1-sbeller@google.com","threadId":"44777","inReplyTo":null,"subject":"[PATCH] pathspec: give better message for submodule related pathspec error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-28T00:05:59Z","receivedAt":"2016-12-28T00:06:21Z","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\nIf you were following the mailing list closely today, you may sense\nthat I am cleaning up stalled branches. :)\n\nI think such a hot fix is warranted given how often we had reports\non the mailing list.\n\nThanks,\nStefan\n\n pathspec.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 22ca74a126..d522f43331 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -313,8 +313,11 @@ 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\tdie (_(\"Path leads inside submodule '%s', but the submodule \"\n+\t\t       \"was not recognized, i.e. not initialized or deleted\"),\n+\t\t       ce->name);\n \treturn magic;\n }\n \n-- \n2.11.0.196.gee862f456e.dirty\n\n"},{"id":"308434","messageId":"20161228055826.xu2gclwkvisbft6o@sigill.intra.peff.net","threadId":"44777","inReplyTo":"20161228000559.17842-1-sbeller@google.com","subject":"Re: [PATCH] pathspec: give better message for submodule related pathspec error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-28T05:58:26Z","receivedAt":"2016-12-28T05:58:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 27, 2016 at 04:05:59PM -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].\n\nIf people are running into it, it definitely should not be an assert,\nnor a die(\"BUG\"). It should be a regular die(), which your patch does.\nSo this is definitely a good step, even if the ultimate goal may be to\nhandle the case more gracefully (I say that without having even read the\nbackground, or knowing what the right handling would be).\n\nBut...\n\n> diff --git a/pathspec.c b/pathspec.c\n> index 22ca74a126..d522f43331 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -313,8 +313,11 @@ 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\tdie (_(\"Path leads inside submodule '%s', but the submodule \"\n> +\t\t       \"was not recognized, i.e. not initialized or deleted\"),\n> +\t\t       ce->name);\n\nDon't you need to flip the logic here? An assert() triggers when the\ncondition is not true, but an \"if\" does the opposite. So \"assert(X)\"\nshould always become \"if (!X) die(...)\".\n\n-Peff\n"},{"id":"308451","messageId":"20161228171746.22859-1-sbeller@google.com","threadId":"44777","inReplyTo":"20161228055826.xu2gclwkvisbft6o@sigill.intra.peff.net","subject":"[PATCHv2] pathspec: give better message for submodule related pathspec error","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-28T17:17:46Z","receivedAt":"2016-12-28T17:18:04Z","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].\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\nPeff wrote:\n> Don't you need to flip the logic here? An assert() triggers when the\n> condition is not true, but an \"if\" does the opposite. So \"assert(X)\"\n> should always become \"if (!X) die(...)\".\n\nDuh! and it should compile as well. \n\nThanks,\nStefan\n\n pathspec.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/pathspec.c b/pathspec.c\nindex 22ca74a126..4724d522f2 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -313,8 +313,11 @@ 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\tdie (_(\"Path leads inside submodule '%s', but the submodule \"\n+\t\t       \"was not recognized, i.e. not initialized or deleted\"),\n+\t\t       item->original);\n \treturn magic;\n }\n \n-- \n2.11.0.196.gee862f456e.dirty\n\n"},{"id":"308460","messageId":"20161228181335.GA33595@google.com","threadId":"44777","inReplyTo":"20161228000559.17842-1-sbeller@google.com","subject":"Re: [PATCH] pathspec: give better message for submodule related pathspec error","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-12-28T18:13:35Z","receivedAt":"2016-12-28T18:13:43Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/27, Stefan Beller wrote:\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> [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> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> \n> If you were following the mailing list closely today, you may sense\n> that I am cleaning up stalled branches. :)\n> \n> I think such a hot fix is warranted given how often we had reports\n> on the mailing list.\n> \n> Thanks,\n> Stefan\n> \n>  pathspec.c | 7 +++++--\n>  1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/pathspec.c b/pathspec.c\n> index 22ca74a126..d522f43331 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -313,8 +313,11 @@ 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\tdie (_(\"Path leads inside submodule '%s', but the submodule \"\n> +\t\t       \"was not recognized, i.e. not initialized or deleted\"),\n> +\t\t       ce->name);\n>  \treturn magic;\n\nI haven't been following everything on the list these past couple days,\nbut are we sure this is caused by submodules?  Also variable 'ce'\nshouldn't be in scope here.\n\n-- \nBrandon Williams\n"},{"id":"308461","messageId":"20161228181518.GB33595@google.com","threadId":"44777","inReplyTo":"20161228171746.22859-1-sbeller@google.com","subject":"Re: [PATCHv2] pathspec: give better message for submodule related pathspec error","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-12-28T18:15:18Z","receivedAt":"2016-12-28T18:15:48Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/28, Stefan Beller wrote:\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> [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> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> \n> Peff wrote:\n> > Don't you need to flip the logic here? An assert() triggers when the\n> > condition is not true, but an \"if\" does the opposite. So \"assert(X)\"\n> > should always become \"if (!X) die(...)\".\n> \n> Duh! and it should compile as well. \n> \n> Thanks,\n> Stefan\n> \n>  pathspec.c | 7 +++++--\n>  1 file changed, 5 insertions(+), 2 deletions(-)\n> \n> diff --git a/pathspec.c b/pathspec.c\n> index 22ca74a126..4724d522f2 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -313,8 +313,11 @@ 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\tdie (_(\"Path leads inside submodule '%s', but the submodule \"\n> +\t\t       \"was not recognized, i.e. not initialized or deleted\"),\n> +\t\t       item->original);\n>  \treturn magic;\n>  }\n\nTurns out I should comment on the most recent version of the patch :P\nThis looks better to me. (It resolves the issue with using a variable\nnot in scope).\n\n-- \nBrandon Williams\n"}]}