This applies to all three tests: you're not working in a repository here, as you forgot to call

g_repo = cl_git_sandbox_init("empty_standard_repo");

This comment gave me a bit of a fright, but it looks like make_test_data is init-ing the repository for me, so I think I'm OK? 😌

Heh, yeah. My fault for didn't taking a proper look

Minor style-nit: all the other calls have the first argument on the same line. There's also a newline between make_test_data and cl_git_mkfile.

Good call. I did a survey of the other test functions, and most of the have the first argument on the next line, and tests that use make_test_data tend to not have the space between it and cl_git_mkfile, so I'll push a new commit standardizing on that form.

This is not really a fault of yours, as you simply mirror the previous style, but: do we really want to compare the pointers here? I bet this should be a strcmp instead. And if not, we should at least add a comment here to explain what's going on.

I honestly don't recall if I was intending for this to be pointer comparison, however I added some debug logging this morning and it does work, because the pointers in question are all derived from offsets into path->path (basename gets derived in git_attr_path__init; relpath gets derived earlier in git_attr_fnmatch__match).

I would definitely like someone to triple-check my thought process on this though. I don't know if this is actually helpful to others, but it helps my thinking to write it down so:

Based on the immediate closing if-statement: (match->flags & GIT_ATTR_FNMATCH_DIRECTORY) && !path->is_dir), my read of the original early return is:

a) IF we're trying to match a directory
b) AND the path we're looking at is not a directory
c) AND the basename and path are the same (i.e., they represent an entry at the root of the tree),
THEN we can't possibly match because we must be looking at a file (because of b) ) in the root of the git tree.

The change to use relpath is an attempt to say the same thing, but to extend c) to include any file directly under the containing_dir of the rule we're comparing with.

Or at least that's what I've been telling myself. :)

I'll throw some more test cases at it. I'm still not 100% convinced that replacing path->path with relpath is the perfect solution. (Does it work if the .gitignore is at the root, and it's ignoring directories in a subdirectory? I think it doesn't...) I think it does preserve existing behavior w.r.t. files at the root, and extends it to files at the root of directories w/ .gitignores.

Okay. I'll take a jab at this, hopefully no later than tomorrow.

So just to clear up my thinking here:

a) IF we're trying to match a directory
b) AND the path we're looking at is not a directory
c) AND the basename and path are not the same

then we have a path containing at least one directory, e.g. "foo/bar", where we have to do additional checks. If we're inside of "foo" and try to match a directory "bar" but find a file "bar", we should not match, but currently we do not realize because we use the root-based path->path ("foo/bar") instead of relpath ("bar") to match against the basename.

So I think this change is correct. The comment is becoming a bit stale though, as strictly speaking we do not do this check for the root directory only.

That line of thinking is what I was thinking. :)

I fixed and force-pushed the changes requested w.r.t. test code style, and an attempt at updating the comment here.