threads / patch / 24755

patch, 6 partsAdd string comparison functions that respect the ignore_case variable.

Subject: [PATCH 1/6] Add string comparison functions that respect the ignore_case variable.

## tl;dr

26 messages between Aug 16, 2010 and Aug 30, 2010. Diffs are folded; open one to read it.

replies: 25people: 6as markdown or json

Johannes Sixt· Aug 16, 2010, 19:38 UTC · lore

[PATCH 0/6] Extensions of core.ignorecase=true support

The following patch series extends the core.ignorecase=true support to handle case insensitive comparisons for the .gitignore file, git status, and git ls-files. git add and git fast-import will fold the case of the file being added, matching that of an already added directory entry. Case folding is also applied to git fast-import for renames, copies, and deletes.

The most notable benefit, IMO, is that the case of directories in the worktree does not matter if, and only if, the directory exists already in the index with some different case variant. This helps applications on Windows that change the case even of directories in unpredictable ways. Joshua mentioned Perforce as the primary example.

Concerning the implementation, Joshua explained when he initially submitted the series to the msysgit mailing list:

 git status and add both use an update made to name-hash.c where 
 directories, specifically names with a trailing slash, can be looked up 
 in a case insensitive manner. After trying a myriad of solutions, this 
 seemed to be the cleanest. Does anyone see a problem with embedding the 
 directory names in the same hash as the file names? I couldn't find one, 
 especially since I append a slash to each directory name.
 The git add path case folding functionality is a somewhat radical 
 departure from what Git does now. It is described in detail in patch 5. 
 Does anyone have any concerns?

I support the idea of this patch, and I can confirm that it works: I've used this series in production both with core.ignorecase set to true and to false, and in the former case, with directories and files with case different from the index.

Joshua Jensen (6):
  Add string comparison functions that respect the ignore_case
    variable.
  Case insensitivity support for .gitignore via core.ignorecase
  Add case insensitivity support for directories when using git status
  Add case insensitivity support when using git ls-files
  Support case folding for git add when core.ignorecase=true
  Support case folding in git fast-import when core.ignorecase=true
 dir.c         |  105 ++++++++++++++++++++++++++++++++++++++++++++++----------
 dir.h         |    4 ++
 fast-import.c |    7 ++--
 name-hash.c   |   72 ++++++++++++++++++++++++++++++++++++++-
 read-cache.c  |   23 ++++++++++++
 5 files changed, 188 insertions(+), 23 deletions(-)
Johannes Sixt· Aug 16, 2010, 19:38 UTC · re: Johannes Sixt · lore
From: Joshua Jensen <jjensen@workspacewhiz.com>

Multiple locations within this patch series alter a case sensitive string comparison call such as strcmp() to be a call to a string comparison call that selects case comparison based on the global ignore_case variable. Behaviorally, when core.ignorecase=false, the *_icase() versions are functionally equivalent to their C runtime counterparts. When core.ignorecase=true, the *_icase() versions perform a case insensitive comparison.

Like Linus' earlier ignorecase patch, these may ignore filename conventions on certain file systems. By isolating filename comparisons to certain functions, support for those filename conventions may be more easily met.

Signed-off-by: Joshua Jensen <jjensen@workspacewhiz.com>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 dir.c |   16 ++++++++++++++++
 dir.h |    4 ++++
 2 files changed, 20 insertions(+), 0 deletions(-)
Show changes to 2 files +20 −0

dir.c, dir.h

diff --git a/dir.c b/dir.c
index 133f472..4d001fd 100644
--- a/dir.c
+++ b/dir.c
@@ -18,6 +18,22 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, in
 	int check_only, const struct path_simplify *simplify);
 static int get_dtype(struct dirent *de, const char *path, int len);
 
+/* helper string functions with support for the ignore_case flag */
+int strcmp_icase(const char *a, const char *b)
+{
+	return ignore_case ? strcasecmp(a, b) : strcmp(a, b);
+}
+
+int strncmp_icase(const char *a, const char *b, size_t count)
+{
+	return ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count);
+}
+
+int fnmatch_icase(const char *pattern, const char *string, int flags)
+{
+	return fnmatch(pattern, string, flags | (ignore_case ? FNM_CASEFOLD : 0));
+}
+
 static int common_prefix(const char **pathspec)
 {
 	const char *path, *slash, *next;
diff --git a/dir.h b/dir.h
index 278d84c..b3e2104 100644
--- a/dir.h
+++ b/dir.h
@@ -101,4 +101,8 @@ extern int remove_dir_recursively(struct strbuf *path, int flag);
 /* tries to remove the path with empty directories along it, ignores ENOENT */
 extern int remove_path(const char *path);
 
+extern int strcmp_icase(const char *a, const char *b);
+extern int strncmp_icase(const char *a, const char *b, size_t count);
+extern int fnmatch_icase(const char *pattern, const char *string, int flags);
+
 #endif
-- 
1.7.1.402.gf1eeb
Ævar Arnfjörð Bjarmason· Aug 18, 2010, 12:52 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/6] Add string comparison functions that respect the ignore_case variable.

On Mon, Aug 16, 2010 at 19:38, Johannes Sixt <j6t@kdbg.org> wrote:
> +       return fnmatch(pattern, string, flags | (ignore_case ? FNM_CASEFOLD : 0));
On Solaris 10:

dir.c: In function `fnmatch_icase': dir.c:34: error: `FNM_CASEFOLD' undeclared (first use in this function) dir.c:34: error: (Each undeclared identifier is reported only once dir.c:34: error: for each function it appears in.)

Ævar Arnfjörð Bjarmason· Aug 18, 2010, 12:53 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/6] Add string comparison functions that respect the ignore_case variable.

