[BUG] gitattribute macro expansion oddity

I'm seeing some very odd behavior with git's attribute expansion for diffs. You can see it with this repository:

  git clone git://github.com/libgit2/libgit2sharp.git

Try a diff of a non-binary file:

  $ git show 2a0f4bf7 LibGit2Sharp/Configuration.cs
  ...
  diff --git a/LibGit2Sharp/Configuration.cs b/LibGit2Sharp/Configuration.cs
  index 83cc9d6..9ab0b60 100644
  --- a/LibGit2Sharp/Configuration.cs
  +++ b/LibGit2Sharp/Configuration.cs

Looks OK. Now try a diff that also has a binary file (that is marked such via gitattributes):

  $ git show 2a0f4bf7 Lib/NativeBinaries/x86/git2.dll \
                      LibGit2Sharp/Configuration.cs
  ...
  diff --git a/Lib/NativeBinaries/x86/git2.dll b/Lib/NativeBinaries/x86/git2.dll
  index dab0d04..8de18ab 100644
  Binary files a/Lib/NativeBinaries/x86/git2.dll and b/Lib/NativeBinaries/x86/git2.dll differ
  diff --git a/LibGit2Sharp/Configuration.cs b/LibGit2Sharp/Configuration.cs
  index 83cc9d6..9ab0b60 100644
  Binary files a/LibGit2Sharp/Configuration.cs and b/LibGit2Sharp/Configuration.cs differ

Now the Configuration.cs blobs appear binary!

It has nothing to do with pathspecs; if you do a non-limited diff of 2a0f4bf7, you'll see many of the files appear as binary. Running it through the debugger, it looks like we are getting wrong diff attribute values for later paths, as if the earlier lookups are somehow polluting the attribute stack.

The gitattributes in this repository look reasonably sane, but even if they were not, nothing should make a file have different attributes based on other files that were diffed.

Bisection points to ec775c4 (attr: Expand macros immediately when encountered., 2010-04-06), but it's too late for me to dig further tonight. Cc'ing Junio as the author of the attr code and Henrik as the author of ec775c4.

-Peff

On 01/10/2012 08:03 AM, Jeff King wrote:

The problem has nothing with diffing; simply interrogating the attribute values gives different results depending on the order of the files:

$ git check-attr --all Lib/NativeBinaries/x86/git2.dll LibGit2Sharp/Configuration.cs Lib/NativeBinaries/x86/git2.dll: binary: set Lib/NativeBinaries/x86/git2.dll: diff: unset Lib/NativeBinaries/x86/git2.dll: text: unset LibGit2Sharp/Configuration.cs: binary: set LibGit2Sharp/Configuration.cs: diff: unset LibGit2Sharp/Configuration.cs: text: unset LibGit2Sharp/Configuration.cs: crlf: set $ git check-attr --all LibGit2Sharp/Configuration.cs Lib/NativeBinaries/x86/git2.dll LibGit2Sharp/Configuration.cs: diff: csharp LibGit2Sharp/Configuration.cs: crlf: set Lib/NativeBinaries/x86/git2.dll: binary: set Lib/NativeBinaries/x86/git2.dll: diff: unset Lib/NativeBinaries/x86/git2.dll: text: unset

It also doesn't depend on the fact that Lib/.gitattributes uses CRLF as its EOL, nor does it depend on the use of the "binary" macro. However, it does depend on the fact that the directory name "Lib" matches the first part of the directory name "LibGit2Sharp". Here is a simplified demonstration of the problem:

a=LibA/a.txt b=Lib/b.bin rm -rf foo git init foo cd foo mkdir $(dirname $a) $(dirname $b) touch $a $b echo '*.txt foo' >.gitattributes echo '* bar' >$(dirname $b)/.gitattributes git add . git commit -am 'Demonstrate problem' echo '=================================================' git check-attr --all $b $a echo '=================================================' git check-attr --all $a $b echo '================================================='

The attributes of $a are different depending on what order $a and $b appear in the "git check-attr" command line.

Changing the example to "a=foo/a.txt" makes the problem go away.

Michael

On Tue, Jan 10, 2012 at 10:01:34AM +0100, Michael Haggerty wrote:

Thanks. I tried to test it with check-attr but for some reason wasn't able to provoke the bug (I think I probably just screwed up the invocation).

Ah, very helpful. That allowed me to find the problem very quickly by grepping for "strncmp". :)

The patch below seem to fix it for me. I'll do a bit more testing before posting it for real, though.

-Peff

When we prepare the attribute stack for a lookup on a path, we start with the cached stack from the previous lookup (because it is common to do several lookups in the same directory hierarchy). So the first thing we must do in preparing the stack is to pop any entries that point to directories we are no longer interested in.

For example, if our stack contains gitattributes for:

  foo/bar/baz
  foo/bar
  foo

but we want to do a lookup in "foo/bar/bleep", then we want to pop the top element, but retain the others.

To do this we walk down the stack from the top, popping elements that do not match our lookup directory. However, the test do this simply checked strncmp, meaning we would mistake "foo/bar/baz" as a leading directory of "foo/bar/baz_plus". We must also check that the character after our match is '/', meaning we matched the whole path component.

There are two special cases to consider:

  1. The top of our attr stack has the empty path. So we
     must not check for '/', but rather special-case the
     empty path, which always matches.

  2. Typically when matching paths in this way, you would
     also need to check for a full string match (i.e., the
     character after is '\0'). We don't need to do so in
     this case, though, because our path string is actually
     just the directory component of the path to a file
     (i.e., we know that it terminates with "/", because the
     filename comes after that).

