{"thread":{"id":"50250","subject":"Change on check-attr behavior","startedAt":"2019-01-17T15:47:22Z","lastAt":"2019-01-23T05:40:30Z","messageCount":15,"participants":["Sérgio Peixoto","Jeff King","Stefan Beller","Duy Nguyen","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"366998","messageId":"CAK3b1G+88a=xfO=6wfRi1SMy3xtca2NcFyxuBLKwSifb_L9xwA@mail.gmail.com","threadId":"50250","inReplyTo":null,"subject":"Change on check-attr behavior","fromName":"Sérgio Peixoto","fromEmail":"sergio.peixoto@gmail.com","sentAt":"2019-01-17T15:47:09Z","receivedAt":"2019-01-17T15:47:22Z","isPatch":false,"sender":{"key":"sergio.peixoto@gmail.com","avatar":null},"body":"Hi,\n\nI think there is a bug on the check-attr behavior of git when asking\nfor the \"allowed-ext\" attribute. Check the logs below to see that\nwith version 2.20.1.windows.1 we get unspecified even the attribute\nis there as you can see when asking for all the attributes.\n\n=== OLD VERSION ===\n> git version\ngit version 2.8.1.windows.1\n\n> git check-attr -a test.py\ntest.py: text: set\ntest.py: allowed-ext: 100\n\n> git check-attr allowed-ext test.py\ntest.py: allowed-ext: 100\n\n=== NEW VERSION ===\n> git --version\ngit version 2.20.1.windows.1\n\n> git check-attr -a test.py\ntest.py: text: set\ntest.py: allowed-ext: 100\n\n> git check-attr allowed-ext test.py\ntest.py: allowed-ext: unspecified\n\nCheers,\nSérgio Peixoto\n"},{"id":"366999","messageId":"20190117160752.GA29375@sigill.intra.peff.net","threadId":"50250","inReplyTo":"CAK3b1G+88a=xfO=6wfRi1SMy3xtca2NcFyxuBLKwSifb_L9xwA@mail.gmail.com","subject":"Re: Change on check-attr behavior","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-17T16:07:52Z","receivedAt":"2019-01-17T16:07:56Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 17, 2019 at 03:47:09PM +0000, Sérgio Peixoto wrote:\n\n> I think there is a bug on the check-attr behavior of git when asking\n> for the \"allowed-ext\" attribute. Check the logs below to see that\n> with version 2.20.1.windows.1 we get unspecified even the attribute\n> is there as you can see when asking for all the attributes.\n> \n> === OLD VERSION ===\n> > git version\n> git version 2.8.1.windows.1\n> \n> > git check-attr -a test.py\n> test.py: text: set\n> test.py: allowed-ext: 100\n> \n> > git check-attr allowed-ext test.py\n> test.py: allowed-ext: 100\n> \n> === NEW VERSION ===\n> > git --version\n> git version 2.20.1.windows.1\n> \n> > git check-attr -a test.py\n> test.py: text: set\n> test.py: allowed-ext: 100\n> \n> > git check-attr allowed-ext test.py\n> test.py: allowed-ext: unspecified\n\nI can't reproduce here (on Linux, but I don't think the attr code is\nparticularly Windows-specific). Can you show us what's in your\n.gitattributes file(s)?\n\n-Peff\n"},{"id":"367070","messageId":"CAK3b1GJPZ5X3uEP1a-NF9PZkE0tTKVLda5hM32jExVz_OD2E=g@mail.gmail.com","threadId":"50250","inReplyTo":"20190117160752.GA29375@sigill.intra.peff.net","subject":"Re: Change on check-attr behavior","fromName":"Sérgio Peixoto","fromEmail":"sergio.peixoto@gmail.com","sentAt":"2019-01-18T09:41:03Z","receivedAt":"2019-01-18T09:41:18Z","isPatch":false,"sender":{"key":"sergio.peixoto@gmail.com","avatar":null},"body":"Create a file .git/info/attributes with contents\n\nIf the contents are:\n[attr]allowed-ext\n*.py allowed-ext=100\n\nthen the problem occurs.\n\nIf contents are:\n#[attr]allowed-ext\n*.py allowed-ext=100\n\nthe problem is gone\n\nOn Thu, Jan 17, 2019 at 4:07 PM Jeff King wrote:\n>\n> On Thu, Jan 17, 2019 at 03:47:09PM +0000, Sérgio Peixoto wrote:\n>\n> > I think there is a bug on the check-attr behavior of git when asking\n> > for the \"allowed-ext\" attribute. Check the logs below to see that\n> > with version 2.20.1.windows.1 we get unspecified even the attribute\n> > is there as you can see when asking for all the attributes.\n> >\n> > === OLD VERSION ===\n> > > git version\n> > git version 2.8.1.windows.1\n> >\n> > > git check-attr -a test.py\n> > test.py: text: set\n> > test.py: allowed-ext: 100\n> >\n> > > git check-attr allowed-ext test.py\n> > test.py: allowed-ext: 100\n> >\n> > === NEW VERSION ===\n> > > git --version\n> > git version 2.20.1.windows.1\n> >\n> > > git check-attr -a test.py\n> > test.py: text: set\n> > test.py: allowed-ext: 100\n> >\n> > > git check-attr allowed-ext test.py\n> > test.py: allowed-ext: unspecified\n>\n> I can't reproduce here (on Linux, but I don't think the attr code is\n> particularly Windows-specific). Can you show us what's in your\n> .gitattributes file(s)?\n>\n> -Peff\n"},{"id":"367110","messageId":"20190118165800.GA9956@sigill.intra.peff.net","threadId":"50250","inReplyTo":"CAK3b1GJPZ5X3uEP1a-NF9PZkE0tTKVLda5hM32jExVz_OD2E=g@mail.gmail.com","subject":"Re: Change on check-attr behavior","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-18T16:58:01Z","receivedAt":"2019-01-18T16:58:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 18, 2019 at 09:41:03AM +0000, Sérgio Peixoto wrote:\n\n> Create a file .git/info/attributes with contents\n> \n> If the contents are:\n> [attr]allowed-ext\n> *.py allowed-ext=100\n> \n> then the problem occurs.\n> \n> If contents are:\n> #[attr]allowed-ext\n> *.py allowed-ext=100\n> \n> the problem is gone\n\nAh, thanks, that's the secret sauce: it only affects macros.\n\nI don't know how representative that attributes file is of your real\nrepo, but there's possibly one immediate workaround: there's no need for\nthe [attr] line here. You are free to define your own attributes, and\nonly need \"[attr]\" if you're defining a macro that expands to other\nattributes.\n\nNow, on to the actual bug. The simplest reproduction is:\n\n (echo \"[attr]foo bar\"; echo \"* foo\") >.gitattributes\n git check-attr foo file\n\nwhich should report \"foo\" as set. This bisects to 60a12722ac (attr:\nremove maybe-real, maybe-macro from git_attr, 2017-01-27), and it seems\nlike an unintentional regression there. I haven't yet poked into that\ncommit to see what the fix will look like.\n\n-Peff\n"},{"id":"367146","messageId":"20190118213458.GB28808@sigill.intra.peff.net","threadId":"50250","inReplyTo":"20190118165800.GA9956@sigill.intra.peff.net","subject":"[PATCH] attr: do not mark queried macros as unset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-18T21:34:58Z","receivedAt":"2019-01-18T21:35:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 18, 2019 at 11:58:01AM -0500, Jeff King wrote:\n\n> Now, on to the actual bug. The simplest reproduction is:\n> \n> (echo \"[attr]foo bar\"; echo \"* foo\") >.gitattributes\n> git check-attr foo file\n\nActually, even simpler is to just \"binary\", which is pre-defined as a\nmacro. :)\n\n> which should report \"foo\" as set. This bisects to 60a12722ac (attr:\n> remove maybe-real, maybe-macro from git_attr, 2017-01-27), and it seems\n> like an unintentional regression there. I haven't yet poked into that\n> commit to see what the fix will look like.\n\nSo here's the fix I came up with. +cc Duy, as this is really tangled\nwith his older 06a604e670.\n\n-- >8 --\nSubject: [PATCH] attr: do not mark queried macros as unset\n\nSince 60a12722ac (attr: remove maybe-real, maybe-macro from git_attr,\n2017-01-27), we will always mark an attribute macro (e.g., \"binary\")\nthat is specifically queried for as \"unspecified\", even though listing\n_all_ attributes would display it at set. E.g.:\n\n $ echo \"* binary\" >.gitattributes\n\n $ git check-attr -a file\n file: binary: set\n file: diff: unset\n file: merge: unset\n file: text: unset\n\n $ git check-attr binary file\n file: binary: unspecified\n\nThe problem stems from an incorrect conversion of the optimization from\n06a604e670 (attr: avoid heavy work when we know the specified attr is\nnot defined, 2014-12-28). There we tried in collect_some_attrs() to\navoid even looking at the attr_stack when the user has asked for \"foo\"\nand we know that \"foo\" did not ever appear in any .gitattributes file.\n\nIt used a flag \"maybe_real\" in each attribute struct, where \"real\" meant\nthat the attribute appeared in an actual file (we have to make this\ndistinction because we also create an attribute struct for any names\nthat are being queried). But as explained in that commit message, the\nmeaning of \"real\" was tangled with some special cases around macros.\n\nWhen 06a604e670 later refactored the macro code, it dropped maybe_real\nentirely. This missed the fact that \"maybe_real\" could be unset for two\nreasons: because of a macro, or because it was never found during\nparsing. This had two results:\n\n - the optimization in collect_some_attrs() ceased doing anything\n meaningful, since it no longer kept track of \"was it found during\n parsing\"\n\n - worse, it actually kicked in when the caller _did_ ask about a macro\n by name, causing us to mark it as unspecified\n\nIt should be possible to salvage this optimization, but let's start with\njust removing the remnants. It hasn't been doing anything (except\ncreating bugs) since 60a12722ac, and nobody seems to have noticed the\nperformance regression. It's more important to fix the correctness\nproblem clearly first.\n\nI've added two tests here. The second one actually shows off the bug.\nThe test of \"check-attr -a\" is not strictly necessary, but we currently\ndo not test attribute macros much, and the builtin \"binary\" not at all.\nSo this increases our general test coverage, as well as making sure we\ndidn't mess up this related case.\n\nSigned-off-by: Jeff King \n---\n attr.c | 16 +---------------\n t/t0003-attributes.sh | 20 ++++++++++++++++++++\n 2 files changed, 21 insertions(+), 15 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex eaece6658d..57ced792f8 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -1092,7 +1092,7 @@ static void collect_some_attrs(const struct index_state *istate,\n \t\t\t const char *path,\n \t\t\t struct attr_check *check)\n {\n-\tint i, pathlen, rem, dirlen;\n+\tint pathlen, rem, dirlen;\n \tconst char *cp, *last_slash = NULL;\n \tint basename_offset;\n \n@@ -1113,20 +1113,6 @@ static void collect_some_attrs(const struct index_state *istate,\n \tall_attrs_init(&g_attr_hashmap, check);\n \tdetermine_macros(check->all_attrs, check->stack);\n \n-\tif (check->nr) {\n-\t\trem = 0;\n-\t\tfor (i = 0; i < check->nr; i++) {\n-\t\t\tint n = check->items[i].attr->attr_nr;\n-\t\t\tstruct all_attrs_item *item = &check->all_attrs[n];\n-\t\t\tif (item->macro) {\n-\t\t\t\titem->value = ATTR__UNSET;\n-\t\t\t\trem++;\n-\t\t\t}\n-\t\t}\n-\t\tif (rem == check->nr)\n-\t\t\treturn;\n-\t}\n-\n \trem = check->all_attrs_nr;\n \tfill(path, pathlen, basename_offset, check->stack, check->all_attrs, rem);\n }\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 22499bce5f..71e63d8b50 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -322,4 +322,24 @@ test_expect_success 'bare repository: test info/attributes' '\n \t)\n '\n \n+test_expect_success 'binary macro expanded by -a' '\n+\techo \"file binary\" >.gitattributes &&\n+\tcat >expect <<-\\EOF &&\n+\tfile: binary: set\n+\tfile: diff: unset\n+\tfile: merge: unset\n+\tfile: text: unset\n+\tEOF\n+\tgit check-attr -a file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+\n+test_expect_success 'query binary macro directly' '\n+\techo \"file binary\" >.gitattributes &&\n+\techo file: binary: set >expect &&\n+\tgit check-attr binary file >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.20.1.691.ge06e0a624f\n\n"},{"id":"367147","messageId":"20190118214626.GC28808@sigill.intra.peff.net","threadId":"50250","inReplyTo":"20190118213458.GB28808@sigill.intra.peff.net","subject":"Re: [PATCH] attr: do not mark queried macros as unset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-18T21:46:27Z","receivedAt":"2019-01-18T21:46:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 18, 2019 at 04:34:58PM -0500, Jeff King wrote:\n\n> When 06a604e670 later refactored the macro code, it dropped maybe_real\n> entirely. This missed the fact that \"maybe_real\" could be unset for two\n> reasons: because of a macro, or because it was never found during\n> parsing. This had two results:\n> \n> - the optimization in collect_some_attrs() ceased doing anything\n> meaningful, since it no longer kept track of \"was it found during\n> parsing\"\n> \n> - worse, it actually kicked in when the caller _did_ ask about a macro\n> by name, causing us to mark it as unspecified\n> \n> It should be possible to salvage this optimization, but let's start with\n> just removing the remnants. It hasn't been doing anything (except\n> creating bugs) since 60a12722ac, and nobody seems to have noticed the\n> performance regression. It's more important to fix the correctness\n> problem clearly first.\n\nAnd here's a resurrection of the optimization that _seems_ to work, but\nI'm not 100% confident in.\n\nIn particular, it does not care about macros at all. It simply asks: is\nthis queried attribute a thing which was ever mentioned in the\nattributes files (either as a path match or as a possible macro\nexpansion). If not, then we know we do not need to look further for it.\n\nBut that leaves me unsure why the original optimization needed to care\nabout macros at all. Has something changed since then with respect to\nthe way we expand macros since then? Or am I totally missing some case\nthat will cause problems?\n\nI guess maybe what I'm missing is that asking for \"diff\" means that we\nneed to care about:\n\n - whether \"diff\" was mentioned in the stack\n\n - whether \"binary\" was mentioned in the stack\n\nBut just \"binary\" mentioning \"diff\" is not interesting without somebody\nactually mentioning \"binary\". I.e., I don't think the patch here will\nproduce wrong results, but it will not kick in as often as we might\nlike.\n\nI'm not sure how to do it robustly without being able to reverse-map all\nof the macros after we've resolved them (i.e., to know that \"diff\" gets\nmentioned by \"binary\", and then check if \"binary\" is actually\nmentioned). I think that would be possible now, as we should know that\nafter determine_macros(). But I also wonder if we are hitting\ndiminishing returns (after all, determine_macros() is already walking\nthe attr stack).\n\nI dunno. This is why I submitted the initial patch as the simplest fix. ;)\n\n---\ndiff --git a/attr.c b/attr.c\nindex 57ced792f8..c3cbfa6501 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -31,6 +31,7 @@ static const char git_attr__unknown[] = \"(builtin)unknown\";\n \n struct git_attr {\n \tint attr_nr; /* unique attribute number */\n+\tint in_stack; /* actually found in some attribute stack */\n \tchar name[FLEX_ARRAY]; /* attribute name */\n };\n \n@@ -220,7 +221,8 @@ static void report_invalid_attr(const char *name, size_t len,\n * dictionary. If no entry is found, create a new attribute and store it in\n * the dictionary.\n */\n-static const struct git_attr *git_attr_internal(const char *name, int namelen)\n+static const struct git_attr *git_attr_internal(const char *name, int namelen,\n+\t\t\t\t\t\tint in_stack)\n {\n \tstruct git_attr *a;\n \n@@ -240,6 +242,8 @@ static const struct git_attr *git_attr_internal(const char *name, int namelen)\n \t\t (hashmap_get_size(&g_attr_hashmap.map) - 1));\n \t}\n \n+\ta->in_stack |= in_stack;\n+\n \thashmap_unlock(&g_attr_hashmap);\n \n \treturn a;\n@@ -247,7 +251,7 @@ static const struct git_attr *git_attr_internal(const char *name, int namelen)\n \n const struct git_attr *git_attr(const char *name)\n {\n-\treturn git_attr_internal(name, strlen(name));\n+\treturn git_attr_internal(name, strlen(name), 0);\n }\n \n /* What does a matched pattern decide? */\n@@ -335,7 +339,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,\n \t\telse {\n \t\t\te->setto = xmemdupz(equals + 1, ep - equals - 1);\n \t\t}\n-\t\te->attr = git_attr_internal(cp, len);\n+\t\te->attr = git_attr_internal(cp, len, 1);\n \t}\n \treturn ep + strspn(ep, blank);\n }\n@@ -396,7 +400,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,\n \t\t sizeof(struct attr_state) * num_attr +\n \t\t (is_macro ? 0 : namelen + 1));\n \tif (is_macro) {\n-\t\tres->u.attr = git_attr_internal(name, namelen);\n+\t\tres->u.attr = git_attr_internal(name, namelen, 1);\n \t} else {\n \t\tchar *p = (char *)&(res->state[num_attr]);\n \t\tmemcpy(p, name, namelen);\n@@ -1093,6 +1097,7 @@ static void collect_some_attrs(const struct index_state *istate,\n \t\t\t struct attr_check *check)\n {\n \tint pathlen, rem, dirlen;\n+\tint i;\n \tconst char *cp, *last_slash = NULL;\n \tint basename_offset;\n \n@@ -1111,6 +1116,21 @@ static void collect_some_attrs(const struct index_state *istate,\n \n \tprepare_attr_stack(istate, path, dirlen, &check->stack);\n \tall_attrs_init(&g_attr_hashmap, check);\n+\n+\tif (check->nr) {\n+\t\trem = 0;\n+\t\tfor (i = 0; i < check->nr; i++) {\n+\t\t\tint n = check->items[i].attr->attr_nr;\n+\t\t\tstruct all_attrs_item *item = &check->all_attrs[n];\n+\t\t\tif (!item->attr->in_stack) {\n+\t\t\t\titem->value = ATTR__UNSET;\n+\t\t\t\trem++;\n+\t\t\t}\n+\t\t}\n+\t\tif (rem == check->nr)\n+\t\t\treturn;\n+\t}\n+\n \tdetermine_macros(check->all_attrs, check->stack);\n \n \trem = check->all_attrs_nr;\n"},{"id":"367151","messageId":"CAGZ79kaPXQUY=FN3qusc2PNs=o1EiNarcBejOQKiozMSPvEOYw@mail.gmail.com","threadId":"50250","inReplyTo":"20190118214626.GC28808@sigill.intra.peff.net","subject":"Re: [PATCH] attr: do not mark queried macros as unset","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2019-01-18T22:19:55Z","receivedAt":"2019-01-18T22:20:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> I dunno. This is why I submitted the initial patch as the simplest fix. ;)\n>\n\nThe first patch is\nReviewed-by: Stefan Beller \n\nDiffing across both patches, this seems to be the relevant part:\n\n---8<---\n@@ -1111,14 +1116,13 @@ static void collect_some_attrs(const struct\nindex_state *istate,\n\n prepare_attr_stack(istate, path, dirlen, &check->stack);\n all_attrs_init(&g_attr_hashmap, check);\n- determine_macros(check->all_attrs, check->stack);\n\n if (check->nr) {\n rem = 0;\n for (i = 0; i < check->nr; i++) {\n int n = check->items[i].attr->attr_nr;\n struct all_attrs_item *item = &check->all_attrs[n];\n- if (item->macro) {\n+ if (!item->attr->in_stack) {\n item->value = ATTR__UNSET;\n rem++;\n }\n@@ -1127,6 +1131,8 @@ static void collect_some_attrs(const struct\nindex_state *istate,\n return;\n }\n\n+ determine_macros(check->all_attrs, check->stack);\n+\n rem = check->all_attrs_nr;\n fill(path, pathlen, basename_offset, check->stack,\ncheck->all_attrs, rem);\n }\n---8<---\n\nwhich I think is correct.\n\nMaybe we could refactor the big condition (if (check->nr)) to be\nits own function and have\n\n if (!check_overlaps_all_attrs(check))\n return;\n\ninstead. The function would allow for a natural place to put a comment\nconvincing us why the optimisation works as expected. :-)\n\nAnd after rereading that code, the optimisation checks\nif any of the requested attributes in 'check' are touched in\nall_attrs, which sounds like a natural optimisation when we assume\nthat filling in the actual values take a lot of time as the stack\nof attribute files might be large.\n\nI think this patch is correct, too.\n\nStefan\n"},{"id":"367229","messageId":"CACsJy8DFX2P6nF200YV_3VjXiags0W28awbSAwc9ztfEZPbJ4g@mail.gmail.com","threadId":"50250","inReplyTo":"20190118213458.GB28808@sigill.intra.peff.net","subject":"Re: [PATCH] attr: do not mark queried macros as unset","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-21T10:05:56Z","receivedAt":"2019-01-21T10:06:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 19, 2019 at 4:35 AM Jeff King wrote:\n>\n> On Fri, Jan 18, 2019 at 11:58:01AM -0500, Jeff King wrote:\n>\n> > Now, on to the actual bug. The simplest reproduction is:\n> >\n> > (echo \"[attr]foo bar\"; echo \"* foo\") >.gitattributes\n> > git check-attr foo file\n>\n> Actually, even simpler is to just \"binary\", which is pre-defined as a\n> macro. :)\n>\n> > which should report \"foo\" as set. This bisects to 60a12722ac (attr:\n> > remove maybe-real, maybe-macro from git_attr, 2017-01-27), and it seems\n> > like an unintentional regression there. I haven't yet poked into that\n> > commit to see what the fix will look like.\n>\n> So here's the fix I came up with. +cc Duy, as this is really tangled\n> with his older 06a604e670.\n>\n> -- >8 --\n> Subject: [PATCH] attr: do not mark queried macros as unset\n>\n> Since 60a12722ac (attr: remove maybe-real, maybe-macro from git_attr,\n> 2017-01-27), we will always mark an attribute macro (e.g., \"binary\")\n> that is specifically queried for as \"unspecified\", even though listing\n> _all_ attributes would display it at set. E.g.:\n>\n> $ echo \"* binary\" >.gitattributes\n>\n> $ git check-attr -a file\n> file: binary: set\n> file: diff: unset\n> file: merge: unset\n> file: text: unset\n>\n> $ git check-attr binary file\n> file: binary: unspecified\n>\n> The problem stems from an incorrect conversion of the optimization from\n> 06a604e670 (attr: avoid heavy work when we know the specified attr is\n> not defined, 2014-12-28). There we tried in collect_some_attrs() to\n> avoid even looking at the attr_stack when the user has asked for \"foo\"\n> and we know that \"foo\" did not ever appear in any .gitattributes file.\n>\n> It used a flag \"maybe_real\" in each attribute struct, where \"real\" meant\n> that the attribute appeared in an actual file (we have to make this\n> distinction because we also create an attribute struct for any names\n> that are being queried). But as explained in that commit message, the\n> meaning of \"real\" was tangled with some special cases around macros.\n>\n> When 06a604e670 later refactored the macro code, it dropped maybe_real\n> entirely. This missed the fact that \"maybe_real\" could be unset for two\n> reasons: because of a macro, or because it was never found during\n> parsing. This had two results:\n>\n> - the optimization in collect_some_attrs() ceased doing anything\n> meaningful, since it no longer kept track of \"was it found during\n> parsing\"\n>\n> - worse, it actually kicked in when the caller _did_ ask about a macro\n> by name, causing us to mark it as unspecified\n>\n> It should be possible to salvage this optimization, but let's start with\n> just removing the remnants. It hasn't been doing anything (except\n> creating bugs) since 60a12722ac, and nobody seems to have noticed the\n> performance regression. It's more important to fix the correctness\n> problem clearly first.\n\nBut muh optimization!!! You're right of course, correctness comes\nfirst. I did try to look at this code but it's been a while and I'm\nafraid I don't have anything valuable to say. I'll dig in more in the\nnext couple days.\n-- \nDuy\n"},{"id":"367284","messageId":"20190122071921.GC28555@sigill.intra.peff.net","threadId":"50250","inReplyTo":"CAGZ79kaPXQUY=FN3qusc2PNs=o1EiNarcBejOQKiozMSPvEOYw@mail.gmail.com","subject":"Re: [PATCH] attr: do not mark queried macros as unset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-22T07:19:22Z","receivedAt":"2019-01-22T07:19:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 18, 2019 at 02:19:55PM -0800, Stefan Beller wrote:\n\n> > I dunno. This is why I submitted the initial patch as the simplest fix. ;)\n> >\n> \n> The first patch is\n> Reviewed-by: Stefan Beller \n\nThanks!\n\n> Diffing across both patches, this seems to be the relevant part:\n> [...]\n> \n> ---8<---\n> @@ -1111,14 +1116,13 @@ static void collect_some_attrs(const struct\n> index_state *istate,\n> \n> prepare_attr_stack(istate, path, dirlen, &check->stack);\n> all_attrs_init(&g_attr_hashmap, check);\n> - determine_macros(check->all_attrs, check->stack);\n> \n> if (check->nr) {\n> rem = 0;\n> for (i = 0; i < check->nr; i++) {\n> int n = check->items[i].attr->attr_nr;\n> struct all_attrs_item *item = &check->all_attrs[n];\n> - if (item->macro) {\n> + if (!item->attr->in_stack) {\n> item->value = ATTR__UNSET;\n> rem++;\n> }\n> @@ -1127,6 +1131,8 @@ static void collect_some_attrs(const struct\n> index_state *istate,\n> return;\n> }\n> \n> + determine_macros(check->all_attrs, check->stack);\n> +\n> rem = check->all_attrs_nr;\n> fill(path, pathlen, basename_offset, check->stack,\n> check->all_attrs, rem);\n> }\n> ---8<---\n> \n> which I think is correct.\n\nYes, that's the interesting part. I think I've convinced myself, too,\nthat it doesn't do the _wrong_ thing ever. But I think it misses the\npoint of the original, which is that you want common ones like \"diff\"\nnot to trigger in_stack if nobody has actually used them. And doing that\nreally does mean marking in_stack not just when a macro mentions it\n(because clearly \"binary\" is going to mention it for every repo), but\nwaiting to see if anybody mentions that macro.\n\nWhich means we must call determine_macros(), and then propagate the\nmacro's in_stack to its expansion (if it's indeed called at all).\n\nI don't think that would be _too_ hard to do. But I also wonder if\nthere's much point. We are trying to avoid fill(), but I think that\ndetermine_macros() is of roughly the same complexity (look at all\nmatches of all stacks). I guess it does avoid path_matches(), which is a\nbit more expensive. And in theory it could be cached for a particular\nstack top, so the work is amortized across many path lookups (though I\nthink that gets even more tricky).\n\n-Peff\n"},{"id":"367285","messageId":"20190122072111.GD28555@sigill.intra.peff.net","threadId":"50250","inReplyTo":"CACsJy8DFX2P6nF200YV_3VjXiags0W28awbSAwc9ztfEZPbJ4g@mail.gmail.com","subject":"Re: [PATCH] attr: do not mark queried macros as unset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-22T07:21:11Z","receivedAt":"2019-01-22T07:21:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 21, 2019 at 05:05:56PM +0700, Duy Nguyen wrote:\n\n> > It should be possible to salvage this optimization, but let's start with\n> > just removing the remnants. It hasn't been doing anything (except\n> > creating bugs) since 60a12722ac, and nobody seems to have noticed the\n> > performance regression. It's more important to fix the correctness\n> > problem clearly first.\n> \n> But muh optimization!!! You're right of course, correctness comes\n> first. I did try to look at this code but it's been a while and I'm\n> afraid I don't have anything valuable to say. I'll dig in more in the\n> next couple days.\n\n:) See the side-thread, and the response I just wrote to Stefan. At this\npoint I do think it's possible, but my hope is that I could call it\nquits with the bugfix portion and hand it off to somebody interested in\nthis area. I know you're juggling quite a few other series, though.\n\n-Peff\n"},{"id":"367293","messageId":"CACsJy8A2PVY_f7mSLwraroUEtSy8ZgSksAyqqiHgm+HXFC2=uw@mail.gmail.com","threadId":"50250","inReplyTo":"20190118213458.GB28808@sigill.intra.peff.net","subject":"Re: [PATCH] attr: do not mark queried macros as unset","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-22T09:34:45Z","receivedAt":"2019-01-22T09:35:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jan 19, 2019 at 4:35 AM Jeff King wrote:\n>\n> On Fri, Jan 18, 2019 at 11:58:01AM -0500, Jeff King wrote:\n>\n> > Now, on to the actual bug. The simplest reproduction is:\n> >\n> > (echo \"[attr]foo bar\"; echo \"* foo\") >.gitattributes\n> > git check-attr foo file\n>\n> Actually, even simpler is to just \"binary\", which is pre-defined as a\n> macro. :)\n>\n> > which should report \"foo\" as set. This bisects to 60a12722ac (attr:\n> > remove maybe-real, maybe-macro from git_attr, 2017-01-27), and it seems\n> > like an unintentional regression there. I haven't yet poked into that\n> > commit to see what the fix will look like.\n>\n> So here's the fix I came up with. +cc Duy, as this is really tangled\n> with his older 06a604e670.\n>\n> -- >8 --\n> Subject: [PATCH] attr: do not mark queried macros as unset\n>\n> Since 60a12722ac (attr: remove maybe-real, maybe-macro from git_attr,\n> 2017-01-27), we will always mark an attribute macro (e.g., \"binary\")\n> that is specifically queried for as \"unspecified\", even though listing\n> _all_ attributes would display it at set. E.g.:\n>\n> $ echo \"* binary\" >.gitattributes\n>\n> $ git check-attr -a file\n> file: binary: set\n> file: diff: unset\n> file: merge: unset\n> file: text: unset\n>\n> $ git check-attr binary file\n> file: binary: unspecified\n>\n> The problem stems from an incorrect conversion of the optimization from\n> 06a604e670 (attr: avoid heavy work when we know the specified attr is\n> not defined, 2014-12-28). There we tried in collect_some_attrs() to\n> avoid even looking at the attr_stack when the user has asked for \"foo\"\n> and we know that \"foo\" did not ever appear in any .gitattributes file.\n>\n> It used a flag \"maybe_real\" in each attribute struct, where \"real\" meant\n> that the attribute appeared in an actual file (we have to make this\n> distinction because we also create an attribute struct for any names\n> that are being queried). But as explained in that commit message, the\n> meaning of \"real\" was tangled with some special cases around macros.\n>\n> When 06a604e670 later refactored the macro code, it dropped maybe_real\n\n60a12722ac or 06a604e670? I'm guessing the former.\n\n> entirely. This missed the fact that \"maybe_real\" could be unset for two\n> reasons: because of a macro, or because it was never found during\n> parsing. This had two results:\n>\n> - the optimization in collect_some_attrs() ceased doing anything\n> meaningful, since it no longer kept track of \"was it found during\n> parsing\"\n>\n> - worse, it actually kicked in when the caller _did_ ask about a macro\n> by name, causing us to mark it as unspecified\n>\n> It should be possible to salvage this optimization, but let's start with\n> just removing the remnants. It hasn't been doing anything (except\n> creating bugs) since 60a12722ac, and nobody seems to have noticed the\n> performance regression. It's more important to fix the correctness\n> problem clearly first.\n>\n> I've added two tests here. The second one actually shows off the bug.\n> The test of \"check-attr -a\" is not strictly necessary, but we currently\n> do not test attribute macros much, and the builtin \"binary\" not at all.\n> So this increases our general test coverage, as well as making sure we\n> didn't mess up this related case.\n>\n> Signed-off-by: Jeff King \n\nReviewed-by: me.\n\n> ---\n> attr.c | 16 +---------------\n> t/t0003-attributes.sh | 20 ++++++++++++++++++++\n> 2 files changed, 21 insertions(+), 15 deletions(-)\n>\n> diff --git a/attr.c b/attr.c\n> index eaece6658d..57ced792f8 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -1092,7 +1092,7 @@ static void collect_some_attrs(const struct index_state *istate,\n> const char *path,\n> struct attr_check *check)\n> {\n> - int i, pathlen, rem, dirlen;\n> + int pathlen, rem, dirlen;\n> const char *cp, *last_slash = NULL;\n> int basename_offset;\n>\n> @@ -1113,20 +1113,6 @@ static void collect_some_attrs(const struct index_state *istate,\n> all_attrs_init(&g_attr_hashmap, check);\n> determine_macros(check->all_attrs, check->stack);\n>\n> - if (check->nr) {\n> - rem = 0;\n> - for (i = 0; i < check->nr; i++) {\n> - int n = check->items[i].attr->attr_nr;\n> - struct all_attrs_item *item = &check->all_attrs[n];\n> - if (item->macro) {\n> - item->value = ATTR__UNSET;\n> - rem++;\n> - }\n> - }\n> - if (rem == check->nr)\n> - return;\n> - }\n> -\n> rem = check->all_attrs_nr;\n> fill(path, pathlen, basename_offset, check->stack, check->all_attrs, rem);\n> }\n> diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\n> index 22499bce5f..71e63d8b50 100755\n> --- a/t/t0003-attributes.sh\n> +++ b/t/t0003-attributes.sh\n> @@ -322,4 +322,24 @@ test_expect_success 'bare repository: test info/attributes' '\n> )\n> '\n>\n> +test_expect_success 'binary macro expanded by -a' '\n> + echo \"file binary\" >.gitattributes &&\n> + cat >expect <<-\\EOF &&\n> + file: binary: set\n> + file: diff: unset\n> + file: merge: unset\n> + file: text: unset\n> + EOF\n> + git check-attr -a file >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> +\n> +test_expect_success 'query binary macro directly' '\n> + echo \"file binary\" >.gitattributes &&\n> + echo file: binary: set >expect &&\n> + git check-attr binary file >actual &&\n> + test_cmp expect actual\n> +'\n> +\n> test_done\n> --\n> 2.20.1.691.ge06e0a624f\n>\n\n\n-- \nDuy\n"},{"id":"367295","messageId":"CACsJy8ALL5_gHro9jZcSBnfnV01UEJLReCrqi+w727bkqnjUAA@mail.gmail.com","threadId":"50250","inReplyTo":"20190122071921.GC28555@sigill.intra.peff.net","subject":"Re: [PATCH] attr: do not mark queried macros as unset","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-01-22T09:50:30Z","receivedAt":"2019-01-22T09:50:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jan 22, 2019 at 2:19 PM Jeff King wrote:\n> Yes, that's the interesting part. I think I've convinced myself, too,\n> that it doesn't do the _wrong_ thing ever. But I think it misses the\n> point of the original, which is that you want common ones like \"diff\"\n> not to trigger in_stack if nobody has actually used them.\n\nYes. I don't think it matters much when you don't have a lot of\nattributes, but if you do, the cost of lookup will be proportional to\nthe stack's depth even whenever you look up some attribute, even\nthough you don't use it. This makes code that uses attributes just a\ntiny bit slower over time because I think we still add more and more\nattributes.\n\n> And doing that\n> really does mean marking in_stack not just when a macro mentions it\n> (because clearly \"binary\" is going to mention it for every repo), but\n> waiting to see if anybody mentions that macro.\n>\n> Which means we must call determine_macros(), and then propagate the\n> macro's in_stack to its expansion (if it's indeed called at all).\n>\n> I don't think that would be _too_ hard to do. But I also wonder if\n> there's much point. We are trying to avoid fill(), but I think that\n> determine_macros() is of roughly the same complexity (look at all\n> matches of all stacks). I guess it does avoid path_matches(), which is a\n> bit more expensive. And in theory it could be cached for a particular\n> stack top, so the work is amortized across many path lookups (though I\n> think that gets even more tricky).\n\nThere is a comment that got eventually removed in bw/attr, especially\nthe second to last sentence.\n\n-/*\n- * NEEDSWORK: maybe-real, maybe-macro are not property of\n- * an attribute, as it depends on what .gitattributes are\n- * read. Once we introduce per git_attr_check attr_stack\n- * and check_all_attr, the optimization based on them will\n- * become unnecessary and can go away. So is this variable.\n- */\n-static int cannot_trust_maybe_real;\n\nThe promise here is, after we have moved away from global attribute\nstack, we can build custom stacks containing only queried attributes.\nThis makes attribute stacks short (in the best case, empty, which is\nwhat my optimization is for) which means fill time (I think it's\npath_matches() would dominate) becomes shorter in the _general_ case,\nso this optimization \"will become unnecessary\". More importantly the\ntotal number of attributes will not matter since we only look at what\nwe are interested. This makes attribute lookup scale much better in\nthe long run.\n\nThis part, building custom stacks, has not come true yet. But if we\noptimize this code again, I think this is the way forward. Perhaps\nthis could be one of the mini projects for Matthey's students. The\nscope is relatively small, and optimization is always fun.\n-- \nDuy\n"},{"id":"367383","messageId":"xmqqlg3ce545.fsf@gitster-ct.c.googlers.com","threadId":"50250","inReplyTo":"20190118213458.GB28808@sigill.intra.peff.net","subject":"Re: [PATCH] attr: do not mark queried macros as unset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-22T21:48:10Z","receivedAt":"2019-01-22T21:48:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Jan 18, 2019 at 11:58:01AM -0500, Jeff King wrote:\n>\n>> Now, on to the actual bug. The simplest reproduction is:\n>> \n>> (echo \"[attr]foo bar\"; echo \"* foo\") >.gitattributes\n>> git check-attr foo file\n>\n> Actually, even simpler is to just \"binary\", which is pre-defined as a\n> macro. :)\n>\n>> which should report \"foo\" as set. This bisects to 60a12722ac (attr:\n>> remove maybe-real, maybe-macro from git_attr, 2017-01-27), and it seems\n>> like an unintentional regression there. I haven't yet poked into that\n>> commit to see what the fix will look like.\n>\n> So here's the fix I came up with. +cc Duy, as this is really tangled\n> with his older 06a604e670.\n>\n> -- >8 --\n> Subject: [PATCH] attr: do not mark queried macros as unset\n>\n> Since 60a12722ac (attr: remove maybe-real, maybe-macro from git_attr,\n> 2017-01-27), we will always mark an attribute macro (e.g., \"binary\")\n> that is specifically queried for as \"unspecified\", even though listing\n> _all_ attributes would display it at set. E.g.:\n>\n> $ echo \"* binary\" >.gitattributes\n>\n> $ git check-attr -a file\n> file: binary: set\n> file: diff: unset\n> file: merge: unset\n> file: text: unset\n>\n> $ git check-attr binary file\n> file: binary: unspecified\n>\n> The problem stems from an incorrect conversion of the optimization from\n> 06a604e670 (attr: avoid heavy work when we know the specified attr is\n> not defined, 2014-12-28). There we tried in collect_some_attrs() to\n> avoid even looking at the attr_stack when the user has asked for \"foo\"\n> and we know that \"foo\" did not ever appear in any .gitattributes file.\n>\n> It used a flag \"maybe_real\" in each attribute struct, where \"real\" meant\n> that the attribute appeared in an actual file (we have to make this\n> distinction because we also create an attribute struct for any names\n> that are being queried). But as explained in that commit message, the\n> meaning of \"real\" was tangled with some special cases around macros.\n>\n> When 06a604e670 later refactored the macro code, it dropped maybe_real\n\nI think 60a12722ac is what you meant here.\n\n> entirely. This missed the fact that \"maybe_real\" could be unset for two\n> reasons: because of a macro, or because it was never found during\n> parsing. This had two results:\n>\n> - the optimization in collect_some_attrs() ceased doing anything\n> meaningful, since it no longer kept track of \"was it found during\n> parsing\"\n>\n> - worse, it actually kicked in when the caller _did_ ask about a macro\n> by name, causing us to mark it as unspecified\n\n"},{"id":"367384","messageId":"xmqqef94e4kc.fsf@gitster-ct.c.googlers.com","threadId":"50250","inReplyTo":"20190118214626.GC28808@sigill.intra.peff.net","subject":"Re: [PATCH] attr: do not mark queried macros as unset","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-01-22T22:00:03Z","receivedAt":"2019-01-22T22:00:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> And here's a resurrection of the optimization that _seems_ to work, but\n> I'm not 100% confident in.\n>\n> In particular, it does not care about macros at all. It simply asks: is\n> this queried attribute a thing which was ever mentioned in the\n> attributes files (either as a path match or as a possible macro\n> expansion). If not, then we know we do not need to look further for it.\n\nSo, if we are looking for 'diff' and we know no .gitattributes (or\n$GIT_DIR/info/attributes) entry for 'diff' or any macro that expands\nto touch 'diff' (e.g. 'binary') is in use, we know for any path\ngoverned by the current attr-stack 'diff' attribute is unspecified.\nBut if we see an entry, say, \"*.exe binary\", then we do need to be\naware of the possibility that 'diff' may be unset for some paths.\n\nMakes sense.\n\n> I guess maybe what I'm missing is that asking for \"diff\" means that we\n> need to care about:\n>\n> - whether \"diff\" was mentioned in the stack\n>\n> - whether \"binary\" was mentioned in the stack\n>\n> But just \"binary\" mentioning \"diff\" is not interesting without somebody\n> actually mentioning \"binary\".\n\nYeah, that matches my understanding (which mostly comes from the\noriginal design before even Duy's optimization).\n\n> I'm not sure how to do it robustly without being able to reverse-map all\n> of the macros after we've resolved them (i.e., to know that \"diff\" gets\n> mentioned by \"binary\", and then check if \"binary\" is actually\n> mentioned). I think that would be possible now, as we should know that\n> after determine_macros(). But I also wonder if we are hitting\n> diminishing returns (after all, determine_macros() is already walking\n> the attr stack).\n"},{"id":"367403","messageId":"20190123054026.GA19443@sigill.intra.peff.net","threadId":"50250","inReplyTo":"xmqqlg3ce545.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] attr: do not mark queried macros as unset","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-01-23T05:40:26Z","receivedAt":"2019-01-23T05:40:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 22, 2019 at 01:48:10PM -0800, Junio C Hamano wrote:\n\n> > When 06a604e670 later refactored the macro code, it dropped maybe_real\n> \n> I think 60a12722ac is what you meant here.\n\nYes indeed, too many 6's, 0's, and a's. :) I see you marked it up when\nyou picked up the patch. Thanks.\n\nI should also have added:\n\n Reported-by: Sérgio Peixoto \n\nto the original, if it's not too late.\n\n-Peff\n"}]}