On Wed, Aug 18, 2010 at 12:52, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 10 quoted lines
> On Mon, Aug 16, 2010 at 19:38, Johannes Sixt <j6t@kdbg.org> wrote:
>
>> +       return fnmatch(pattern, string, flags | (ignore_case ? FNM_CASEFOLD : 0));
>
> On Solaris 10:
>
> dir.c: In function `fnmatch_icase':
> dir.c:34: error: `FNM_CASEFOLD' undeclared (first use in this function)
> dir.c:34: error: (Each undeclared identifier is reported only once
> dir.c:34: error: for each function it appears in.)

Actually, reading the fnmatch manpage it's not just Solaris, but all non-GNU systems:

       FNM_CASEFOLD
              If this flag (a GNU extension) is set, the pattern is
matched case-insensitively.
Joshua Jensen· Aug 18, 2010, 15:52 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/6] Add string comparison functions that respect the ignore_case variable.

  ----- Original Message -----
From: Ævar Arnfjörð Bjarmason
Date: 8/18/2010 6:53 AM
Show 13 quoted lines
> On Wed, Aug 18, 2010 at 12:52, Ævar Arnfjörð Bjarmason<avarab@gmail.com>  wrote:
>> On Mon, Aug 16, 2010 at 19:38, Johannes Sixt<j6t@kdbg.org>  wrote:
>>> +       return fnmatch(pattern, string, flags | (ignore_case ? FNM_CASEFOLD : 0));
>> On Solaris 10:
>>
>> dir.c: In function `fnmatch_icase':
>> dir.c:34: error: `FNM_CASEFOLD' undeclared (first use in this function)
>> dir.c:34: error: (Each undeclared identifier is reported only once
>> dir.c:34: error: for each function it appears in.)
> Actually, reading the fnmatch manpage it's not just Solaris, but all
> non-GNU systems:
>
>         FNM_CASEFOLD - If this flag (a GNU extension) is set, the pattern is matched case-insensitively
Well, that's no good.  :(
Thanks for the research.  It helps tremendously.

One easy way out of this situation would be to duplicate the GNU fnmatch() into fnmatch_icase(). I have not looked at the source code, so it may not be possible. If it can be copied in, does anyone object?

I'll also look for a non-GNU function that may work.
Josh
Ævar Arnfjörð Bjarmason· Aug 18, 2010, 16:07 UTC · re: Joshua Jensen · lore

Re: [PATCH 1/6] Add string comparison functions that respect the ignore_case variable.

On Wed, Aug 18, 2010 at 15:52, Joshua Jensen <jjensen@workspacewhiz.com> wrote:
Show 34 quoted lines
>  ----- Original Message -----
> From: Ævar Arnfjörð Bjarmason
> Date: 8/18/2010 6:53 AM
>>
>> On Wed, Aug 18, 2010 at 12:52, Ævar Arnfjörð Bjarmason<avarab@gmail.com>
>>  wrote:
>>>
>>> On Mon, Aug 16, 2010 at 19:38, Johannes Sixt<j6t@kdbg.org>  wrote:
>>>>
>>>> +       return fnmatch(pattern, string, flags | (ignore_case ?
>>>> FNM_CASEFOLD : 0));
>>>
>>> On Solaris 10:
>>>
>>> dir.c: In function `fnmatch_icase':
>>> dir.c:34: error: `FNM_CASEFOLD' undeclared (first use in this function)
>>> dir.c:34: error: (Each undeclared identifier is reported only once
>>> dir.c:34: error: for each function it appears in.)
>>
>> Actually, reading the fnmatch manpage it's not just Solaris, but all
>> non-GNU systems:
>>
>>        FNM_CASEFOLD - If this flag (a GNU extension) is set, the pattern
>> is matched case-insensitively
>
> Well, that's no good.  :(
>
> Thanks for the research.  It helps tremendously.
>
> One easy way out of this situation would be to duplicate the GNU fnmatch()
> into fnmatch_icase().  I have not looked at the source code, so it may not
> be possible.  If it can be copied in, does anyone object?
>
> I'll also look for a non-GNU function that may work.

According to some further research at least FreeBSD and NetBSD have copied this GNU extension. You may find their versions easier to integrate.

Johannes Sixt· Aug 18, 2010, 18:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/6] Add string comparison functions that respect the ignore_case variable.

On Mittwoch, 18. August 2010, Ævar Arnfjörð Bjarmason wrote:
> According to some further research at least FreeBSD and NetBSD have
> copied this GNU extension. You may find their versions easier to
> integrate.
We already have a GNU fnmatch in compat/fnmatch.
-- Hannes
Ævar Arnfjörð Bjarmason· Aug 18, 2010, 18:58 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/6] Add string comparison functions that respect the ignore_case variable.

On Wed, Aug 18, 2010 at 18:32, Johannes Sixt <j6t@kdbg.org> wrote:
Show 6 quoted lines
> On Mittwoch, 18. August 2010, Ævar Arnfjörð Bjarmason wrote:
>> According to some further research at least FreeBSD and NetBSD have
>> copied this GNU extension. You may find their versions easier to
>> integrate.
>
> We already have a GNU fnmatch in compat/fnmatch.

I didn't spot that. But until now it's only been used for Windows, there isn't a NO_* flag you can set to compile it in.

Ævar Arnfjörð Bjarmason· Aug 29, 2010, 19:39 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/6] Add string comparison functions that respect the ignore_case variable.

On Wed, Aug 18, 2010 at 18:32, Johannes Sixt <j6t@kdbg.org> wrote:
Show 6 quoted lines
> On Mittwoch, 18. August 2010, Ævar Arnfjörð Bjarmason wrote:
>> According to some further research at least FreeBSD and NetBSD have
>> copied this GNU extension. You may find their versions easier to
>> integrate.
>
> We already have a GNU fnmatch in compat/fnmatch.

Do you have any plan to deal with this? I currently have this monkeypatch to build on Solaris:

    diff --git a/Makefile b/Makefile
    index 62d526a..079fae5 100644
    --- a/Makefile
    +++ b/Makefile
    @@ -863,2 +863,4 @@ endif
     ifeq ($(uname_S),SunOS)
    +       COMPAT_OBJS = compat/fnmatch/fnmatch.o
    +       COMPAT_CFLAGS = -Icompat -Icompat/fnmatch
            NEEDS_SOCKET = YesPlease