Helped-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Jeff King <peff@peff.net>
---
I wrote it in the minimally-intrusive way, but you could also pull
the "!namelen" case before the strncmp (since a strncmp of size 0 is
always true, anyway). I don't know if it would be more obvious that way.

I prepared this on top of master, but the patch applies (with some shifted line counts) on older releases, too.

 attr.c                |    3 ++-
 t/t0003-attributes.sh |   10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletions(-)

On Tue, Jan 10, 2012 at 01:08:21PM -0500, Jeff King wrote:

Side note. One thing that confused me about this code is that prepare_attr_stack does a popping loop like this:

  while (attr_stack && attr_stack->origin) {
          if (/* attr_stack->origin is a prefix */)
                  break;
          /* otherwise, pop */
          elem = attr_stack;
          attr_stack = elem->prev;
          free(elem);
  }

  /* now push our new ones */
  ...
  len = strlen(attr_stack->origin);

IOW, our loop breaks out when attr_stack is NULL, but then we go on to assume that attr_stack is _not_ NULL. This isn't a bug, because it turns out that we always leave something in the attr_stack: the root gitattributes file (and the builtins). But it is slightly confusing to a reader because of the useless loop condition.

I'm not sure if the right solution is to change the popping loop to:

  /* we will never run out of stack, because we always have the root */
  while (attr_stack->origin) {
          ...

Or to be extra defensive and put:

  if (!attr_stack)
          die("BUG: we ran out of attr stack!?");

after the loop, or to somehow handle the case of an empty attr stack below (which is hard to do, because it can't be triggered, so I have no idea what it would mean).

-Peff

Jeff King <peff@peff.net> writes:

Yeah, that makes sense, as that existing check "attr_stack &&" was a misguided defensive coding, that was _not_ defensive at all as we didn't do anything after we stop iterating from that loop and without checking dereferenced attr_stack->origin, which was a simple bogosity.

And this is even more so.

On Tue, Jan 10, 2012 at 11:23:01AM -0800, Junio C Hamano wrote:

I wasn't clear: the second one is "even more so" making sense, or "even more so" misguided defensive coding?

-Peff

On Tue, Jan 10, 2012 at 02:28:10PM -0500, Jeff King wrote:

If the latter, then I think we want this:

-- >8 --
Subject: [PATCH] attr: drop misguided defensive coding

In prepare_attr_stack, we pop the old elements of the stack (which were left from a previous lookup and may or may not be useful to us). Our loop to do so checks that we never reach the top of the stack. However, the code immediately afterwards will segfault if we did actually reach the top of the stack.

Fortunately, this is not an actual bug, since we will never pop all of the stack elements (we will always keep the root gitattributes, as well as the builtin ones). So the extra check in the loop condition simply clutters the code and makes the intent less clear. Let's get rid of it.

Signed-off-by: Jeff King <peff@peff.net>
---
 attr.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Jeff King <peff@peff.net> writes:

Sorry for sending a half-baked response. The initial draft of my response had just "that makes sense" and nothing else in the first paragraph.

If the original meant to be defensive, it should have had your "extra defensive" die(), but it didn't.

But the condition to break out of that loop is either we hit an elem that satisfy the condition (in which case that elem cannot be NULL) or we successfully saw that attr_stack->origin is NULL (in which case attr_stack couldn't have been NULL), so it is pointless to check the NULLness of attr_stack itself. Assertion _before_ going into the while loop might make sense, but if we look at what bootstrap_attr_stack() does, it should be pretty obvious that that cannot happen.

An assert(attr_stack->origin) before we use it may make sense, though, in order to make sure we do not mistakenly pop the root one and expose the built-in ones whose origin are set to NULL.

On Tue, Jan 10, 2012 at 12:25:09PM -0800, Junio C Hamano wrote:

Yeah, this version looks good to me (though I thought we usually spelled assert as die("BUG: ...")).

-Peff

Jeff King <peff@peff.net> writes:

Thanks for the fix; I was looking at path_matches() and wondering about the same thing.

Jeff King <peff@peff.net> writes:

Thanks for getting the ball rolling.

Regardless of this unrelated regression, after looking at what ec775c4 wanted to do again, I am very much tempted to just revert it.

It aimed to take these three

        *       ident
        foo     mybin
        bar     mybin ident

and wanted to omit 'ident' from "foo" when there is this macro definition elsewhere:

	[attr] mybin binary -ident

But the real point of the macro was that the users do not have to know their internals, iow, if you explicitly specify a pattern that overrides the contents of the macro, that explicit pattern should win. When deciding the value of "ident" attribute for path "foo", "* ident" is stronger than "foo mybin" (the latter of which does not say anything about 'ident' explicitly).

On 01/10/2012 06:22 PM, Junio C Hamano wrote:

I like the simplicity of the rule "apply attributes in the order found in the .gitattributes files" better than the rule you are proposing, which seems like it will become more complicated to explain.

For example, it would seem under your rule for the above example that the "mybin" macro should bestow on file foo the "binary" attribute and also the "mybin" attribute (given that macros are themselves attributes), but not "-ident".

You would also have to decide and explain whether a macro that invokes a macro that sets or clears attribute "foo" is "weaker" than a simple macro that clears or sets attribute "foo".

I have one real-life use case that would become more difficult with your rule:

# Marker for textlike files whose EOL characters haven't been # normalized yet: [attr]eol-fixme -text !eol

*.cc text eol=lf

# Then later, perhaps in some subdirectory's .gitattributes: SomeParticularScrewedUpFile.cc eol-fixme

The point of the eol-fixme macro is (1) to prevent git from throwing a tantrum and (2) to mark the file as needing cleanup sometime in the future.

Michael