t9200 still failing...

I got sidetracked by work and buying a house, so I didn't get to mention it at the time the attempted fix was committed, but t/t9200- git-cvsexportcommit.sh still is failing on test 8, even in latest master. I'm still getting the message about a file being in my .gitignore:

What I don't understand is that there are no .gitignore files in the test repo and the .git/info/exclude file only has comments. This seems like it might actually be a subtle bug in git-add, although I'm betting it's more OS X filesystem oddness. I'm trying to look into it, but thought that maybe someone with more experience with the code might catch it first.

Things I've discovered:
- A "git status" appropriately shows Å as untracked.
- Terminal.app seems to ignore attempts to type Å.
- Using "git add . && git commit" by hand in the test repo seems to  
work (using git version 1.5.0.rc2.g73a2)

~~ Brian

On 03.02.2007, at 16:56, Brian Gernhardt wrote:

That was already discussed a lot. Any filename test on OSX with a HFS 
+ filesystem containing characters with a different UTF-8-NFC and  
UTF-8-NFD will make such a test fail. If you are using OSX, you might  
want to use UnicodeChecker to see the encoding difference for such  
characters. If you want to make such tests pass, either use  
characters with only one UTF-8 encoding or use a UFS partition to run  
such tests.

	Wolfgang

On Feb 3, 2007, at 11:17 AM, Wolfgang Fischer wrote:

I understand why that causes git-status to continually display "gitweb/test/Märchen" as an untracked file, and I'm fine with that (for the most part). But I don't understand why that causes "git add Å/goo/a/b/c/d/e/f/g/h/i/j/k/l/m/n/o/p/q/r/s/t/u/v/w/x/y/z/å/ä/ö/ gårdetsågårdet.txt" to show a message about gitignore, especially when a "git add ." will pick it up just fine. I'd argue that it's a bug in git, personally, and I can't figure out why it happens.

In any case, to get the test to pass we should probably just put a "- f" on the "git add", at which point it works just fine.

~~ Brian

Without -f, the UTF-8 named files are ignored (supposedly via .gitignore) on systems with a different UTF-8 encoding. This patch makes the test pass on OS X on HFS+, in particular.

Signed-off-by: Brian Gernhardt <benji@silverinsanity.com>
---
 
 I still think that the behavior of git-add here is an error.  The file
 is NOT in any ignore file and so should not be marked as such. However,
 this allows me to finally use "make test && sudo make install".

 t/t9200-git-cvsexportcommit.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Brian Gernhardt <benji@silverinsanity.com> writes:

Can you describe why git-add finds the (presumably mangled by HFS) path in ".gitignore"? If we had some default pattern in info/exclude that is installed in the trash test repository I would understand that a mangled path could happen to match it, but I do not think we do not have any exclude pattern by default.

Unless/until you know why git-add thinks it is ignored,...

... I think this change means you are installing something the existing test knows to be broken, which is not very pretty.

On Feb 3, 2007, at 2:42 PM, Junio C Hamano wrote:

It occurs because of the normalization issue on HFS+. git-add compares the pathspecs given on the command line to a directory tree read from disk. The pathspec is in NFC, and the tree is in NFD. When it tries to find the pathspec in the tree, it fails because of that. When it checks to see if the file exists, HFS+ converts the pathspec to NFD transparently. And since the file exists, but wasn't read by read_directory, git thinks it was because of an ignore file.

The test finds that the ignore code is slightly broken on HFS+, which is not what it thinks it's testing. And since the ignore files are empty, it should not adversely affect any other platforms.

The only solution I can think of is to make dir.c:match_one() unicode- aware. Which I'm working on to see if it will work, but don't know if you want to include that complication.

~~ Brian

Brian Gernhardt <benji@silverinsanity.com> writes:

A filesystem that reports success on creat(path) and then does not return that path to later readdir() from that directory is broken from git's point of view. At least that has been the definition so far.

I am not sure offhand the implications of the change you propose to make to match_one(), but it could also be the place to handle case-challenged filesystems.

But even if you do that, I wonder what your plan would be to handle something like "git add .". read_directory_recursive() asks readdir() for existing pathnames, and we expect that can be used as parameter to add_file_to_index and then eventually we can call creat() or symlink() with it, so it also needs to be taught to behave consistently with respect to what your updated match_one() does.

On Feb 3, 2007, at 3:51 PM, Junio C Hamano wrote:

I agree, it's fairly idiotic and obtuse. By using the great Google, I've seen lots of other people complaining about it as well.

I would simply like match_one to agree with the file system as to what is a match. On OS X that would mean that both normalized forms of UTF-8 match. I'm trying it as a proof-of-concept mostly to see what kind of effects it would have. match_one only seems to be used in match_pathspec, which is only used by add and rm... And I think that getting it to match the behavior of the OS would make things less surprising for the user.

"git add ." works just fine, as it reads the name of the file from disk which is already in the form HFS+ accepts. The only confusion exists when comparing data from the user to data from disk.

In any case, I don't appear to understand the proper incantations to bend ICU to my will, as it either doesn't change anything or breaks git completely. Perhaps further attempts will help, but my lack of experience with the API is a serious hinderance, and I can't find any other way to do the normalization.

~~ Brian

Brian Gernhardt <benji@silverinsanity.com> writes:

I am not sure if we are really agreeing, but I'll let it pass.

I wonder even if that is true.

Luckily or unluckily, I do not have access to a system with broken readdir() vs creat() confusion, so I cannot test it myself, but I suspect that this sequence may not work as expected:

	#!/bin/sh

	pathname='a pathname that canonicalize differently from original'
	rm -fr testrepo
	mkdir testrepo
        cd testrepo
        git init-db
	echo hello >"$pathname"
        git add -f .
        git ls-files -s "$pathname"