One way to deal with it would be a new NONGNU_FNMATCH=UnfortunatelyYes flag, or the fnmatch_icase() suggestion above which we could bundle and always use. But having next build on systems without GNU extensions would be preferrable.

Joshua Jensen· Aug 30, 2010, 14:42 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/6] Add string comparison functions that respect the ignore_case variable.

  ----- Original Message -----
From: Ævar Arnfjörð Bjarmason
Date: 8/29/2010 1:39 PM
Show 23 quoted lines
> On Wed, Aug 18, 2010 at 18:32, Johannes Sixt<j6t@kdbg.org>  wrote:
>> On Mittwoch, 18. August 2010, Ævar Arnfjörð Bjarmason wrote:
>>> According to some further research at least FreeBSD and NetBSD have
>>> copied this GNU extension. You may find their versions easier to
>>> integrate.
>> We already have a GNU fnmatch in compat/fnmatch.
> Do you have any plan to deal with this? I currently have this
> monkeypatch to build on Solaris:
>
>      diff --git a/Makefile b/Makefile
>      index 62d526a..079fae5 100644
>      --- a/Makefile
>      +++ b/Makefile
>      @@ -863,2 +863,4 @@ endif
>       ifeq ($(uname_S),SunOS)
>      +       COMPAT_OBJS = compat/fnmatch/fnmatch.o
>      +       COMPAT_CFLAGS = -Icompat -Icompat/fnmatch
>              NEEDS_SOCKET = YesPlease
>
> One way to deal with it would be a new NONGNU_FNMATCH=UnfortunatelyYes
> flag, or the fnmatch_icase() suggestion above which we could bundle
> and always use. But having next build on systems without GNU
> extensions would be preferrable.

I am going to deal with this, but I haven't been around. I hope for some time this week.

Short of duplicating fnmatch's code and renaming the function, I am not sure how to make this play nice on all systems. You added COMPAT_OBJS above, but I think there is no linker guarantee it will pick up compat/fnmatch/fnmatch.o over the C runtime version? Perhaps the makefile is architected to do so.

The safest alternative is to allocate character buffers, lowercase the filename and match arguments into those buffers, and pass them off to fnmatch without any special flags. I don't like the idea of a double memory allocation/free combo per each call to this function, but it would work. Is anyone opposed to this approach?

Josh
Ævar Arnfjörð Bjarmason· Aug 30, 2010, 14:51 UTC · re: Joshua Jensen · lore

Re: [PATCH 1/6] Add string comparison functions that respect the ignore_case variable.

On Mon, Aug 30, 2010 at 14:42, Joshua Jensen <jjensen@workspacewhiz.com> wrote:
Show 34 quoted lines
>  ----- Original Message -----
> From: Ævar Arnfjörð Bjarmason
> Date: 8/29/2010 1:39 PM
>>
>> On Wed, Aug 18, 2010 at 18:32, Johannes Sixt<j6t@kdbg.org>  wrote:
>>>
>>> On Mittwoch, 18. August 2010, Ævar Arnfjörð Bjarmason wrote:
>>>>
>>>> According to some further research at least FreeBSD and NetBSD have
>>>> copied this GNU extension. You may find their versions easier to
>>>> integrate.
>>>
>>> We already have a GNU fnmatch in compat/fnmatch.
>>
>> Do you have any plan to deal with this? I currently have this
>> monkeypatch to build on Solaris:
>>
>>     diff --git a/Makefile b/Makefile
>>     index 62d526a..079fae5 100644
>>     --- a/Makefile
>>     +++ b/Makefile
>>     @@ -863,2 +863,4 @@ endif
>>      ifeq ($(uname_S),SunOS)
>>     +       COMPAT_OBJS = compat/fnmatch/fnmatch.o
>>     +       COMPAT_CFLAGS = -Icompat -Icompat/fnmatch
>>             NEEDS_SOCKET = YesPlease
>>
>> One way to deal with it would be a new NONGNU_FNMATCH=UnfortunatelyYes
>> flag, or the fnmatch_icase() suggestion above which we could bundle
>> and always use. But having next build on systems without GNU
>> extensions would be preferrable.
>
> I am going to deal with this, but I haven't been around.  I hope for some
> time this week.

Sure, no rush. I was just wondering whether you had some plan for it, or whether I should submit a patch to use the fallback on e.g. Solaris.

> Short of duplicating fnmatch's code and renaming the function, I am not sure
> how to make this play nice on all systems.

I don't think duplicating the GNU (or *BSD) version into a compat/fnmatch.c would be a bad thing. See e.g. compat/snprintf.c.

> You added COMPAT_OBJS above, but I think there is no linker
> guarantee it will pick up compat/fnmatch/fnmatch.o over the C
> runtime version?  Perhaps the makefile is architected to do so.

It's probably just an artifact of how the Solaris linker works, it doesn't go through the trouble of undefining an existing fnmatch symbol or anything.

Show 5 quoted lines
> The safest alternative is to allocate character buffers, lowercase the
> filename and match arguments into those buffers, and pass them off to
> fnmatch without any special flags.  I don't like the idea of a double memory
> allocation/free combo per each call to this function, but it would work.  Is
> anyone opposed to this approach?

Just using the GNU extension and providing a fallback is probably cleaner and easier to maintain.

Jonathan Nieder· Aug 30, 2010, 15:05 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/6] Add string comparison functions that respect the ignore_case variable.

Ævar Arnfjörð Bjarmason wrote:
> I don't think duplicating the GNU (or *BSD) version into a
> compat/fnmatch.c would be a bad thing. See e.g. compat/snprintf.c.

Maybe meanwhile someone (I guess that means "I") can ask around about FNM_CASEFOLD going into posix. I like to imagine the compat/ directory shrinking over time.

