{"thread":{"id":"49355","subject":"[Bug] Pathspec matching breaks the add command","startedAt":"2018-09-17T01:53:01Z","lastAt":"2018-09-20T21:40:34Z","messageCount":7,"participants":["smaudet@sebastianaudet.com","Duy Nguyen","Ævar Arnfjörð Bjarmason","Stefan Beller","Nguyễn Thái Ngọc Duy","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"358242","messageId":"20180917015259.GA26339@sebastianaudet.com","threadId":"49355","inReplyTo":null,"subject":"[Bug] Pathspec matching breaks the add command","fromName":"","fromEmail":"smaudet@sebastianaudet.com","sentAt":"2018-09-17T01:52:59Z","receivedAt":"2018-09-17T01:53:01Z","isPatch":false,"sender":{"key":"smaudet@sebastianaudet.com","avatar":null},"body":"The following:\n\ngit add -u :\\(glob,attr:-someAttr\\):src/**\n\nProduces an error that, according to the source code, should never be visible to the user. This attribute/pathspec *should* be supported according to the documentation provided by git:\n\nfatal: BUG:builtin/add.c:498: unsupported magic 40\n\nIt looks like the documentation claims more features than the source code supports, perhaps incorrectly because I think you shouldn't be restricted to a set of attributes, and the source code\ndoesn't properly do its job anyways and never handles this scenario.\n\nThis should be fixed, if I have any time I'll look into what it would take to submit a patch, but I don't have the time for this right now.\n"},{"id":"358287","messageId":"CACsJy8CAyXAaax8dSPUUzTFvpKVG19FDives3JthL4hBxgf=6A@mail.gmail.com","threadId":"49355","inReplyTo":"20180917015259.GA26339@sebastianaudet.com","subject":"Re: [Bug] Pathspec matching breaks the add command","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-09-17T16:28:40Z","receivedAt":"2018-09-17T16:29:08Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Sep 17, 2018 at 3:55 AM wrote:\n>\n> The following:\n>\n> git add -u :\\(glob,attr:-someAttr\\):src/**\n>\n> Produces an error that, according to the source code, should never be visible to the user. This attribute/pathspec *should* be supported according to the documentation provided by git:\n>\n> fatal: BUG:builtin/add.c:498: unsupported magic 40\n\nBrandon, b0db704652 (pathspec: allow querying for attributes -\n2017-03-13) added attr support to match_pathspec(), but this add.c\nmanipulates pathspec directly and (I think) does not support 'attr'\nattribute, which is correctly caught here.\n\nI think we need to update the parse_pathspec() call in this file to\ndeclare 'attr' not supported. Then we get a friendlier error (or you\nmake this code work too, that's even better). You may want to go\nthrough all parse_pathspec() call and make sure that if the pathspec\nis not consumed only by match_pathspec() then it should reject 'attr',\notherwise we'll get this \"BUG\" again.\n-- \nDuy\n"},{"id":"358303","messageId":"87zhwgj9mc.fsf@evledraar.gmail.com","threadId":"49355","inReplyTo":"CACsJy8CAyXAaax8dSPUUzTFvpKVG19FDives3JthL4hBxgf=6A@mail.gmail.com","subject":"Brandon Williams's E-Mail bouncing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-09-17T17:51:07Z","receivedAt":"2018-09-17T17:51:13Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\n[Sorry about the double send in private, forgot to CC the list]\n\nOn Mon, Sep 17 2018, Duy Nguyen wrote:\n\n> Brandon, b0db704652[...]\n\nI noticed a few days ago that CC's to bmwill@google.com bounce. Has he\nleft Google, and if so is he still interested in being CC'd on Git stuff\n(maybe he'll chime in), or not interested?\n\nAside from that particular address bouncing (or not, maybe Google's MX\njust dislikes me), it would be nice if git\n{format-patch,send-email,check-contacts} & the .mailmap format would\nsupport and understand some way to map addresses to\ne.g. noreply@example.com, and prune them out appropriately. We have a\nlot of addresses from past authors which bounce, and where no current\ncontact address is known.\n"},{"id":"358305","messageId":"CAGZ79ka4hQYHRc-=LkGFbFmzhJfj8+nOTUB41PnajySXeLuCFw@mail.gmail.com","threadId":"49355","inReplyTo":"87zhwgj9mc.fsf@evledraar.gmail.com","subject":"Re: Brandon Williams's E-Mail bouncing","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-09-17T18:04:43Z","receivedAt":"2018-09-17T18:04:57Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Sep 17, 2018 at 10:51 AM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> Aside from that particular address bouncing [...] it would be nice if git\n> {format-patch,send-email,check-contacts} & the .mailmap format would\n> support and understand some way to map addresses to\n> e.g. noreply@example.com, and prune them out appropriately. We have a\n> lot of addresses from past authors which bounce, and where no current\n> contact address is known.\n\nTombstones, eh?\n\nFor send-email I would very much prefer the proactive warning instead\nof silently dropping that email address as I would want to know, so I can\ntry to follow up somehow and deal with it as-is.\n\nI think the main purpose of the mail map file until now was to consolidate\nmultiple identities into one (e.g. misspellings or capitalisation issues in\nnames, different email addresses for all the same person), now you want to\nextend it to also contain more information about an author-ident, such as\nemails being active? Or recording if someone is not interested in the project\nanymore? I think recording that an email bouncesl is just duplicating\ninformation\nthat can be easily checked. Recording that a particular contributor doesn't\nhave time/interest any more is slightly different, but IMHO the mail map file\nis also not very well suited for that endeavor.\n\nOh, the above paragraph is written with git - the tool - in mind, not\nthe git.git\nrepository where we can (ab)use our own tooling. ;-)\n\nSo mapping all bouncing emails to no-reply@example.com would map them\nto the same author ident, which we would not want. (e.g. we'd still want to\nknow how many different people contributed so far, hence keep them separate).\nWe'd need to have unique email addresses that still explain their intent,\ni.e. +no-reply@example.org would work?\n\nThanks,\nStefan\n"},{"id":"358404","messageId":"20180918173159.30300-1-pclouds@gmail.com","threadId":"49355","inReplyTo":"20180917015259.GA26339@sebastianaudet.com","subject":"[PATCH] add: do not accept pathspec magic 'attr'","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-09-18T17:31:59Z","receivedAt":"2018-09-18T17:32:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Commit b0db704652 (pathspec: allow querying for attributes -\n2017-03-13) adds new pathspec magic 'attr' but only with\nmatch_pathspec(). \"git add\" has some pathspec related code that still\ndoes not know about 'attr' and will bail out:\n\n $ git add ':(attr:foo)'\n fatal: BUG:dir.c:1584: unsupported magic 40\n\nA better solution would be making this code support 'attr'. But I\ndon't know how much work is needed (I'm not familiar with this new\nmagic). For now, let's simply reject this magic with a friendlier\nmessage:\n\n $ git add ':(attr:foo)'\n fatal: :(attr:foo): pathspec magic not supported by this command: 'attr'\n\nReported-by: smaudet@sebastianaudet.com\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Since Brandon is currently unreachable, let's do this for now. I\n might eventually find time to go over pathspec code and see if I can\n add 'attr' support to the rest of the commands, but no promise.\n\n builtin/add.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 9916498a29..0b64bcdebe 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -454,7 +454,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t * Check the \"pathspec '%s' did not match any files\" block\n \t * below before enabling new magic.\n \t */\n-\tparse_pathspec(&pathspec, 0,\n+\tparse_pathspec(&pathspec, PATHSPEC_ATTR,\n \t\t PATHSPEC_PREFER_FULL |\n \t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t prefix, argv);\n-- \n2.19.0.rc0.337.ge906d732e7\n\n"},{"id":"358478","messageId":"xmqqk1nh47no.fsf@gitster-ct.c.googlers.com","threadId":"49355","inReplyTo":"20180918173159.30300-1-pclouds@gmail.com","subject":"Re: [PATCH] add: do not accept pathspec magic 'attr'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-19T19:19:23Z","receivedAt":"2018-09-19T19:19:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Commit b0db704652 (pathspec: allow querying for attributes -\n> 2017-03-13) adds new pathspec magic 'attr' but only with\n> match_pathspec(). \"git add\" has some pathspec related code that still\n> does not know about 'attr' and will bail out:\n>\n> $ git add ':(attr:foo)'\n> fatal: BUG:dir.c:1584: unsupported magic 40\n>\n> A better solution would be making this code support 'attr'. But I\n> don't know how much work is needed (I'm not familiar with this new\n> magic). For now, let's simply reject this magic with a friendlier\n> message:\n>\n> $ git add ':(attr:foo)'\n> fatal: :(attr:foo): pathspec magic not supported by this command: 'attr'\n>\n> Reported-by: smaudet@sebastianaudet.com\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> Since Brandon is currently unreachable, let's do this for now.\n\nThanks. This certainly make it better than the status quo.\n"},{"id":"358542","messageId":"xmqq36u3zw36.fsf@gitster-ct.c.googlers.com","threadId":"49355","inReplyTo":"xmqqk1nh47no.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] add: do not accept pathspec magic 'attr'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-20T21:40:29Z","receivedAt":"2018-09-20T21:40:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> Commit b0db704652 (pathspec: allow querying for attributes -\n>> 2017-03-13) adds new pathspec magic 'attr' but only with\n>> match_pathspec(). \"git add\" has some pathspec related code that still\n>> does not know about 'attr' and will bail out:\n>>\n>> $ git add ':(attr:foo)'\n>> fatal: BUG:dir.c:1584: unsupported magic 40\n>>\n>> A better solution would be making this code support 'attr'. But I\n>> don't know how much work is needed (I'm not familiar with this new\n>> magic). For now, let's simply reject this magic with a friendlier\n>> message:\n>>\n>> $ git add ':(attr:foo)'\n>> fatal: :(attr:foo): pathspec magic not supported by this command: 'attr'\n>>\n>> Reported-by: smaudet@sebastianaudet.com\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> ---\n>> Since Brandon is currently unreachable, let's do this for now.\n>\n> Thanks. This certainly make it better than the status quo.\n\nAnd of course, there is an interesting glitch found after I almost\nfinish day's integration cycle X-<.\n\n-- >8 --\nSubject: [PATCH] fixup! add: do not accept pathspec magic 'attr'\n\n[Add the following when squashing]\n\nUpdate t6135 so that the expected error message is from the\n\"graceful\" rejection codepath, not \"oops, we were supposed to reject\nthe request to trigger this magic\" codepath.\n\nSigned-off-by: Junio C Hamano \n---\n t/t6135-pathspec-with-attrs.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex 77b8cef661..e436a73962 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -164,11 +164,11 @@ test_expect_success 'fail if attr magic is used places not implemented' '\n \t# when you attempt to use attr magic in commands that do not implement\n \t# attr magic. This test does not advocate git-add to stay that way,\n \t# though, but git-add is convenient as it has its own internal pathspec\n \t# parsing.\n \ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n-\ttest_i18ngrep \"unsupported magic\" actual\n+\ttest_i18ngrep \"magic not supported\" actual\n '\n \n test_expect_success 'abort on giving invalid label on the command line' '\n \ttest_must_fail git ls-files . \":(attr:☺)\"\n '\n-- \n2.19.0-216-g2d3b1c576c\n\n"}]}