If my reading of what you said is correct, then in the above sequence:

 (1) The shell creates, via redirection of output of echo, a file
     but using canonicalized string, which is different from
     what the user gave;

 (2) "git add" will ask readdir(2) to get inventory of files in
     the working directory, and grabs canonicalized string;

 (3) "git add" uses that canonicalized string, open(2)s, mmap(2)s and
     hashes the blob contents and registers that object name
     under the canonicazlied string in the index;

 (4) "git ls-files" tries to look up the index with the string
     user used to create the file in (1), which is without the
     canonicalization.

I think it is fairly idiotic and obtuse for a filesystem to treat pathnames anything but a random sequence of bytes that is slash separated and NUL terminated. I would need a really hard convincing to buy any path munging on the git side to match whatever a stupid filesystem does, especially because we do not live in the ideal Unicode/utf-8 only world.

Junio C Hamano <junkio@cox.net> writes:

Addendum.

This is not to make any point but purely for my own education, but I wonder what would happen on HFS+ to the following sequence, which does not involve any git operation:

   #!/bin/sh
   LANG=C LC_ALL=C
   export LANG LC_ALL

   pathname='a pathname that canonicalize differently from original'
   rm -fr testrepo
   mkdir testrepo
   cd testrepo
   echo hello >"$pathname"
   /bin/ls | fgrep -e "$pathname"

Doesn't the grep fail to find the file that was successfully created in the previous step?

Junio C Hamano <junkio@cox.net> wrote:

Indeed. I stole the path "gitweb/test/M\303\244rchen" from git.git and ran your script on Mac OS X:

  $ cat jt.sh; echo; sh jt.sh 
  #!/bin/sh
  LANG=C LC_ALL=C
  export LANG LC_ALL
  
  rm -fr testrepo
  mkdir testrepo
  cd testrepo
  
  pathname='Märchen'
  echo $pathname
  echo hello >"$pathname"
  /bin/ls | fgrep -e "$pathname" | wc -l
  
  pathname='Mrchen'
  echo $pathname
  echo hello >"$pathname"
  /bin/ls | fgrep -e "$pathname" | wc -l
  Märchen
         0
  Mrchen
         1

The first pathname= line in the above script is the *exact* byte sequence which appears in gitweb/test's tree. I apologize if my email chain screws up the line.

Shawn has already provided an example of what happens to a UTF8 string on OS X, so I won't provide a duplicate.

On Feb 3, 2007, at 6:50 PM, Junio C Hamano wrote:

That's what I was agreeing with. It drives me crazy that HFS+ is case-insensitive and now does bad things to UTF8 strings. And I didn't think that doing munging similar to the FS in git is a good idea for Git in general, but might be useful for a few people like myself to have. And it wouldn't break anything that isn't already broken.

Unfortunately my lack of understanding of ICU means I can't actually get the code to do anything useful, so it's a moot point.

~~ Brian

And I forgot to re-iterate that the "-f" in the test only bypasses the .gitignore machinery which is perfectly safe in a test environment where the ignore files are empty, and makes the git- cvsexportcommit test only git-cvsexportcommit. If you wish to add a test for filesystem stupidity separately, that's a different issue.

I really think the patch is useful and allows the test to pass even on systems with idiotic filesystems and doesn't make it vulnerable to any bugs in cvsexportcommit. (Bugs in git-add, maybe, but that's not what it's testing.)

~~ Brian

Brian Gernhardt <benji@silverinsanity.com> writes:

In reality, I find highly valuable that our tests find new bugs introduced in unexpected places. I agree that the person who wrote t9200 did not _mean_ to test "git add", but it ended up helping us identify the problematic behaviour between creat() and readdir() on HFS+ affects "git add".

We at least know that the test as written has a problem in an environment where "touch '$p'; ls | fgrep '$p'" fails, and have a clear understand why it fails. I think that knowledge is valuable -- adding '-f' to the test may hide potential problems we might have with "git add" in other environments.

So how about doing this instead? It tests if the filesystem has that particular issue we know "git add" has a problem with, and skips the test in such an environment.

---

 t/t9200-git-cvsexportcommit.sh |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

On Feb 4, 2007, at 8:30 PM, Junio C Hamano wrote:

Alright, I understand that. I just meant that since we've identified it, the workaround didn't matter much.

Yes, something like this is fine. The patch looks good, but I couldn't get it to apply and don't have the time to fix it at the moment. If you *need* me to test it, I'll do so tomorrow or Tuesday.

~~ Brian

On Feb 4, 2007, at 8:30 PM, Junio C Hamano wrote:

I'd be tempted to add a "test_expect_success 'skipping UTF8 test on broken filesystem'", but I finally tested it (Sunday and Monday are my weekends) and it works as expected. Thanks.

~~ Brian

On Feb 3, 2007, at 2:06 PM, Wolfgang Fischer wrote:

The true solution was to read the code more completely. The way git tests for things that are ignored is to read the directory and compare it to the pathspecs from the command line. If we don't find a match, it's obviously because the file was in an ignore file. Unfortunately, in OS X it's not obvious because of the mismatch between NFC and NFD on different file systems.

The solution, as I see it, is to use a Unicode-aware comparison in dir.c:match_one(). ICU appears to both be available on OS X (although without headers) and have the needed functions. I'm unfamiliar with both ICU and this chunk of the git internals, but I'll work on a proof of concept patch.

~~ Brian