Show 5 quoted lines
>> You added COMPAT_OBJS above, but I think there is no linker
>> guarantee it will pick up compat/fnmatch/fnmatch.o over the C
>> runtime version?  Perhaps the makefile is architected to do so.
>
> It's probably just an artifact of how the Solaris linker works
Isn't the portable way to do something like
 #define fnmatch gitfnmatch
in fnmatch.h?

Meanwhile I guess that no other system header includes fnmatch.h, but if that were to happen, there would be preprocessor symbol conflicts. So on systems with fnmatch, ideally one would want to do something like this:

 #include <fnmatch.h>
 #ifdef FNMATCH_LACKS_CASEFOLD
 # undef FNM_PATHNAME
 # define FNM_PATHNAME	(1 << 0)
 # define FNM_CASEFOLD	(1 << 4)
 # define fnmatch gitfnmatch
 extern int fnmatch(const char *pattern, const char *name, int flags);
 #endif
Jonathan Nieder· Aug 30, 2010, 14:52 UTC · re: Joshua Jensen · lore

Re: [PATCH 1/6] Add string comparison functions that respect the ignore_case variable.

Joshua Jensen wrote:
> I don't like the idea of
> a double memory allocation/free combo per each call to this
> function, but it would work.
Maybe the caller can provide a (strbuf) buffer?
Johannes Sixt· Aug 30, 2010, 18:40 UTC · re: Joshua Jensen · lore

Re: [PATCH 1/6] Add string comparison functions that respect the ignore_case variable.

On Montag, 30. August 2010, Joshua Jensen wrote:
> From: Ævar Arnfjörð Bjarmason
> >      +       COMPAT_OBJS = compat/fnmatch/fnmatch.o
> >      +       COMPAT_CFLAGS = -Icompat -Icompat/fnmatch
...
> You added COMPAT_OBJS
> above, but I think there is no linker guarantee it will pick up
> compat/fnmatch/fnmatch.o over the C runtime version?

The way linkers work ensures that git code picks up the compat version when defined this way.

-- Hannes
Ævar Arnfjörð Bjarmason· Aug 30, 2010, 19:57 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/6] Add string comparison functions that respect the ignore_case variable.

On Mon, Aug 30, 2010 at 18:40, Johannes Sixt <j6t@kdbg.org> wrote:
Show 11 quoted lines
> On Montag, 30. August 2010, Joshua Jensen wrote:
>> From: Ævar Arnfjörð Bjarmason
>> >      +       COMPAT_OBJS = compat/fnmatch/fnmatch.o
>> >      +       COMPAT_CFLAGS = -Icompat -Icompat/fnmatch
> ...
>> You added COMPAT_OBJS
>> above, but I think there is no linker guarantee it will pick up
>> compat/fnmatch/fnmatch.o over the C runtime version?
>
> The way linkers work ensures that git code picks up the compat version when
> defined this way.

So why does e.g. compat/snprintf.c need to "#undef snprintf" before defining it again?

Johannes Sixt· Aug 30, 2010, 20:13 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/6] Add string comparison functions that respect the ignore_case variable.

On Montag, 30. August 2010, Ævar Arnfjörð Bjarmason wrote:
> So why does e.g. compat/snprintf.c need to "#undef snprintf" before
> defining it again?
Because the implementation of the compat snprintf uses the system's snprintf.

There are other cases, like e.g., pread, where it would not be necessary to #define pread git_pread.

-- Hannes
Johannes Sixt· Aug 16, 2010, 19:38 UTC · re: Johannes Sixt · lore

[PATCH 2/6] Case insensitivity support for .gitignore via core.ignorecase

From: Joshua Jensen <jjensen@workspacewhiz.com>

This is especially beneficial when using Windows and Perforce and the git-p4 bridge. Internally, Perforce preserves a given file's full path including its case at the time it was added to the Perforce repository. When syncing a file down via Perforce, missing directories are created, if necessary, using the case as stored with the filename. Unfortunately, two files in the same directory can have differing cases for their respective paths, such as /diRa/file1.c and /DirA/file2.c. Depending on sync order, DirA/ may get created instead of diRa/.

It is possible to handle directory names in a case insensitive manner without this patch, but it is highly inconvenient, requiring each character to be specified like so: [Bb][Uu][Ii][Ll][Dd]. With this patch, the gitignore exclusions honor the core.ignorecase=true configuration setting and make the process less error prone. The above is specified like so: Build

Signed-off-by: Joshua Jensen <jjensen@workspacewhiz.com>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 dir.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
Show changes to dir.c +6 −6
diff --git a/dir.c b/dir.c
index 4d001fd..be21c20 100644
--- a/dir.c
+++ b/dir.c
@@ -390,14 +390,14 @@ int excluded_from_list(const char *pathname,
 			if (x->flags & EXC_FLAG_NODIR) {
 				/* match basename */
 				if (x->flags & EXC_FLAG_NOWILDCARD) {
-					if (!strcmp(exclude, basename))
+					if (!strcmp_icase(exclude, basename))
 						return to_exclude;
 				} else if (x->flags & EXC_FLAG_ENDSWITH) {
 					if (x->patternlen - 1 <= pathlen &&
-					    !strcmp(exclude + 1, pathname + pathlen - x->patternlen + 1))
+					    !strcmp_icase(exclude + 1, pathname + pathlen - x->patternlen + 1))
 						return to_exclude;
 				} else {
-					if (fnmatch(exclude, basename, 0) == 0)
+					if (fnmatch_icase(exclude, basename, 0) == 0)
 						return to_exclude;
 				}
 			}
