Re: [PATCH v3 5/5] attr.c: respect core.ignorecase when matching attribute patterns

On Sun, Oct 9, 2011 at 10:16 AM, Michael Haggerty [off-list ref] wrote:

On 10/06/2011 08:22 PM, Brandon Casey wrote:

quoted

The last set of tests is performed only on a case-insensitive filesystem.
Those tests make sure that git handles the case where the .gitignore file
resides in a subdirectory and the user supplies a path that does not match
the case in the filesystem.  In that case^H^H^H^Hsituation, part of the
path supplied by the user is effectively interpreted case-insensitively,
and part of it is dependent on the setting of core.ignorecase.  git should
only match the portion of the path below the directory holding the
.gitignore file according to the setting of core.ignorecase.

Isn't this a rather perverse scenario?

No argument here. :)

It is hard to imagine that
anybody *wants* part of a single filename to be matched
case-insensitively and another part to be matched case-sensitively.
ISTM that a person who is using a case-insensitive filesystem and has
core-ignorecase=false is (1) a glutton for punishment and (2) probably
very careful to make sure that the case of all pathnames is correct.  So
such a person is not likely to benefit from this schizophrenic behavior.

quoted

[...]  If git instead built the attr
stack by scanning the repository, then the paths in the origin field would
not necessarily match the paths supplied by the user.  If someone makes a
change like that in the future, these tests will notice.

Your decision to treat path names as partly case-insensitive...

You are giving more credit to this patch than it deserves.  It really
doesn't do much.  It's not a design decision that I made to treat path
names as case-insensitive.  That is a consequence of using a
case-insensitive file system.  When you give git the path A/b/c and
there exists a/.gitattributes, then on a case insensitive file system
git will try to read A/.gitattributes and the filesystem will return a
handle for a/.gitattributes and git won't be able to tell the
difference.  git will then apply the rules in the file to the paths
below a/ even when the path is supplied like A/.  So, at the moment,
regardless of the setting of core.ignorecase, the path above a
.gitignore file is treated as case-insensitive on a case-insensitive
filesystem.

The purpose of the tests are primarily to ensure that nothing special
needs to be done concerning paths leading up to a directory containing
a .gitignore file in the core.ignorecase=1 case.  For example, it is
_not_ necessary to use strncmp_icase instead of strncmp on the leading
portion of the path.

What I meant when I said "If someone makes a change like that in the
future, these tests will notice", is that they will notice that they
must now use strncmp_icase when comparing the portion of the path
above the .gitattributes file.

will make
this kind of improvement considerably more complicated.
Therefore, weighing the negligible benefit of declaring this
schizophrenic behavior against the potential big pain of remaining
backwards-compatible with this behavior, I suggest that we either (1)
declare that when core.ignorecase=false then the *whole* filenames
(including the path leading up to the .gitignore file) must match
case-sensitively, or (2) declare the behavior in this situation to be
undefined so that nobody thinks they should depend on it.

I do not plan to make git treat leading paths case-sensitively on a
case-insensitive file system when core.ignorecase=0 any more than I
plan to make git treat leading paths case-insensitively on a
case-sensitive file system when core.ignorecase=1.  For justification,
I refer back to your original comment about perversion. :)

So, #2 gets my vote.


Maybe my commit message is not clear that it is describing the current
behavior and not defining it.  Instead of

   git should only match the portion of the path below the directory
   holding the .gitignore file according to the setting of
   core.ignorecase.

maybe I should say

    git will currently only match the portion of the path...

I could also remove the following test from the CASE_INSENSITIVE_FS
tests since it is really a dontcare:

   attr_check A/b/h a/b/h "-c core.ignorecase=0"

We don't care what happens when the user supplies A/b/h and a/b/h
exists on disk when core.ignorecase=0, we only care that A/b/h is
interpreted correctly when core.ignorecase=1.

-Brandon