{"thread":{"id":"29330","subject":"[BUG] gitattribute macro expansion oddity","startedAt":"2012-01-10T07:03:01Z","lastAt":"2012-01-11T04:37:59Z","messageCount":13,"participants":["Jeff King","Michael Haggerty","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"182216","messageId":"20120110070300.GA17086@sigill.intra.peff.net","threadId":"29330","inReplyTo":null,"subject":"[BUG] gitattribute macro expansion oddity","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-10T07:03:01Z","receivedAt":"2012-01-10T07:03:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I'm seeing some very odd behavior with git's attribute expansion for\ndiffs. You can see it with this repository:\n\n  git clone git://github.com/libgit2/libgit2sharp.git\n\nTry a diff of a non-binary file:\n\n  $ git show 2a0f4bf7 LibGit2Sharp/Configuration.cs\n  ...\n  diff --git a/LibGit2Sharp/Configuration.cs b/LibGit2Sharp/Configuration.cs\n  index 83cc9d6..9ab0b60 100644\n  --- a/LibGit2Sharp/Configuration.cs\n  +++ b/LibGit2Sharp/Configuration.cs\n\nLooks OK. Now try a diff that also has a binary file (that is marked\nsuch via gitattributes):\n\n  $ git show 2a0f4bf7 Lib/NativeBinaries/x86/git2.dll \\\n                      LibGit2Sharp/Configuration.cs\n  ...\n  diff --git a/Lib/NativeBinaries/x86/git2.dll b/Lib/NativeBinaries/x86/git2.dll\n  index dab0d04..8de18ab 100644\n  Binary files a/Lib/NativeBinaries/x86/git2.dll and b/Lib/NativeBinaries/x86/git2.dll differ\n  diff --git a/LibGit2Sharp/Configuration.cs b/LibGit2Sharp/Configuration.cs\n  index 83cc9d6..9ab0b60 100644\n  Binary files a/LibGit2Sharp/Configuration.cs and b/LibGit2Sharp/Configuration.cs differ\n\nNow the Configuration.cs blobs appear binary!\n\nIt has nothing to do with pathspecs; if you do a non-limited diff of\n2a0f4bf7, you'll see many of the files appear as binary. Running it\nthrough the debugger, it looks like we are getting wrong diff attribute\nvalues for later paths, as if the earlier lookups are somehow polluting\nthe attribute stack.\n\nThe gitattributes in this repository look reasonably sane, but even if\nthey were not, nothing should make a file have different attributes\nbased on other files that were diffed.\n\nBisection points to ec775c4 (attr: Expand macros immediately when\nencountered., 2010-04-06), but it's too late for me to dig further\ntonight. Cc'ing Junio as the author of the attr code and Henrik as the\nauthor of ec775c4.\n\n-Peff\n"},{"id":"182218","messageId":"4F0BFE6E.6080904@alum.mit.edu","threadId":"29330","inReplyTo":"20120110070300.GA17086@sigill.intra.peff.net","subject":"Re: [BUG] gitattribute macro expansion oddity","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-01-10T09:01:34Z","receivedAt":"2012-01-10T09:01:34Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/10/2012 08:03 AM, Jeff King wrote:\n> I'm seeing some very odd behavior with git's attribute expansion for\n> diffs. You can see it with this repository:\n> \n>   git clone git://github.com/libgit2/libgit2sharp.git\n> \n> Try a diff of a non-binary file: [...]\n\nThe problem has nothing with diffing; simply interrogating the attribute\nvalues gives different results depending on the order of the files:\n\n$ git check-attr --all Lib/NativeBinaries/x86/git2.dll\nLibGit2Sharp/Configuration.cs\nLib/NativeBinaries/x86/git2.dll: binary: set\nLib/NativeBinaries/x86/git2.dll: diff: unset\nLib/NativeBinaries/x86/git2.dll: text: unset\nLibGit2Sharp/Configuration.cs: binary: set\nLibGit2Sharp/Configuration.cs: diff: unset\nLibGit2Sharp/Configuration.cs: text: unset\nLibGit2Sharp/Configuration.cs: crlf: set\n$ git check-attr --all LibGit2Sharp/Configuration.cs\nLib/NativeBinaries/x86/git2.dll\nLibGit2Sharp/Configuration.cs: diff: csharp\nLibGit2Sharp/Configuration.cs: crlf: set\nLib/NativeBinaries/x86/git2.dll: binary: set\nLib/NativeBinaries/x86/git2.dll: diff: unset\nLib/NativeBinaries/x86/git2.dll: text: unset\n\nIt also doesn't depend on the fact that Lib/.gitattributes uses CRLF as\nits EOL, nor does it depend on the use of the \"binary\" macro.  However,\nit does depend on the fact that the directory name \"Lib\" matches the\nfirst part of the directory name \"LibGit2Sharp\".  Here is a simplified\ndemonstration of the problem:\n\na=LibA/a.txt\nb=Lib/b.bin\nrm -rf foo\ngit init foo\ncd foo\nmkdir $(dirname $a) $(dirname $b)\ntouch $a $b\necho '*.txt foo' >.gitattributes\necho '* bar' >$(dirname $b)/.gitattributes\ngit add .\ngit commit -am 'Demonstrate problem'\necho '================================================='\ngit check-attr --all $b $a\necho '================================================='\ngit check-attr --all $a $b\necho '================================================='\n\nThe attributes of $a are different depending on what order $a and $b\nappear in the \"git check-attr\" command line.\n\nChanging the example to \"a=foo/a.txt\" makes the problem go away.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"182255","messageId":"20120110171100.GA18962@sigill.intra.peff.net","threadId":"29330","inReplyTo":"4F0BFE6E.6080904@alum.mit.edu","subject":"Re: [BUG] gitattribute macro expansion oddity","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-10T17:11:00Z","receivedAt":"2012-01-10T17:11:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 10, 2012 at 10:01:34AM +0100, Michael Haggerty wrote:\n\n> On 01/10/2012 08:03 AM, Jeff King wrote:\n> > I'm seeing some very odd behavior with git's attribute expansion for\n> > diffs. You can see it with this repository:\n> > \n> >   git clone git://github.com/libgit2/libgit2sharp.git\n> > \n> > Try a diff of a non-binary file: [...]\n> \n> The problem has nothing with diffing; simply interrogating the attribute\n> values gives different results depending on the order of the files:\n> \n> $ git check-attr --all Lib/NativeBinaries/x86/git2.dll\n> LibGit2Sharp/Configuration.cs\n> Lib/NativeBinaries/x86/git2.dll: binary: set\n> Lib/NativeBinaries/x86/git2.dll: diff: unset\n> Lib/NativeBinaries/x86/git2.dll: text: unset\n> LibGit2Sharp/Configuration.cs: binary: set\n> LibGit2Sharp/Configuration.cs: diff: unset\n> LibGit2Sharp/Configuration.cs: text: unset\n> LibGit2Sharp/Configuration.cs: crlf: set\n> $ git check-attr --all LibGit2Sharp/Configuration.cs\n> Lib/NativeBinaries/x86/git2.dll\n> LibGit2Sharp/Configuration.cs: diff: csharp\n> LibGit2Sharp/Configuration.cs: crlf: set\n> Lib/NativeBinaries/x86/git2.dll: binary: set\n> Lib/NativeBinaries/x86/git2.dll: diff: unset\n> Lib/NativeBinaries/x86/git2.dll: text: unset\n\nThanks. I tried to test it with check-attr but for some reason wasn't\nable to provoke the bug (I think I probably just screwed up the\ninvocation).\n\n> It also doesn't depend on the fact that Lib/.gitattributes uses CRLF as\n> its EOL, nor does it depend on the use of the \"binary\" macro.  However,\n> it does depend on the fact that the directory name \"Lib\" matches the\n> first part of the directory name \"LibGit2Sharp\".  Here is a simplified\n> demonstration of the problem:\n\nAh, very helpful. That allowed me to find the problem very quickly by\ngrepping for \"strncmp\". :)\n\nThe patch below seem to fix it for me. I'll do a bit more testing before\nposting it for real, though.\n\n-Peff\n\ndiff --git a/attr.c b/attr.c\nindex 7467baf..f4beb62 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -528,7 +528,8 @@ static void prepare_attr_stack(const char *path, int dirlen)\n \n \t\telem = attr_stack;\n \t\tif (namelen <= dirlen &&\n-\t\t    !strncmp(elem->origin, path, namelen))\n+\t\t    !strncmp(elem->origin, path, namelen) &&\n+\t\t    (!namelen || path[namelen] == '/' || path[namelen] == '\\0'))\n \t\t\tbreak;\n \n \t\tdebug_pop(elem);\n"},{"id":"182256","messageId":"7v1ur7bhhe.fsf@alter.siamese.dyndns.org","threadId":"29330","inReplyTo":"20120110070300.GA17086@sigill.intra.peff.net","subject":"Re: [BUG] gitattribute macro expansion oddity","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-10T17:22:05Z","receivedAt":"2012-01-10T17:22:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Bisection points to ec775c4 (attr: Expand macros immediately when\n> encountered., 2010-04-06), but it's too late for me to dig further\n> tonight. Cc'ing Junio as the author of the attr code and Henrik as the\n> author of ec775c4.\n\nThanks for getting the ball rolling.\n\nRegardless of this unrelated regression, after looking at what ec775c4\nwanted to do again, I am very much tempted to just revert it.\n\nIt aimed to take these three\n\n        *       ident\n        foo     mybin\n        bar     mybin ident\n\nand wanted to omit 'ident' from \"foo\" when there is this macro definition\nelsewhere:\n\n\t[attr] mybin binary -ident\n\nBut the real point of the macro was that the users do not have to know\ntheir internals, iow, if you explicitly specify a pattern that overrides\nthe contents of the macro, that explicit pattern should win. When deciding\nthe value of \"ident\" attribute for path \"foo\", \"* ident\" is stronger than\n\"foo mybin\" (the latter of which does not say anything about 'ident'\nexplicitly).\n"},{"id":"182259","messageId":"20120110180820.GA15273@sigill.intra.peff.net","threadId":"29330","inReplyTo":"20120110171100.GA18962@sigill.intra.peff.net","subject":"[PATCH] attr: don't confuse prefixes with leading directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-10T18:08:21Z","receivedAt":"2012-01-10T18:08:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we prepare the attribute stack for a lookup on a path,\nwe start with the cached stack from the previous lookup\n(because it is common to do several lookups in the same\ndirectory hierarchy). So the first thing we must do in\npreparing the stack is to pop any entries that point to\ndirectories we are no longer interested in.\n\nFor example, if our stack contains gitattributes for:\n\n  foo/bar/baz\n  foo/bar\n  foo\n\nbut we want to do a lookup in \"foo/bar/bleep\", then we want\nto pop the top element, but retain the others.\n\nTo do this we walk down the stack from the top, popping\nelements that do not match our lookup directory. However,\nthe test do this simply checked strncmp, meaning we would\nmistake \"foo/bar/baz\" as a leading directory of\n\"foo/bar/baz_plus\". We must also check that the character\nafter our match is '/', meaning we matched the whole path\ncomponent.\n\nThere are two special cases to consider:\n\n  1. The top of our attr stack has the empty path. So we\n     must not check for '/', but rather special-case the\n     empty path, which always matches.\n\n  2. Typically when matching paths in this way, you would\n     also need to check for a full string match (i.e., the\n     character after is '\\0'). We don't need to do so in\n     this case, though, because our path string is actually\n     just the directory component of the path to a file\n     (i.e., we know that it terminates with \"/\", because the\n     filename comes after that).\n\nHelped-by: Michael Haggerty <mhagger@alum.mit.edu>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI wrote it in the minimally-intrusive way, but you could also pull\nthe \"!namelen\" case before the strncmp (since a strncmp of size 0 is\nalways true, anyway). I don't know if it would be more obvious that way.\n\nI prepared this on top of master, but the patch applies (with some\nshifted line counts) on older releases, too.\n\n attr.c                |    3 ++-\n t/t0003-attributes.sh |   10 ++++++++++\n 2 files changed, 12 insertions(+), 1 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 76b079f..fa975da 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -582,7 +582,8 @@ static void prepare_attr_stack(const char *path)\n \n \t\telem = attr_stack;\n \t\tif (namelen <= dirlen &&\n-\t\t    !strncmp(elem->origin, path, namelen))\n+\t\t    !strncmp(elem->origin, path, namelen) &&\n+\t\t    (!namelen || path[namelen] == '/'))\n \t\t\tbreak;\n \n \t\tdebug_pop(elem);\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex dbb2623..51f3045 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -159,6 +159,16 @@ test_expect_success 'relative paths' '\n \t(cd b && attr_check ../a/b/g a/b/g)\n '\n \n+test_expect_success 'prefixes are not confused with leading directories' '\n+\tattr_check a_plus/g unspecified &&\n+\tcat >expect <<-\\EOF &&\n+\ta/g: test: a/g\n+\ta_plus/g: test: unspecified\n+\tEOF\n+\tgit check-attr test a/g a_plus/g >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'core.attributesfile' '\n \tattr_check global unspecified &&\n \tgit config core.attributesfile \"$HOME/global-gitattributes\" &&\n-- \n1.7.9.rc0.33.gd3c17\n"},{"id":"182260","messageId":"20120110182140.GB15273@sigill.intra.peff.net","threadId":"29330","inReplyTo":"20120110180820.GA15273@sigill.intra.peff.net","subject":"Re: [PATCH] attr: don't confuse prefixes with leading directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-10T18:21:40Z","receivedAt":"2012-01-10T18:21:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 10, 2012 at 01:08:21PM -0500, Jeff King wrote:\n\n> diff --git a/attr.c b/attr.c\n> index 76b079f..fa975da 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -582,7 +582,8 @@ static void prepare_attr_stack(const char *path)\n>  \n>  \t\telem = attr_stack;\n>  \t\tif (namelen <= dirlen &&\n> -\t\t    !strncmp(elem->origin, path, namelen))\n> +\t\t    !strncmp(elem->origin, path, namelen) &&\n> +\t\t    (!namelen || path[namelen] == '/'))\n>  \t\t\tbreak;\n\nSide note. One thing that confused me about this code is that\nprepare_attr_stack does a popping loop like this:\n\n  while (attr_stack && attr_stack->origin) {\n          if (/* attr_stack->origin is a prefix */)\n                  break;\n          /* otherwise, pop */\n          elem = attr_stack;\n          attr_stack = elem->prev;\n          free(elem);\n  }\n\n  /* now push our new ones */\n  ...\n  len = strlen(attr_stack->origin);\n\nIOW, our loop breaks out when attr_stack is NULL, but then we go on to\nassume that attr_stack is _not_ NULL. This isn't a bug, because it turns\nout that we always leave something in the attr_stack: the root\ngitattributes file (and the builtins).  But it is slightly confusing to\na reader because of the useless loop condition.\n\nI'm not sure if the right solution is to change the popping loop to:\n\n  /* we will never run out of stack, because we always have the root */\n  while (attr_stack->origin) {\n          ...\n\nOr to be extra defensive and put:\n\n  if (!attr_stack)\n          die(\"BUG: we ran out of attr stack!?\");\n\nafter the loop, or to somehow handle the case of an empty attr stack\nbelow (which is hard to do, because it can't be triggered, so I have no\nidea what it would mean).\n\n-Peff\n"},{"id":"182268","messageId":"7vlipf9xbe.fsf@alter.siamese.dyndns.org","threadId":"29330","inReplyTo":"20120110182140.GB15273@sigill.intra.peff.net","subject":"Re: [PATCH] attr: don't confuse prefixes with leading directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-10T19:23:01Z","receivedAt":"2012-01-10T19:23:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> IOW, our loop breaks out when attr_stack is NULL, but then we go on to\n> assume that attr_stack is _not_ NULL. This isn't a bug, because it turns\n> out that we always leave something in the attr_stack: the root\n> gitattributes file (and the builtins).  But it is slightly confusing to\n> a reader because of the useless loop condition.\n> \n> I'm not sure if the right solution is to change the popping loop to:\n>\n>   /* we will never run out of stack, because we always have the root */\n>   while (attr_stack->origin) {\n>           ...\n\nYeah, that makes sense, as that existing check \"attr_stack &&\" was a\nmisguided defensive coding, that was _not_ defensive at all as we didn't\ndo anything after we stop iterating from that loop and without checking\ndereferenced attr_stack->origin, which was a simple bogosity.\n\n>\n> Or to be extra defensive and put:\n>\n>   if (!attr_stack)\n>           die(\"BUG: we ran out of attr stack!?\");\n>\n> after the loop, or to somehow handle the case of an empty attr stack\n> below (which is hard to do, because it can't be triggered, so I have no\n> idea what it would mean).\n\nAnd this is even more so.\n"},{"id":"182269","messageId":"7vhb039x7k.fsf@alter.siamese.dyndns.org","threadId":"29330","inReplyTo":"20120110180820.GA15273@sigill.intra.peff.net","subject":"Re: [PATCH] attr: don't confuse prefixes with leading directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-10T19:25:19Z","receivedAt":"2012-01-10T19:25:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/attr.c b/attr.c\n> index 76b079f..fa975da 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -582,7 +582,8 @@ static void prepare_attr_stack(const char *path)\n>  \n>  \t\telem = attr_stack;\n>  \t\tif (namelen <= dirlen &&\n> -\t\t    !strncmp(elem->origin, path, namelen))\n> +\t\t    !strncmp(elem->origin, path, namelen) &&\n> +\t\t    (!namelen || path[namelen] == '/'))\n>  \t\t\tbreak;\n\nThanks for the fix; I was looking at path_matches() and wondering about\nthe same thing.\n"},{"id":"182270","messageId":"20120110192810.GA16018@sigill.intra.peff.net","threadId":"29330","inReplyTo":"7vlipf9xbe.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] attr: don't confuse prefixes with leading directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-10T19:28:10Z","receivedAt":"2012-01-10T19:28:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 10, 2012 at 11:23:01AM -0800, Junio C Hamano wrote:\n\n> > I'm not sure if the right solution is to change the popping loop to:\n> >\n> >   /* we will never run out of stack, because we always have the root */\n> >   while (attr_stack->origin) {\n> >           ...\n> \n> Yeah, that makes sense, as that existing check \"attr_stack &&\" was a\n> misguided defensive coding, that was _not_ defensive at all as we didn't\n> do anything after we stop iterating from that loop and without checking\n> dereferenced attr_stack->origin, which was a simple bogosity.\n> \n> >\n> > Or to be extra defensive and put:\n> >\n> >   if (!attr_stack)\n> >           die(\"BUG: we ran out of attr stack!?\");\n> >\n> > after the loop, or to somehow handle the case of an empty attr stack\n> > below (which is hard to do, because it can't be triggered, so I have no\n> > idea what it would mean).\n> \n> And this is even more so.\n\nI wasn't clear: the second one is \"even more so\" making sense, or \"even\nmore so\" misguided defensive coding?\n\n-Peff\n"},{"id":"182271","messageId":"20120110193206.GA16826@sigill.intra.peff.net","threadId":"29330","inReplyTo":"20120110192810.GA16018@sigill.intra.peff.net","subject":"Re: [PATCH] attr: don't confuse prefixes with leading directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-10T19:32:06Z","receivedAt":"2012-01-10T19:32:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 10, 2012 at 02:28:10PM -0500, Jeff King wrote:\n\n> On Tue, Jan 10, 2012 at 11:23:01AM -0800, Junio C Hamano wrote:\n> \n> > > I'm not sure if the right solution is to change the popping loop to:\n> > >\n> > >   /* we will never run out of stack, because we always have the root */\n> > >   while (attr_stack->origin) {\n> > >           ...\n> > \n> > Yeah, that makes sense, as that existing check \"attr_stack &&\" was a\n> > misguided defensive coding, that was _not_ defensive at all as we didn't\n> > do anything after we stop iterating from that loop and without checking\n> > dereferenced attr_stack->origin, which was a simple bogosity.\n> > \n> > >\n> > > Or to be extra defensive and put:\n> > >\n> > >   if (!attr_stack)\n> > >           die(\"BUG: we ran out of attr stack!?\");\n> > >\n> > > after the loop, or to somehow handle the case of an empty attr stack\n> > > below (which is hard to do, because it can't be triggered, so I have no\n> > > idea what it would mean).\n> > \n> > And this is even more so.\n> \n> I wasn't clear: the second one is \"even more so\" making sense, or \"even\n> more so\" misguided defensive coding?\n\nIf the latter, then I think we want this:\n\n-- >8 --\nSubject: [PATCH] attr: drop misguided defensive coding\n\nIn prepare_attr_stack, we pop the old elements of the stack\n(which were left from a previous lookup and may or may not\nbe useful to us). Our loop to do so checks that we never\nreach the top of the stack. However, the code immediately\nafterwards will segfault if we did actually reach the top of\nthe stack.\n\nFortunately, this is not an actual bug, since we will never\npop all of the stack elements (we will always keep the root\ngitattributes, as well as the builtin ones). So the extra\ncheck in the loop condition simply clutters the code and\nmakes the intent less clear. Let's get rid of it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n attr.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex fa975da..cf8f2bc 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -577,7 +577,7 @@ static void prepare_attr_stack(const char *path)\n \t * Pop the ones from directories that are not the prefix of\n \t * the path we are checking.\n \t */\n-\twhile (attr_stack && attr_stack->origin) {\n+\twhile (attr_stack->origin) {\n \t\tint namelen = strlen(attr_stack->origin);\n \n \t\telem = attr_stack;\n-- \n1.7.9.rc0.33.gd3c17\n"},{"id":"182277","messageId":"7v7h0z9ufu.fsf@alter.siamese.dyndns.org","threadId":"29330","inReplyTo":"20120110192810.GA16018@sigill.intra.peff.net","subject":"Re: [PATCH] attr: don't confuse prefixes with leading directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-10T20:25:09Z","receivedAt":"2012-01-10T20:25:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Jan 10, 2012 at 11:23:01AM -0800, Junio C Hamano wrote:\n>\n>> > I'm not sure if the right solution is to change the popping loop to:\n>> >\n>> >   /* we will never run out of stack, because we always have the root */\n>> >   while (attr_stack->origin) {\n>> >           ...\n>> \n>> Yeah, that makes sense, as that existing check \"attr_stack &&\" was a\n>> misguided defensive coding, that was _not_ defensive at all as we didn't\n>> do anything after we stop iterating from that loop and without checking\n>> dereferenced attr_stack->origin, which was a simple bogosity.\n>> \n>> >\n>> > Or to be extra defensive and put:\n>> >\n>> >   if (!attr_stack)\n>> >           die(\"BUG: we ran out of attr stack!?\");\n>> >\n>> > after the loop, or to somehow handle the case of an empty attr stack\n>> > below (which is hard to do, because it can't be triggered, so I have no\n>> > idea what it would mean).\n>> \n>> And this is even more so.\n>\n> I wasn't clear: the second one is \"even more so\" making sense, or \"even\n> more so\" misguided defensive coding?\n\nSorry for sending a half-baked response.  The initial draft of my response\nhad just \"that makes sense\" and nothing else in the first paragraph.\n\nIf the original meant to be defensive, it should have had your \"extra\ndefensive\" die(), but it didn't.\n\nBut the condition to break out of that loop is either we hit an elem that\nsatisfy the condition (in which case that elem cannot be NULL) or we\nsuccessfully saw that attr_stack->origin is NULL (in which case attr_stack\ncouldn't have been NULL), so it is pointless to check the NULLness of\nattr_stack itself. Assertion _before_ going into the while loop might make\nsense, but if we look at what bootstrap_attr_stack() does, it should be\npretty obvious that that cannot happen.\n\nAn assert(attr_stack->origin) before we use it may make sense, though, in\norder to make sure we do not mistakenly pop the root one and expose the\nbuilt-in ones whose origin are set to NULL.\n\ndiff --git a/attr.c b/attr.c\nindex ad7eb9c..4d3b61a 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -566,7 +567,9 @@ static void prepare_attr_stack(const char *path, int dirlen)\n \n \t/*\n \t * Pop the ones from directories that are not the prefix of\n-\t * the path we are checking.\n+\t * the path we are checking. Break out of the loop when we see\n+\t * the root one (whose origin is an empty string \"\") or the builtin\n+\t * one (whose origin is NULL) without popping it.\n \t */\n \twhile (attr_stack->origin) {\n \t\tint namelen = strlen(attr_stack->origin);\n@@ -586,6 +589,13 @@ static void prepare_attr_stack(const char *path, int dirlen)\n \t * Read from parent directories and push them down\n \t */\n \tif (!is_bare_repository() || direction == GIT_ATTR_INDEX) {\n+\t\t/*\n+\t\t * bootstrap_attr_stack() should have added, and the\n+\t\t * above loop should have stopped before popping, the\n+\t\t * root element whose attr_stack->origin is set to an\n+\t\t * empty string.\n+\t\t */ \n+\t\tassert(attr_stack->origin);\n \t\twhile (1) {\n \t\t\tchar *cp;\n \n"},{"id":"182286","messageId":"20120110223104.GA28592@sigill.intra.peff.net","threadId":"29330","inReplyTo":"7v7h0z9ufu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] attr: don't confuse prefixes with leading directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-10T22:31:04Z","receivedAt":"2012-01-10T22:31:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 10, 2012 at 12:25:09PM -0800, Junio C Hamano wrote:\n\n> Sorry for sending a half-baked response.  The initial draft of my response\n> had just \"that makes sense\" and nothing else in the first paragraph.\n> \n> If the original meant to be defensive, it should have had your \"extra\n> defensive\" die(), but it didn't.\n> [...]\n> diff --git a/attr.c b/attr.c\n> index ad7eb9c..4d3b61a 100644\n> --- a/attr.c\n> +++ b/attr.c\n> @@ -566,7 +567,9 @@ static void prepare_attr_stack(const char *path, int dirlen)\n>  \n>  \t/*\n>  \t * Pop the ones from directories that are not the prefix of\n> -\t * the path we are checking.\n> +\t * the path we are checking. Break out of the loop when we see\n> +\t * the root one (whose origin is an empty string \"\") or the builtin\n> +\t * one (whose origin is NULL) without popping it.\n>  \t */\n>  \twhile (attr_stack->origin) {\n>  \t\tint namelen = strlen(attr_stack->origin);\n> @@ -586,6 +589,13 @@ static void prepare_attr_stack(const char *path, int dirlen)\n>  \t * Read from parent directories and push them down\n>  \t */\n>  \tif (!is_bare_repository() || direction == GIT_ATTR_INDEX) {\n> +\t\t/*\n> +\t\t * bootstrap_attr_stack() should have added, and the\n> +\t\t * above loop should have stopped before popping, the\n> +\t\t * root element whose attr_stack->origin is set to an\n> +\t\t * empty string.\n> +\t\t */ \n> +\t\tassert(attr_stack->origin);\n>  \t\twhile (1) {\n>  \t\t\tchar *cp;\n\nYeah, this version looks good to me (though I thought we usually spelled\nassert as die(\"BUG: ...\")).\n\n-Peff\n"},{"id":"182312","messageId":"4F0D1227.5020509@alum.mit.edu","threadId":"29330","inReplyTo":"7v1ur7bhhe.fsf@alter.siamese.dyndns.org","subject":"Re: [BUG] gitattribute macro expansion oddity","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2012-01-11T04:37:59Z","receivedAt":"2012-01-11T04:37:59Z","isPatch":false,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 01/10/2012 06:22 PM, Junio C Hamano wrote:\n> Regardless of this unrelated regression, after looking at what ec775c4\n> wanted to do again, I am very much tempted to just revert it.\n> \n> It aimed to take these three\n> \n>         *       ident\n>         foo     mybin\n>         bar     mybin ident\n> \n> and wanted to omit 'ident' from \"foo\" when there is this macro definition\n> elsewhere:\n> \n> \t[attr] mybin binary -ident\n> \n> But the real point of the macro was that the users do not have to know\n> their internals, iow, if you explicitly specify a pattern that overrides\n> the contents of the macro, that explicit pattern should win. When deciding\n> the value of \"ident\" attribute for path \"foo\", \"* ident\" is stronger than\n> \"foo mybin\" (the latter of which does not say anything about 'ident'\n> explicitly).\n\nI like the simplicity of the rule \"apply attributes in the order found\nin the .gitattributes files\" better than the rule you are proposing,\nwhich seems like it will become more complicated to explain.\n\nFor example, it would seem under your rule for the above example that\nthe \"mybin\" macro should bestow on file foo the \"binary\" attribute and\nalso the \"mybin\" attribute (given that macros are themselves\nattributes), but not \"-ident\".\n\nYou would also have to decide and explain whether a macro that invokes a\nmacro that sets or clears attribute \"foo\" is \"weaker\" than a simple\nmacro that clears or sets attribute \"foo\".\n\nI have one real-life use case that would become more difficult with your\nrule:\n\n# Marker for textlike files whose EOL characters haven't been\n# normalized yet:\n[attr]eol-fixme -text !eol\n\n*.cc text eol=lf\n\n\n# Then later, perhaps in some subdirectory's .gitattributes:\nSomeParticularScrewedUpFile.cc eol-fixme\n\n\nThe point of the eol-fixme macro is (1) to prevent git from throwing a\ntantrum and (2) to mark the file as needing cleanup sometime in the future.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"}]}