@@ -412,14 +412,14 @@ int excluded_from_list(const char *pathname,
 
 				if (pathlen < baselen ||
 				    (baselen && pathname[baselen-1] != '/') ||
-				    strncmp(pathname, x->base, baselen))
+				    strncmp_icase(pathname, x->base, baselen))
 				    continue;
 
 				if (x->flags & EXC_FLAG_NOWILDCARD) {
-					if (!strcmp(exclude, pathname + baselen))
+					if (!strcmp_icase(exclude, pathname + baselen))
 						return to_exclude;
 				} else {
-					if (fnmatch(exclude, pathname+baselen,
+					if (fnmatch_icase(exclude, pathname+baselen,
 						    FNM_PATHNAME) == 0)
 					    return to_exclude;
 				}
-- 
1.7.1.402.gf1eeb
Johannes Sixt· Aug 16, 2010, 19:38 UTC · re: Johannes Sixt · lore

[PATCH 3/6] Add case insensitivity support for directories when using git status

From: Joshua Jensen <jjensen@workspacewhiz.com>

When using a case preserving but case insensitive file system, directory case can differ but still refer to the same physical directory. git status reports the directory with the alternate case as an Untracked file. (That is, when mydir/filea.txt is added to the repository and then the directory on disk is renamed from mydir/ to MyDir/, git status shows MyDir/ as being untracked.)

Support has been added in name-hash.c for hashing directories with a terminating slash into the name hash. When index_name_exists() is called with a directory (a name with a terminating slash), the name is not found via the normal cache_name_compare() call, but it is found in the slow_same_name() function.

Additionally, in dir.c, directory_exists_in_index_icase() allows newly added directories deeper in the directory chain to be identified.

Ultimately, it would be better if the file list was read in case insensitive alphabetical order from disk, but this change seems to suffice for now.

The end result is the directory is looked up in a case insensitive manner and does not show in the Untracked files list.

Signed-off-by: Joshua Jensen <jjensen@workspacewhiz.com>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 dir.c       |   39 +++++++++++++++++++++++++++++++-
 name-hash.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 109 insertions(+), 2 deletions(-)
Show changes to 2 files +109 −2

dir.c, name-hash.c

diff --git a/dir.c b/dir.c
index be21c20..f80e091 100644
--- a/dir.c
+++ b/dir.c
@@ -485,6 +485,38 @@ enum exist_status {
 };
 
 /*
+ * Do not use the alphabetically stored index to look up
+ * the directory name; instead, use the case insensitive
+ * name hash.
+ */
+static enum exist_status directory_exists_in_index_icase(const char *dirname, int len)
+{
+	struct cache_entry *ce = index_name_exists(&the_index, dirname, len + 1, ignore_case);
+	if (!ce)
+		return index_nonexistent;
+
+	unsigned char endchar = ce->name[len];
+
+	/*
+	 * The cache_entry structure returned will contain this dirname
+	 * and possibly additional path components.
+	 */
+	if (endchar == '/')
+		return index_directory;
+
+	/*
+	 * If there are no additional path components, then this cache_entry
+	 * represents a submodule.  Submodules, despite being directories,
+	 * are stored in the cache without a closing slash.
+	 */
+	if (!endchar && S_ISGITLINK(ce->ce_mode))
+		return index_gitdir;
+
+	/* This should never be hit, but it exists just in case. */
+	return index_nonexistent;
+}
+
+/*
  * The index sorts alphabetically by entry name, which
  * means that a gitlink sorts as '\0' at the end, while
  * a directory (which is defined not as an entry, but as
@@ -493,7 +525,12 @@ enum exist_status {
  */
 static enum exist_status directory_exists_in_index(const char *dirname, int len)
 {
-	int pos = cache_name_pos(dirname, len);
+	int pos;
+
+	if (ignore_case)
+		return directory_exists_in_index_icase(dirname, len);
+
+	pos = cache_name_pos(dirname, len);
 	if (pos < 0)
 		pos = -pos-1;
 	while (pos < active_nr) {
diff --git a/name-hash.c b/name-hash.c
index 0031d78..c6b6a3f 100644
--- a/name-hash.c
+++ b/name-hash.c
@@ -32,6 +32,42 @@ static unsigned int hash_name(const char *name, int namelen)
 	return hash;
 }
 
+static void hash_index_entry_directories(struct index_state *istate, struct cache_entry *ce)
+{
+	/*
+	 * Throw each directory component in the hash for quick lookup
+	 * during a git status. Directory components are stored with their
+	 * closing slash.  Despite submodules being a directory, they never
+	 * reach this point, because they are stored without a closing slash
+	 * in the cache.
+	 *
+	 * Note that the cache_entry stored with the directory does not
+	 * represent the directory itself.  It is a pointer to an existing
+	 * filename, and its only purpose is to represent existence of the
+	 * directory in the cache.  It is very possible multiple directory
+	 * hash entries may point to the same cache_entry.
+	 */
+	unsigned int hash;
+	void **pos;
+
+	const char *ptr = ce->name;
+	while (*ptr) {
+		while (*ptr && *ptr != '/')
+			++ptr;
+		if (*ptr == '/') {
+			++ptr;
+			hash = hash_name(ce->name, ptr - ce->name);
+			if (!lookup_hash(hash, &istate->name_hash)) {
+				pos = insert_hash(hash, ce, &istate->name_hash);
+				if (pos) {
+					ce->next = *pos;
+					*pos = ce;
+				}
+			}
+		}
+	}
+}
+
 static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
 {
 	void **pos;
@@ -47,6 +83,9 @@ static void hash_index_entry(struct index_state *istate, struct cache_entry *ce)
 		ce->next = *pos;
 		*pos = ce;
 	}
+
+	if (ignore_case)
+		hash_index_entry_directories(istate, ce);
 }
 
 static void lazy_init_name_hash(struct index_state *istate)
@@ -97,7 +136,21 @@ static int same_name(const struct cache_entry *ce, const char *name, int namelen
 	if (len == namelen && !cache_name_compare(name, namelen, ce->name, len))
 		return 1;
 
-	return icase && slow_same_name(name, namelen, ce->name, len);
+	if (!icase)
+		return 0;
+
+	/*
+	 * If the entry we're comparing is a filename (no trailing slash), then compare
+	 * the lengths exactly.
+	 */
+	if (name[namelen - 1] != '/')
+		return slow_same_name(name, namelen, ce->name, len);
+
+	/*
+	 * For a directory, we point to an arbitrary cache_entry filename.  Just
+	 * make sure the directory portion matches.
+	 */
+	return slow_same_name(name, namelen, ce->name, namelen < len ? namelen : len);
 }
 
 struct cache_entry *index_name_exists(struct index_state *istate, const char *name, int namelen, int icase)
@@ -115,5 +168,22 @@ struct cache_entry *index_name_exists(struct index_state *istate, const char *na
 		}
 		ce = ce->next;
 	}
+
+	/*
+	 * Might be a submodule.  Despite submodules being directories,
+	 * they are stored in the name hash without a closing slash.
+	 * When ignore_case is 1, directories are stored in the name hash
+	 * with their closing slash.
+	 *
+	 * The side effect of this storage technique is we have need to
+	 * remove the slash from name and perform the lookup again without
+	 * the slash.  If a match is made, S_ISGITLINK(ce->mode) will be
+	 * true.
+	 */
+	if (icase && name[namelen - 1] == '/') {
+		ce = index_name_exists(istate, name, namelen - 1, icase);
+		if (ce && S_ISGITLINK(ce->ce_mode))
+			return ce;
+	}
 	return NULL;
 }
