t9200 still failing...

t9200 still failing...

[Brian Gernhardt]

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:

> The following paths are ignored by one of your .gitignore files:
> Å/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
> Use -f if you really want to add them.

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

Re: t9200 still failing...

[Wolfgang Fischer]

On 03.02.2007, at 16:56, Brian Gernhardt wrote:
> 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.

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

Re: t9200 still failing...

[Brian Gernhardt]

On Feb 3, 2007, at 11:17 AM, Wolfgang Fischer 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.

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

[PATCH] Use "-f" when adding files with odd names in t9200.

[Brian Gernhardt]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=unknown-8bit, Size: 1751 bytes --]

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(-)

diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index c443f32..6955cdd 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -174,9 +174,9 @@ test_expect_success \
      'File with non-ascii file name' \
      'mkdir -p Å/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/å/ä/ö &&
       echo Foo >Å/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 &&
-      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 &&
+      git add -f Å/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 &&
       cp ../test9200a.png Å/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.png &&
-      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.png &&
+      git add -f Å/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.png &&
       git commit -a -m "Går det så går det" && \
       id=$(git rev-list --max-count=1 HEAD) &&
       (cd "$CVSWORK" &&
-- 
1.5.0.rc3.22.g5057

Re: t9200 still failing...

[Brian Gernhardt]

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

> A simple test would be to create a UFS image, mount it somewhere,  
> and run the tests inside the mounted UFS image. If the test  
> succeeds, there is a getdirent() somewhere in the codepath, where  
> you are not expecting it. OTOH we can assume already, that there is  
> a getdirents() somewhere, so you might just start with a debugger   
> with an appropriate breakpoint.

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

Re: [PATCH] Use "-f" when adding files with odd names in t9200.

[Junio C Hamano]

Brian Gernhardt <benji@silverinsanity.com> writes:

> 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.

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,...

>  However,
>  this allows me to finally use "make test && sudo make install".

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

Re: [PATCH] Use "-f" when adding files with odd names in t9200.

[Brian Gernhardt]

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

> 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,...

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.

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

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

Re: [PATCH] Use "-f" when adding files with odd names in t9200.

[Junio C Hamano]

Brian Gernhardt <benji@silverinsanity.com> writes:

> 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.

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.

Re: [PATCH] Use "-f" when adding files with odd names in t9200.

[Brian Gernhardt]

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

> 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 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 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.

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.

> 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.

"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

Re: [PATCH] Use "-f" when adding files with odd names in t9200.

[Junio C Hamano]

Brian Gernhardt <benji@silverinsanity.com> writes:

> On Feb 3, 2007, at 3:51 PM, Junio C Hamano wrote:
>
>> 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 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 am not sure if we are really agreeing, but I'll let it pass.

> "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.

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.

Re: [PATCH] Use "-f" when adding files with odd names in t9200.

[Junio C Hamano]

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

> ... 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"

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?

Re: [PATCH] Use "-f" when adding files with odd names in t9200.

[Shawn O. Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> 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:
> 
> Doesn't the grep fail to find the file that was successfully
> created in the previous step?

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.

Re: [PATCH] Use "-f" when adding files with odd names in t9200.

[Brian Gernhardt]

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:

> 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.

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

Re: [PATCH] Use "-f" when adding files with odd names in t9200.

[Brian Gernhardt]

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

Re: [PATCH] Use "-f" when adding files with odd names in t9200.

[Junio C Hamano]

Brian Gernhardt <benji@silverinsanity.com> writes:

> 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.)

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(-)

diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index c443f32..4efa0c9 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -169,6 +169,16 @@ test_expect_success \
       test "$(echo $(sort "G g/CVS/Entries"|cut -d/ -f2,3,5))" = "with spaces.png/1.2/-kb with spaces.txt/1.2/"
       )'
 
+# Some filesystems mangle pathnames with UTF-8 characters --
+# check and skip
+if p="Å/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/å/ä/ö" &&
+	mkdir -p "tst/$p" &&
+	date >"tst/$p/day" &&
+	found=$(find tst -type f -print) &&
+	test "z$found" = "ztst/$p/day" &&
+	rm -fr tst
+then
+
 # This test contains UTF-8 characters
 test_expect_success \
      'File with non-ascii file name' \
@@ -184,6 +194,10 @@ test_expect_success \
       test "$(echo $(sort Å/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/å/ä/ö/CVS/Entries|cut -d/ -f2,3,5))" = "gårdetsågårdet.png/1.1/-kb gårdetsågårdet.txt/1.1/"
       )'
 
+fi
+
+rm -fr tst
+
 test_expect_success \
      'Mismatching patch should fail' \
      'date >>"E/newfile5.txt" &&

Re: [PATCH] Use "-f" when adding files with odd names in t9200.

[Brian Gernhardt]

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

> 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".

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

> 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.

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

Re: [PATCH] Use "-f" when adding files with odd names in t9200.

[Brian Gernhardt]

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

> 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.

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