-- 
1.7.1.402.gf1eeb
Johannes Sixt· Aug 16, 2010, 19:38 UTC · re: Johannes Sixt · lore

[PATCH 4/6] Add case insensitivity support when using git ls-files

From: Joshua Jensen <jjensen@workspacewhiz.com>

When mydir/filea.txt is added, mydir/ is renamed to MyDir/, and MyDir/fileb.txt is added, running git ls-files mydir only shows mydir/filea.txt. Running git ls-files MyDir shows MyDir/fileb.txt. Running git ls-files mYdIR shows nothing.

With this patch running git ls-files for mydir, MyDir, and mYdIR shows mydir/filea.txt and MyDir/fileb.txt.

Wildcards are not handled case insensitively in this patch. Example: MyDir/aBc/file.txt is added. git ls-files MyDir/a* works fine, but git ls-files mydir/a* does not.

Signed-off-by: Joshua Jensen <jjensen@workspacewhiz.com>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 dir.c |   38 ++++++++++++++++++++++++++------------
 1 files changed, 26 insertions(+), 12 deletions(-)
Show changes to dir.c +26 −12
diff --git a/dir.c b/dir.c
index f80e091..af5f54f 100644
--- a/dir.c
+++ b/dir.c
@@ -107,16 +107,30 @@ static int match_one(const char *match, const char *name, int namelen)
 	if (!*match)
 		return MATCHED_RECURSIVELY;
 
-	for (;;) {
-		unsigned char c1 = *match;
-		unsigned char c2 = *name;
-		if (c1 == '\0' || is_glob_special(c1))
-			break;
-		if (c1 != c2)
-			return 0;
-		match++;
-		name++;
-		namelen--;
+	if (ignore_case) {
+		for (;;) {
+			unsigned char c1 = tolower(*match);
+			unsigned char c2 = tolower(*name);
+			if (c1 == '\0' || is_glob_special(c1))
+				break;
+			if (c1 != c2)
+				return 0;
+			match++;
+			name++;
+			namelen--;
+		}
+	} else {
+		for (;;) {
+			unsigned char c1 = *match;
+			unsigned char c2 = *name;
+			if (c1 == '\0' || is_glob_special(c1))
+				break;
+			if (c1 != c2)
+				return 0;
+			match++;
+			name++;
+			namelen--;
+		}
 	}
 
 
@@ -125,8 +139,8 @@ static int match_one(const char *match, const char *name, int namelen)
 	 * we need to match by fnmatch
 	 */
 	matchlen = strlen(match);
-	if (strncmp(match, name, matchlen))
-		return !fnmatch(match, name, 0) ? MATCHED_FNMATCH : 0;
+	if (strncmp_icase(match, name, matchlen))
+		return !fnmatch_icase(match, name, 0) ? MATCHED_FNMATCH : 0;
 
 	if (namelen == matchlen)
 		return MATCHED_EXACTLY;
-- 
1.7.1.402.gf1eeb
Johannes Sixt· Aug 16, 2010, 19:38 UTC · re: Johannes Sixt · lore

[PATCH 5/6] Support case folding for git add when core.ignorecase=true

From: Joshua Jensen <jjensen@workspacewhiz.com>

When MyDir/ABC/filea.txt is added to Git, the disk directory MyDir/ABC/ is renamed to mydir/aBc/, and then mydir/aBc/fileb.txt is added, the index will contain MyDir/ABC/filea.txt and mydir/aBc/fileb.txt. Although the earlier portions of this patch series account for those differences in case, this patch makes the pathing consistent by folding the case of newly added files against the first file added with that path.

In read-cache.c's add_to_index(), the index_name_exists() support used for git status's case insensitive directory lookups is used to find the proper directory case according to what the user already checked in. That is, MyDir/ABC/'s case is used to alter the stored path for fileb.txt to MyDir/ABC/fileb.txt (instead of mydir/aBc/fileb.txt).

This is especially important when cloning a repository to a case sensitive file system. MyDir/ABC/ and mydir/aBc/ exist in the same directory on a Windows machine, but on Linux, the files exist in two separate directories. The update to add_to_index(), in effect, treats a Windows file system as case sensitive by making path case consistent.

Signed-off-by: Joshua Jensen <jjensen@workspacewhiz.com>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 read-cache.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)
Show changes to read-cache.c +23 −0
diff --git a/read-cache.c b/read-cache.c
index f1f789b..ae1366a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -608,6 +608,29 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
 		ce->ce_mode = ce_mode_from_stat(ent, st_mode);
 	}
 
+	/* When core.ignorecase=true, determine if a directory of the same name but differing
+	 * case already exists within the Git repository.  If it does, ensure the directory
+	 * case of the file being added to the repository matches (is folded into) the existing
+	 * entry's directory case.
+	 */
+	if (ignore_case) {
+		const char *startPtr = ce->name;
+		const char *ptr = startPtr;
+		while (*ptr) {
+			while (*ptr && *ptr != '/')
+				++ptr;
+			if (*ptr == '/') {
+				struct cache_entry *foundce;
+				++ptr;
+				foundce = index_name_exists(&the_index, ce->name, ptr - ce->name, ignore_case);
+				if (foundce) {
+					memcpy((void*)startPtr, foundce->name + (startPtr - ce->name), ptr - startPtr);
+					startPtr = ptr;
+				}
+			}
+		}
+	}
+
 	alias = index_name_exists(istate, ce->name, ce_namelen(ce), ignore_case);
 	if (alias && !ce_stage(alias) && !ie_match_stat(istate, alias, st, ce_option)) {
 		/* Nothing changed, really */
-- 
1.7.1.402.gf1eeb
Johannes Sixt· Aug 16, 2010, 19:38 UTC · re: Johannes Sixt · lore

[PATCH 6/6] Support case folding in git fast-import when core.ignorecase=true

From: Joshua Jensen <jjensen@workspacewhiz.com>

When core.ignorecase=true, imported file paths will be folded to match existing directory case.

Signed-off-by: Joshua Jensen <jjensen@workspacewhiz.com>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 fast-import.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
Show changes to fast-import.c +4 −3
diff --git a/fast-import.c b/fast-import.c
index 1e5d66e..82df00e 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -156,6 +156,7 @@ Format of STDIN stream:
 #include "csum-file.h"
 #include "quote.h"
 #include "exec_cmd.h"
+#include "dir.h"
 
 #define PACK_ID_BITS 16
 #define MAX_PACK_ID ((1<<PACK_ID_BITS)-1)
@@ -1461,7 +1462,7 @@ static int tree_content_set(
 
 	for (i = 0; i < t->entry_count; i++) {
 		e = t->entries[i];
-		if (e->name->str_len == n && !strncmp(p, e->name->str_dat, n)) {
+		if (e->name->str_len == n && !strncmp_icase(p, e->name->str_dat, n)) {
 			if (!slash1) {
 				if (!S_ISDIR(mode)
 						&& e->versions[1].mode == mode
@@ -1527,7 +1528,7 @@ static int tree_content_remove(
 
 	for (i = 0; i < t->entry_count; i++) {
 		e = t->entries[i];
-		if (e->name->str_len == n && !strncmp(p, e->name->str_dat, n)) {
+		if (e->name->str_len == n && !strncmp_icase(p, e->name->str_dat, n)) {
 			if (!slash1 || !S_ISDIR(e->versions[1].mode))
 				goto del_entry;
 			if (!e->tree)
@@ -1577,7 +1578,7 @@ static int tree_content_get(
 
 	for (i = 0; i < t->entry_count; i++) {
 		e = t->entries[i];
-		if (e->name->str_len == n && !strncmp(p, e->name->str_dat, n)) {
+		if (e->name->str_len == n && !strncmp_icase(p, e->name->str_dat, n)) {
 			if (!slash1) {
 				memcpy(leaf, e, sizeof(*leaf));
 				if (e->tree && is_null_sha1(e->versions[1].sha1))
-- 
1.7.1.402.gf1eeb
Robert Buck· Aug 17, 2010, 19:36 UTC · re: Johannes Sixt · lore

Re: [PATCH 0/6] Extensions of core.ignorecase=true support

While I tend to agree with case-insensitive searches, I would tend to question the use of a non-case-preserving / last-use methodology reminiscent of the days of DOS. I was never terribly fond of DOS nor of Windows for Workgroups, and this change smacks of that. That said, as an algorithm it is one legitimate option, I just would not tend to use it.

Have you thought of other approaches that are case-preserving / case-insensitive (modern Windows implementations and Mac OS X) or of case-sensitive (UNIX)? What about giving the user the choice?

Looking back at how flexible and simple folks here on this list made the EOL support (the best I have seen to date) what if you followed a similar approach and provided the flexibility to the users by giving the options to control the behavior themselves, then fall back to reasonable defaults if left unspecified?

I would imagine having two properties would be somewhat more useful:

core.casepreserving=true|false core.caseinsensitive=true|false

For Unix-centric environments the defaults would be true & false, respectively. For mixed Windows/Unix environments users would configure it as true & true. A combination of false & true, respectively, would be appropriate for DOS users, but this may be another option for mixed environments. A combination of false & false would be impossible by definition.

What this would mean is that searches in the purely Unix world would store and match sensitively; but in such an environment there may be issues should any Windows users attempt to work with the repository if two files having a similar names existed. For Windows users the last read operation would overwrite the first (not a good thing).

For mixed configurations there are two options, the Windows/Mac option, and the DOS option.

The nice thing about case-insensitivity is that when requesting a file by name any capitalization can be used. The bad thing, well come to think of it, there is none that I can think of, but for those more religious than I about Unix they may cite otherwise.

-Bob
On Mon, Aug 16, 2010 at 3:38 PM, Johannes Sixt <j6t@kdbg.org> wrote:
Show 52 quoted lines
> The following patch series extends the core.ignorecase=true support to
> handle case insensitive comparisons for the .gitignore file, git status,
> and git ls-files.  git add and git fast-import will fold the case of the
> file being added, matching that of an already added directory entry.  Case
> folding is also applied to git fast-import for renames, copies, and deletes.
>
> The most notable benefit, IMO, is that the case of directories in the
> worktree does not matter if, and only if, the directory exists already in
> the index with some different case variant.  This helps applications on
> Windows that change the case even of directories in unpredictable ways.
> Joshua mentioned Perforce as the primary example.
>
> Concerning the implementation, Joshua explained when he initially submitted
> the series to the msysgit mailing list:
>
>  git status and add both use an update made to name-hash.c where
>  directories, specifically names with a trailing slash, can be looked up
>  in a case insensitive manner. After trying a myriad of solutions, this
>  seemed to be the cleanest. Does anyone see a problem with embedding the
>  directory names in the same hash as the file names? I couldn't find one,
>  especially since I append a slash to each directory name.
>
>  The git add path case folding functionality is a somewhat radical
>  departure from what Git does now. It is described in detail in patch 5.
>  Does anyone have any concerns?
>
> I support the idea of this patch, and I can confirm that it works: I've
> used this series in production both with core.ignorecase set to true and
> to false, and in the former case, with directories and files with case
> different from the index.
>
> Joshua Jensen (6):
>  Add string comparison functions that respect the ignore_case
>    variable.
>  Case insensitivity support for .gitignore via core.ignorecase
>  Add case insensitivity support for directories when using git status
>  Add case insensitivity support when using git ls-files
>  Support case folding for git add when core.ignorecase=true
>  Support case folding in git fast-import when core.ignorecase=true
>
>  dir.c         |  105 ++++++++++++++++++++++++++++++++++++++++++++++----------
>  dir.h         |    4 ++
>  fast-import.c |    7 ++--
>  name-hash.c   |   72 ++++++++++++++++++++++++++++++++++++++-
>  read-cache.c  |   23 ++++++++++++
>  5 files changed, 188 insertions(+), 23 deletions(-)
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Johannes Sixt· Aug 17, 2010, 21:20 UTC · re: Robert Buck · lore

Re: [PATCH 0/6] Extensions of core.ignorecase=true support

On Dienstag, 17. August 2010, Robert Buck wrote:
> While I tend to agree with case-insensitive searches, I would tend to
> question the use of a non-case-preserving / last-use methodology
> reminiscent of the days of DOS.

There is no "last-use" involved. Everything's rather "first-use", i.e., case-preserving.

> The nice thing about case-insensitivity is that when requesting a file
> by name any capitalization can be used. The bad thing, well come to
> think of it, there is none that I can think of, but for those more
> religious than I about Unix they may cite otherwise.
What do you mean by "requesting a file"?

core.ignorecase is purely about the worktree and the transition of files from the worktree to the index. It is *not* involved when files are moved from the index or the repository to the worktree. In particular, it is not used when you give a pathspec to limit 'git log' results. (Joshua proposed a change where core.ignorecase would also kick in in this case as well, but this change is not included in this series, and I would not agree to it.)

-- Hannes
Robert Buck· Aug 18, 2010, 02:41 UTC · re: Johannes Sixt · lore

Re: [PATCH 0/6] Extensions of core.ignorecase=true support

So apparently when core.ignorecase=true, this really means casepreserving=false, casesensitive=true. Yet when core.ignorecase=false it actually means casepreserving=true, casesensitive=true. That's what I infer from the git-config documentation. Would you agree?

On Tue, Aug 17, 2010 at 5:20 PM, Johannes Sixt <j6t@kdbg.org> wrote:
Show 21 quoted lines
> On Dienstag, 17. August 2010, Robert Buck wrote:
>> While I tend to agree with case-insensitive searches, I would tend to
>> question the use of a non-case-preserving / last-use methodology
>> reminiscent of the days of DOS.
>
> There is no "last-use" involved. Everything's rather "first-use", i.e.,
> case-preserving.
>
>> The nice thing about case-insensitivity is that when requesting a file
>> by name any capitalization can be used. The bad thing, well come to
>> think of it, there is none that I can think of, but for those more
>> religious than I about Unix they may cite otherwise.
>
> What do you mean by "requesting a file"?
>
> core.ignorecase is purely about the worktree and the transition of files from
> the worktree to the index. It is *not* involved when files are moved from the
> index or the repository to the worktree. In particular, it is not used when
> you give a pathspec to limit 'git log' results. (Joshua proposed a change
> where core.ignorecase would also kick in in this case as well, but this
> change is not included in this series, and I would not agree to it.)

So what I am hearing is that unless one sets core.ignorecase, in mixed environments you are in for a world of hurt; you'd end up with Foo and foo from the Unix side of the house, and on Macs or Windows the last file materialized from the index or repository into the working directory would clobber the first one materialized, potentially introducing relatively quiescent bugs that could make their way into production environments.

Johannes Sixt· Aug 18, 2010, 18:31 UTC · re: Robert Buck · lore

Re: [PATCH 0/6] Extensions of core.ignorecase=true support

On Mittwoch, 18. August 2010, Robert Buck wrote:
> So apparently when core.ignorecase=true, this really means
> casepreserving=false, casesensitive=true.

No. When an entry enters the index from the worktree, an existing entry is reused and the index entry's case is preserved, where existence is determined in a case-insensitive manner[*]; if the entry is new, the case is preserved.

In the oppsite direction, case preservation depends entirely on the capabilities of the file system.

[*] The new part of this series is that this case-insensitive existence test happens on for the entire path, and not just the file name part.

Show 5 quoted lines
> So what I am hearing is that unless one sets core.ignorecase, in mixed
> environments you are in for a world of hurt; you'd end up with Foo and
> foo from the Unix side of the house, and on Macs or Windows the last
> file materialized from the index or repository into the working
> directory would clobber the first one materialized,

core.ignorecase is not designed to help this case. It is a "Doctor, it hurts when I poke myself in the eye" problem. Don't have both foo and Foo in your repository if you go cross-platform. The git repository format is not designed to treat them as identical, and no configuration option alters this.

-- Hannes
Junio C Hamano· Aug 22, 2010, 07:23 UTC · re: Johannes Sixt · lore

Re: [PATCH 0/6] Extensions of core.ignorecase=true support

Johannes Sixt <j6t@kdbg.org> writes:
> I support the idea of this patch, and I can confirm that it works: I've
> used this series in production both with core.ignorecase set to true and
> to false, and in the former case, with directories and files with case
> different from the index.
Thanks

← back to recent threads