Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Mark Levedahl]

Beginning with the referenced commit, the git project cannot be checked 
out on Cygwin (and I assume cannot be checked out on Windows using 
msysgit, though I have not verified this) as this commit introduces the 
file "t/5100/nul." On Windows, the file name "nul" is reserved, 
regardless of path, and cannot be created or deleted. It serves 
essentially the same function as /dev/null.

As a for instance of the troubles:

git>git checkout -f origin/master
Previous HEAD position was a2f5be5... Merge branch 
'jk/maint-send-email-compose' into maint
error: git-checkout-index: unable to create file t/t5100/nul (File exists)

As this commit is part of the published master branch, I am not sure the 
correct resolution: leaving this commit in place means that any commit 
between it and a commit fixing this will always cause an error on Cygwin 
/ Windows. Of course, it *is* on the published master branch.

Mark

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Johannes Schindelin]

Hi,

On Mon, 26 May 2008, Mark Levedahl wrote:

> Beginning with the referenced commit, the git project cannot be checked 
> out on Cygwin (and I assume cannot be checked out on Windows using 
> msysgit, though I have not verified this) as this commit introduces the 
> file "t/5100/nul." On Windows, the file name "nul" is reserved, 
> regardless of path, and cannot be created or deleted. It serves 
> essentially the same function as /dev/null.

Even when referencing the full (or a relative) path?  That's bad!

> As a for instance of the troubles:
> 
> git>git checkout -f origin/master
> Previous HEAD position was a2f5be5... Merge branch
> 'jk/maint-send-email-compose' into maint
> error: git-checkout-index: unable to create file t/t5100/nul (File exists)
> 
> As this commit is part of the published master branch, I am not sure the 
> correct resolution: leaving this commit in place means that any commit 
> between it and a commit fixing this will always cause an error on Cygwin 
> / Windows. Of course, it *is* on the published master branch.

That's the case for all regressions: we do not rewrite history for them.

As for the resolution, could you quickly try the 'my-next' branch of 
git://repo.or.cz/git/dscho.git?

If that works, I'll send a proper patch to Junio.

Ciao,
Dscho

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Mark Levedahl]

Johannes Schindelin wrote:
>> As a for instance of the troubles:
>>
>> git>git checkout -f origin/master
>> Previous HEAD position was a2f5be5... Merge branch
>> 'jk/maint-send-email-compose' into maint
>> error: git-checkout-index: unable to create file t/t5100/nul (File exists)
>>
>> As this commit is part of the published master branch, I am not sure the 
>> correct resolution: leaving this commit in place means that any commit 
>> between it and a commit fixing this will always cause an error on Cygwin 
>> / Windows. Of course, it *is* on the published master branch.
>>     
>
> That's the case for all regressions: we do not rewrite history for them.
>
>   
I understand that, and the reasons: however, as this leads to a long 
sequence of commits pointing to *illegal* trees, not just trees with bad 
code, a different policy might be in order here. Or, it might not.
> As for the resolution, could you quickly try the 'my-next' branch of 
> git://repo.or.cz/git/dscho.git?
>
>   
I can check that branch out, but don't get very far in the tests:

*** t0004-unwritable.sh ***
*   ok 1: setup
* FAIL 2: write-tree should notice unwritable repository


                (
                        chmod a-w .git/objects
                        test_must_fail git write-tree
                )
                status=$?
                chmod 775 .git/objects
                (exit $status)


* FAIL 3: commit should notice unwritable repository


                (
                        chmod a-w .git/objects
                        test_must_fail git commit -m second
                )
                status=$?
                chmod 775 .git/objects
                (exit $status)


* FAIL 4: update-index should notice unwritable repository


                (
                        echo a >file &&
                        chmod a-w .git/objects
                        test_must_fail git update-index file
                )
                status=$?
                chmod 775 .git/objects
                (exit $status)


* FAIL 5: add should notice unwritable repository


                (
                        echo b >file &&
                        chmod a-w .git/objects
                        test_must_fail git add file
                )
                status=$?
                chmod 775 .git/objects
                (exit $status)


* failed 4 among 5 test(s)
make[1]: *** [t0004-unwritable.sh] Error 1
make[1]: Leaving directory `/usr/src/dscho
make: *** [test] Error 2

I don't have access to a linux box today, so I can't manipulate master 
to find if that branch with your patch would work right now.

Mark

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Johannes Schindelin]

Hi,

On Mon, 26 May 2008, Mark Levedahl wrote:

> Johannes Schindelin wrote:
>
> > You wrote:
> >
> > > As this commit is part of the published master branch, I am not sure 
> > > the correct resolution: leaving this commit in place means that any 
> > > commit between it and a commit fixing this will always cause an 
> > > error on Cygwin / Windows. Of course, it *is* on the published 
> > > master branch.
> >
> > That's the case for all regressions: we do not rewrite history for 
> > them.
>
> I understand that, and the reasons: however, as this leads to a long 
> sequence of commits pointing to *illegal* trees, not just trees with bad 
> code, a different policy might be in order here. Or, it might not.

I fail to see how Cygwin is so special as to merit a falsification of 
history.

> > As for the resolution, could you quickly try the 'my-next' branch of 
> > git://repo.or.cz/git/dscho.git?
>
> I can check that branch out, but don't get very far in the tests:
> 
> *** t0004-unwritable.sh ***
> *   ok 1: setup
> * FAIL 2: write-tree should notice unwritable repository

Sorry.  Was worth a try.

> I don't have access to a linux box today, so I can't manipulate master 
> to find if that branch with your patch would work right now.

Sure you can.  You should be able to "git mv t/t5100/nul t/t5100/nul-file" 
and then editing t/t5100-*.sh to refer to nul-file instead of nul.

Hth,
Dscho

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Mark Levedahl]

Johannes Schindelin wrote:
> Hi,
>
>
>   
> I fail to see how Cygwin is so special as to merit a falsification of 
> history.
>
>   
>> I don't have access to a linux box today, so I can't manipulate master 
>> to find if that branch with your patch would work right now.
>>     
>
> Sure you can.  You should be able to "git mv t/t5100/nul t/t5100/nul-file" 
> and then editing t/t5100-*.sh to refer to nul-file instead of nul.
>
> Hth,
> Dscho
>   
The above two points are related: the current master causes git to abort 
before writing the index:

 >git checkout -f origin/master
error: git-checkout-index: unable to create file t/t5100/nul (File exists)
 >git mv t/t5100/nul t/t5100/nul-file
fatal: not under version control, source=t/t5100/nul, 
destination=t/t5100/nul-file

So, there is now a range of git's history that is unusable (and 
non-bisectable) on Windows, at least from the porcelain. And apparently, 
somewhere in that unusable history, a change was introduced that causes 
test failure on Cygwin. Great...

Mark

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Johannes Schindelin]

Hi,

On Mon, 26 May 2008, Mark Levedahl wrote:

> So, there is now a range of git's history that is unusable (and 
> non-bisectable) on Windows, at least from the porcelain. And apparently, 
> somewhere in that unusable history, a change was introduced that causes 
> test failure on Cygwin. Great...

Oh, for the love of God!  It is not like we will not fix this problem 
eventually!  Screwing a lot of users by rewriting history, just because of 
Windows, which we need too many ugly work-arounds in Git's source code for 
anyway, is _not_ an option.

Or would you suggest to scrap almost the complete history of Git just 
because most of it does not compile on platform XYZ, while the initial 
revision did?  Exactly.

Ciao,
Dscho

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Johannes Schindelin]

Hi,

On Mon, 26 May 2008, Mark Levedahl wrote:

> >git checkout -f origin/master
> error: git-checkout-index: unable to create file t/t5100/nul (File exists)
> >git mv t/t5100/nul t/t5100/nul-file
> fatal: not under version control, source=t/t5100/nul,
> destination=t/t5100/nul-file

I should not do this, because you cost me already too much time with your 
outrageous proposal to rewrite history just for that stupid platform 
called Windows, whose users seem to complain more than actually 
contribute, but here it goes:

http://repo.or.cz/w/git/dscho.git?a=shortlog;h=refs/heads/nul-fix

The corresponding patch:

-- snipsnap --
[PATCH] Fix t5100 for Windows

On Windows, "nul" is not allowed as a file name.  So bend over for that
platform even more than we do already.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t5100-mailinfo.sh        |    4 ++--
 t/t5100/{nul => nul-plain} |  Bin 91 -> 91 bytes
 2 files changed, 2 insertions(+), 2 deletions(-)
 rename t/t5100/{nul => nul-plain} (100%)

diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index a8b78eb..577ecc2 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -27,8 +27,8 @@ done
 
 test_expect_success 'respect NULs' '
 
-	git mailsplit -d3 -o. ../t5100/nul &&
-	cmp ../t5100/nul 001 &&
+	git mailsplit -d3 -o. ../t5100/nul-plain &&
+	cmp ../t5100/nul-plain 001 &&
 	(cat 001 | git mailinfo msg patch) &&
 	test 4 = $(wc -l < patch)
 
diff --git a/t/t5100/nul b/t/t5100/nul-plain
similarity index 100%
rename from t/t5100/nul
rename to t/t5100/nul-plain
-- 
1.5.6.rc0.175.gdd78

[PATCH] Makefile: wt-status.h is also a lib header

[Johannes Schindelin]

When a struct in wt-status.h changes, many files need to be rebuilt.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Hannes noticed that this patch was not yet accepted (or not
	even submitted, I do not remember).

 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index dbfa2b6..aced19b 100644
--- a/Makefile
+++ b/Makefile
@@ -376,6 +376,7 @@ LIB_H += tree-walk.h
 LIB_H += unpack-trees.h
 LIB_H += utf8.h
 LIB_H += levenshtein.h
+LIB_H += wt-status.h
 
 LIB_OBJS += alias.o
 LIB_OBJS += alloc.o
@@ -1129,7 +1130,6 @@ git-http-push$X: revision.o http.o http-push.o $(GITLIBS)
 
 $(LIB_OBJS) $(BUILTIN_OBJS): $(LIB_H)
 $(patsubst git-%$X,%.o,$(PROGRAMS)): $(LIB_H) $(wildcard */*.h)
-builtin-revert.o wt-status.o: wt-status.h
 
 $(LIB_FILE): $(LIB_OBJS)
 	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(LIB_OBJS)
-- 
1.5.6.rc0.175.gdd78

Re: [PATCH] Makefile: wt-status.h is also a lib header

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> When a struct in wt-status.h changes, many files need to be rebuilt.

What project, that has levenshtein.h, is this patch is for ;-)?

Re: [PATCH] Makefile: wt-status.h is also a lib header

[Johannes Schindelin]

Hi,

On Mon, 26 May 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > When a struct in wt-status.h changes, many files need to be rebuilt.
> 
> What project, that has levenshtein.h, is this patch is for ;-)?

Heh.  Missed that one.  There was a discussion some time ago that the 
rename detection should prefer names with a smaller Levenshtein distance, 
so I implemented that (back when I still had some time).

This one is on top of the current 'next':

-- snipsnap --
[PATCH v2] Makefile: wt-status.h is also a lib header

When a struct in wt-status.h changes, many files need to be rebuilt.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Makefile b/Makefile
index ef5ce5e..ab358b2 100644
--- a/Makefile
+++ b/Makefile
@@ -375,6 +375,7 @@ LIB_H += tree.h
 LIB_H += tree-walk.h
 LIB_H += unpack-trees.h
 LIB_H += utf8.h
+LIB_H += wt-status.h
 
 LIB_OBJS += alias.o
 LIB_OBJS += alloc.o
@@ -1127,7 +1128,6 @@ git-http-push$X: revision.o http.o http-push.o $(GITLIBS)
 
 $(LIB_OBJS) $(BUILTIN_OBJS): $(LIB_H)
 $(patsubst git-%$X,%.o,$(PROGRAMS)): $(LIB_H) $(wildcard */*.h)
-builtin-revert.o wt-status.o: wt-status.h
 
 $(LIB_FILE): $(LIB_OBJS)
 	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(LIB_OBJS)
-- 
1.5.6.rc0.175.gdd78

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Eric Blake]

Johannes Schindelin <Johannes.Schindelin <at> gmx.de> writes:

> On Mon, 26 May 2008, Mark Levedahl wrote:
> 
> > Beginning with the referenced commit, the git project cannot be checked 
> > out on Cygwin (and I assume cannot be checked out on Windows using 
> > msysgit, though I have not verified this) as this commit introduces the 
> > file "t/5100/nul." On Windows, the file name "nul" is reserved, 
> > regardless of path, and cannot be created or deleted. It serves 
> > essentially the same function as /dev/null.
> 
> Even when referencing the full (or a relative) path?  That's bad!

Yes.  And it is not limited to the case-insensitive name NUL; it also covers
names like "aux".  See the very last paragraph in this section:
http://www.gnu.org/software/autoconf/manual/html_node/File-System-Conventions.html#File-System-Conventions

This will affect all versions of MSYS.  However, with cygwin 1.5.x, you can use
the workaround of a managed mount which intentionally (and transparently) munges
such invalid file names so that you can appear to name a file "nul" in spite of
Windows (at the expense of making an already short PATH_MAX of 256 even
shorter).  And if you are willing to experiment with the (still-in-development)
cygwin 1.7.0, this munging is done without even needing a managed mount and
without any penalty to the larger PATH_MAX of 4k.

At any rate, I agree with your patch to rename the file, as well as with your
aversion to rewriting history just so that a checkout on MSYS or a non-managed
mount on older cygwin can do a 'git bisect' that hits the small window of
commits with an invalid tree.

-- 
Eric Blake

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Junio C Hamano]

Mark Levedahl <mlevedahl@gmail.com> writes:

> As this commit is part of the published master branch, I am not sure
> the correct resolution: leaving this commit in place means that any
> commit between it and a commit fixing this will always cause an error
> on Cygwin / Windows. Of course, it *is* on the published master branch.

Some broken filesystems may not be capable of checking out and using
project files.  Too bad.

It's not a big deal.  It is not limited to this project.  We just fix them
or work them around and move on.

Perhaps we should remove the infamous gitweb/test/Märchen file while we
are at it?  I do not think the file is ever used.

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Wincent Colaiuta]

El 28/5/2008, a las 8:12, Junio C Hamano escribió:
> Mark Levedahl <mlevedahl@gmail.com> writes:
>
>> As this commit is part of the published master branch, I am not sure
>> the correct resolution: leaving this commit in place means that any
>> commit between it and a commit fixing this will always cause an error
>> on Cygwin / Windows. Of course, it *is* on the published master  
>> branch.
>
> Some broken filesystems may not be capable of checking out and using
> project files.  Too bad.
>
> It's not a big deal.  It is not limited to this project.  We just  
> fix them
> or work them around and move on.
>
> Perhaps we should remove the infamous gitweb/test/Märchen file while  
> we
> are at it?  I do not think the file is ever used.

I for one would love to see it go, seeing as I live in the ghetto that  
is HFS+ and am constantly annoyed by it cluttering up my status output  
with spurious content.

I understand that the reason it lives in the tree is precisely to  
discover problems with such filesystems, but the problem is well and  
truly discovered by now and I'd much rather see this kind of thing  
tested from within the test suite rather than every time I do "git  
status" or "git checkout".

Cheers,
Wincent

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Lea Wiemann]

Wincent Colaiuta wrote:
> El 28/5/2008, a las 8:12, Junio C Hamano escribió:
>> Perhaps we should remove the infamous gitweb/test/Märchen file
> 
> [...] I'd much rather see this kind of thing  
> tested from within the test suite rather than every time I do "git  
> status" or "git checkout".

I don't believe the Märchen file is actually used in any test code, so 
removing it should be fine.  If/when we actually write test code for 
gitweb, it seems to me that we might as well generate such test files on 
the fly from within the test suite, rather than having them in the file 
system permanently.

Best,

     Lea

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Wincent Colaiuta]

El 28/5/2008, a las 17:53, Lea Wiemann escribió:
> Wincent Colaiuta wrote:
>> El 28/5/2008, a las 8:12, Junio C Hamano escribió:
>>> Perhaps we should remove the infamous gitweb/test/Märchen file
>> [...] I'd much rather see this kind of thing  tested from within  
>> the test suite rather than every time I do "git  status" or "git  
>> checkout".
>
> I don't believe the Märchen file is actually used in any test code,  
> so removing it should be fine.  If/when we actually write test code  
> for gitweb, it seems to me that we might as well generate such test  
> files on the fly from within the test suite, rather than having them  
> in the file system permanently.

Yes, that's exactly what I intended my comment to imply. Test at test  
time, not every time I do "git status" and "git checkout" etc.

Cheers,
Wincent

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Jakub Narebski]

Wincent Colaiuta wrote:

> El 28/5/2008, a las 17:53, Lea Wiemann escribió:
>> Wincent Colaiuta wrote:
>>> El 28/5/2008, a las 8:12, Junio C Hamano escribió:
>>>>
>>>> Perhaps we should remove the infamous gitweb/test/Märchen file
>>>
>>> [...] I'd much rather see this kind of thing  tested from within  
>>> the test suite rather than every time I do "git  status" or "git  
>>> checkout".
>>
>> I don't believe the Märchen file is actually used in any test code,  
>> so removing it should be fine.  If/when we actually write test code  
>> for gitweb, it seems to me that we might as well generate such test  
>> files on the fly from within the test suite, rather than having them  
>> in the file system permanently.
> 
> Yes, that's exactly what I intended my comment to imply. Test at test  
> time, not every time I do "git status" and "git checkout" etc.

I think it is remainder (the whole gitweb/test/ directory) from the
times when gitweb was separate project, and not part of git.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 796 bytes --]

Hi,

On Wed, 28 May 2008, Wincent Colaiuta wrote:

> El 28/5/2008, a las 8:12, Junio C Hamano escribió:
>
> >Perhaps we should remove the infamous gitweb/test/Märchen file while we 
> >are at it?  I do not think the file is ever used.
> 
> I for one would love to see it go, seeing as I live in the ghetto that 
> is HFS+ and am constantly annoyed by it cluttering up my status output 
> with spurious content.
> 
> I understand that the reason it lives in the tree is precisely to 
> discover problems with such filesystems, but the problem is well and 
> truly discovered by now and I'd much rather see this kind of thing 
> tested from within the test suite rather than every time I do "git 
> status" or "git checkout".

Probably you are not enoyed enough to just go and fix it.

Ciao,
Dscho

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Wincent Colaiuta]

El 29/5/2008, a las 15:22, Johannes Schindelin escribió:
> Hi,
>
> On Wed, 28 May 2008, Wincent Colaiuta wrote:
>
>> El 28/5/2008, a las 8:12, Junio C Hamano escribió:
>>
>>> Perhaps we should remove the infamous gitweb/test/Märchen file  
>>> while we
>>> are at it?  I do not think the file is ever used.
>>
>> I for one would love to see it go, seeing as I live in the ghetto  
>> that
>> is HFS+ and am constantly annoyed by it cluttering up my status  
>> output
>> with spurious content.
>>
>> I understand that the reason it lives in the tree is precisely to
>> discover problems with such filesystems, but the problem is well and
>> truly discovered by now and I'd much rather see this kind of thing
>> tested from within the test suite rather than every time I do "git
>> status" or "git checkout".
>
> Probably you are not enoyed enough to just go and fix it.

No, that's not actually the case. In reality I was pleasantly  
surprised when Junio commented that "perhaps we should remove" that  
file; I had always gotten the impression from this list that such a  
change would be unwelcome because it's easier to just blame the users  
of bad filesystems for choosing those filesystems. I also remember a  
comment from Linus to the effect that that file was kept in the tree  
precisely _because_ it helped us discover such file systems.  
Unfortunately I can't find that message right now but I think it was  
about 6 months ago.

If the powers that be will accept a change that removes Märchen I'll  
be more than happy to whip up a patch.

Wincent

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 586 bytes --]

Hi,

On Thu, 29 May 2008, Wincent Colaiuta wrote:

> El 29/5/2008, a las 15:22, Johannes Schindelin escribió:
>
> >Probably you are not enoyed enough to just go and fix it.
> 
> No, that's not actually the case. In reality I was pleasantly surprised 
> when Junio commented that "perhaps we should remove" that file;

You misunderstood me.  I was not talking about "fixing" it by removing the 
file, and papering over the UTF-8 issue on HFS+.

I was talking about fixing it by handling UTF-8 in a way that is 
compatible with (maybe stupid, but that cannot be helped) HFS+.

Ciao,
Dscho

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Wincent Colaiuta]

El 29/5/2008, a las 18:05, Johannes Schindelin escribió:
> Hi,
>
> On Thu, 29 May 2008, Wincent Colaiuta wrote:
>
>> El 29/5/2008, a las 15:22, Johannes Schindelin escribió:
>>
>>> Probably you are not enoyed enough to just go and fix it.
>>
>> No, that's not actually the case. In reality I was pleasantly  
>> surprised
>> when Junio commented that "perhaps we should remove" that file;
>
> You misunderstood me.  I was not talking about "fixing" it by  
> removing the
> file, and papering over the UTF-8 issue on HFS+.
>
> I was talking about fixing it by handling UTF-8 in a way that is
> compatible with (maybe stupid, but that cannot be helped) HFS+.

Ah, ok. That's way beyond my skill set and a completely different  
subject. I was responding to Junio's comment about _removing_ the file.

Wincent

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Steffen Prohaska]

On May 29, 2008, at 4:58 PM, Wincent Colaiuta wrote:

> El 29/5/2008, a las 15:22, Johannes Schindelin escribió:
>>
>>
>> On Wed, 28 May 2008, Wincent Colaiuta wrote:
>>
>>> El 28/5/2008, a las 8:12, Junio C Hamano escribió:
>>>
>>>> Perhaps we should remove the infamous gitweb/test/Märchen file  
>>>> while we
>>>> are at it?  I do not think the file is ever used.
>>>
>>> I for one would love to see it go, seeing as I live in the ghetto  
>>> that
>>> is HFS+ and am constantly annoyed by it cluttering up my status  
>>> output
>>> with spurious content.
>>>
>>> I understand that the reason it lives in the tree is precisely to
>>> discover problems with such filesystems, but the problem is well and
>>> truly discovered by now and I'd much rather see this kind of thing
>>> tested from within the test suite rather than every time I do "git
>>> status" or "git checkout".
>>
>> Probably you are not enoyed enough to just go and fix it.
>

[...]

>
> If the powers that be will accept a change that removes Märchen I'll  
> be more than happy to whip up a patch.


Unicode normalization is tested in t/t0050-filesystem.sh, which
reports on HFS+:

*   still broken 8: rename (silent unicode normalization)
*   still broken 9: merge (silent unicode normalization)

I believe there is no value in keeping gitweb/test/Märchen for the
reason of testing HFS+, so I vote for removing it, unless there
is another good reason for keeping it.

	Steffen

[PATCH] gitweb: Remove gitweb/test/ directory

[Jakub Narebski]

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

Testing if gitweb handles filenames with spaces, filenames with plus
sign ('+') which encodes spaces in CGI parameters (in URLs), and
filenames with Unicode characters should be handled by gitweb tests.

Those files are remainder of the time when gitweb was project on its
own, not a part of git (with its testsuite).

Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
On Sat, 31 May 2008 19:37:48 +0200, Steffen Prohaska wrote:
> On May 29, 2008, at 4:58 PM, Wincent Colaiuta wrote:
>
[...]
>> If the powers that be will accept a change that removes Märchen I'll  
>> be more than happy to whip up a patch.
> 
> Unicode normalization is tested in t/t0050-filesystem.sh, which
> reports on HFS+:
> 
> *   still broken 8: rename (silent unicode normalization)
> *   still broken 9: merge (silent unicode normalization)
> 
> I believe there is no value in keeping gitweb/test/Märchen for the
> reason of testing HFS+, so I vote for removing it, unless there
> is another good reason for keeping it.

So here it is, the patch to remove offending file; well: the whole
gitweb/test/ directory. 

 "gitweb/test/M\303\244rchen" |    2 --
 gitweb/test/file with spaces |    4 ----
 gitweb/test/file+plus+sign   |    6 ------
 3 files changed, 0 insertions(+), 12 deletions(-)
 delete mode 100644 gitweb/test/Märchen
 delete mode 100644 gitweb/test/file with spaces
 delete mode 100644 gitweb/test/file+plus+sign

diff --git "a/gitweb/test/M\303\244rchen" "b/gitweb/test/M\303\244rchen"
deleted file mode 100644
index 8f7a1d3..0000000
--- "a/gitweb/test/M\303\244rchen"
+++ /dev/null
@@ -1,2 +0,0 @@
-Märchen
-Märchen
diff --git a/gitweb/test/file with spaces b/gitweb/test/file with spaces
deleted file mode 100644
index f108543..0000000
--- a/gitweb/test/file with spaces	
+++ /dev/null
@@ -1,4 +0,0 @@
-This
-filename
-contains
-spaces.
diff --git a/gitweb/test/file+plus+sign b/gitweb/test/file+plus+sign
deleted file mode 100644
index fd05278..0000000
--- a/gitweb/test/file+plus+sign
+++ /dev/null
@@ -1,6 +0,0 @@
-This
-filename
-contains
-+
-plus
-chars.

Re: [PATCH] gitweb: Remove gitweb/test/ directory

[Wincent Colaiuta]

El 31/5/2008, a las 20:28, Jakub Narebski escribió:

> Testing if gitweb handles filenames with spaces, filenames with plus
> sign ('+') which encodes spaces in CGI parameters (in URLs), and
> filenames with Unicode characters should be handled by gitweb tests.
>
> Those files are remainder of the time when gitweb was project on its
> own, not a part of git (with its testsuite).
>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>

Ack.
Wincent

Re: [PATCH] gitweb: Remove gitweb/test/ directory

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 644 bytes --]

Hi,

On Sat, 31 May 2008, Wincent Colaiuta wrote:

> El 31/5/2008, a las 20:28, Jakub Narebski escribió:
> 
> > Testing if gitweb handles filenames with spaces, filenames with plus 
> > sign ('+') which encodes spaces in CGI parameters (in URLs), and 
> > filenames with Unicode characters should be handled by gitweb tests.
> >
> > Those files are remainder of the time when gitweb was project on its 
> > own, not a part of git (with its testsuite).
> >
> > Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> 
> Ack.

And I thought "Ack" was reserved for the people who are considered the 
primary authors of the patched code...

Ciao,
Dscho

Re: [PATCH] gitweb: Remove gitweb/test/ directory

[Jakub Narebski]

On Sun, 1 Jan 2008, Johannes Schindelin wrote:
> On Sat, 31 May 2008, Wincent Colaiuta wrote:
>> El 31/5/2008, a las 20:28, Jakub Narebski escribió:
>> 
>>> Testing if gitweb handles filenames with spaces, filenames with plus 
>>> sign ('+') which encodes spaces in CGI parameters (in URLs), and 
>>> filenames with Unicode characters should be handled by gitweb tests.
>>>
>>> Those files are remainder of the time when gitweb was project on its 
>>> own, not a part of git (with its testsuite).
>>>
>>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
>> 
>> Ack.
> 
> And I thought "Ack" was reserved for the people who are considered the 
> primary authors of the patched code...

Unfortunately, as far as I know, primary and only author of those
lines of code, maintainer of gitweb when it was separate project,
Kay Sievers, is no longer active in git development.

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Remove gitweb/test/ directory

[Kay Sievers]

On Sun, 2008-06-01 at 02:19 +0200, Jakub Narebski wrote:
> On Sun, 1 Jan 2008, Johannes Schindelin wrote:
> > On Sat, 31 May 2008, Wincent Colaiuta wrote:
> >> El 31/5/2008, a las 20:28, Jakub Narebski escribió:
> >> 
> >>> Testing if gitweb handles filenames with spaces, filenames with plus 
> >>> sign ('+') which encodes spaces in CGI parameters (in URLs), and 
> >>> filenames with Unicode characters should be handled by gitweb tests.
> >>>
> >>> Those files are remainder of the time when gitweb was project on its 
> >>> own, not a part of git (with its testsuite).
> >>>
> >>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> >> 
> >> Ack.
> > 
> > And I thought "Ack" was reserved for the people who are considered the 
> > primary authors of the patched code...
> 
> Unfortunately, as far as I know, primary and only author of those
> lines of code, maintainer of gitweb when it was separate project,
> Kay Sievers, is no longer active in git development.

Sure, feel free to do whatever makes sense, there is no reason to get my
ACK, as I'm not actively working on it anymore. I'm glad, you guys take
care of gitweb these days, and it has improved a lot since then.

Thanks,
Kay

Re: [PATCH] gitweb: Remove gitweb/test/ directory

[Wincent Colaiuta]

El 1/6/2008, a las 1:19, Johannes Schindelin escribió:

> Hi,
>
> On Sat, 31 May 2008, Wincent Colaiuta wrote:
>
>> El 31/5/2008, a las 20:28, Jakub Narebski escribió:
>>
>>> Testing if gitweb handles filenames with spaces, filenames with plus
>>> sign ('+') which encodes spaces in CGI parameters (in URLs), and
>>> filenames with Unicode characters should be handled by gitweb tests.
>>>
>>> Those files are remainder of the time when gitweb was project on its
>>> own, not a part of git (with its testsuite).
>>>
>>> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
>>
>> Ack.
>
> And I thought "Ack" was reserved for the people who are considered the
> primary authors of the patched code...

I had no idea. Thanks for letting me know.

Wincent

Re: [PATCH] gitweb: Remove gitweb/test/ directory

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Testing if gitweb handles filenames with spaces, filenames with plus
> sign ('+') which encodes spaces in CGI parameters (in URLs), and
> filenames with Unicode characters should be handled by gitweb tests.
>
> Those files are remainder of the time when gitweb was project on its
> own, not a part of git (with its testsuite).
>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> ...
>  "gitweb/test/M\303\244rchen" |    2 --
>  gitweb/test/file with spaces |    4 ----
>  gitweb/test/file+plus+sign   |    6 ------
>  3 files changed, 0 insertions(+), 12 deletions(-)
>  delete mode 100644 gitweb/test/Märchen
>  delete mode 100644 gitweb/test/file with spaces
>  delete mode 100644 gitweb/test/file+plus+sign

I do not think Kay minds, but it would not hurt to give him a courtesy
copy of this before finally applying such a patch.

-jc

Re: [PATCH] gitweb: Remove gitweb/test/ directory

[Jakub Narebski]

On Sat, 1 Jun 2008, Junio C Hamano wrote:

> I do not think Kay minds, but it would not hurt to give him a courtesy
> copy of this before finally applying such a patch.

Done.
-- 
Jakub Narebski
Poland

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Avery Pennarun]

On 5/28/08, Junio C Hamano <gitster@pobox.com> wrote:
> Mark Levedahl <mlevedahl@gmail.com> writes:
>
>  > As this commit is part of the published master branch, I am not sure
>  > the correct resolution: leaving this commit in place means that any
>  > commit between it and a commit fixing this will always cause an error
>  > on Cygwin / Windows. Of course, it *is* on the published master branch.
>
> Some broken filesystems may not be capable of checking out and using
>  project files.  Too bad.
>
>  It's not a big deal.  It is not limited to this project.  We just fix them
>  or work them around and move on.
>
>  Perhaps we should remove the infamous gitweb/test/Märchen file while we
>  are at it?  I do not think the file is ever used.

As an unhappy Windows user myself (sometimes), I think it might be
better to simply fix git to *survive* failing to create files like
'nul' on Win32, rather than trying to *fix* such files in the repo.
It sounds (from the original post) like git has a *fatal* error
("cannot be checked out on cygwin") when trying to create 'nul', which
might be overkill.

The argument about rewriting the git history for this one file is
mostly predicated on the fact that you can't git-bisect if this file
exists.  Rather than making the file not exist, it might be better to
make git work when it does.

Have fun,

Avery

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Junio C Hamano]

"Avery Pennarun" <apenwarr@gmail.com> writes:

> As an unhappy Windows user myself (sometimes), I think it might be
> better to simply fix git to *survive* failing to create files like
> 'nul' on Win32, rather than trying to *fix* such files in the repo.
> It sounds (from the original post) like git has a *fatal* error
> ("cannot be checked out on cygwin") when trying to create 'nul'.

Please learn to think before typing, let alone sending such a message to
waste other people's time.

We give the user an error message, and signal error by exiting with
non-zero.  You cannot have that path on the system, and we are being
honest about it.  It is not like we are suddenly painting the screen in
blue and refusing to get any more user input when you try to check out
such a tree.  Which part of that is _not_ surviving?

The system with a *fatal* error is not git but the one that does not want
an not-so-unreasonable name "NUL" on it.  Git survives on such a system
and tells you what happened --- you cannot do certain things, such as
checking out such a tree.  You live with it, or get a better system ;-)

What alternatives do you want to implement?  Certainly not silently
creating "nul-garbage" file instead and pretend that nothing bad happened,
as that would lead to madness.

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Sverre Rabbelier]

On Wed, May 28, 2008 at 7:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Please learn to think before typing, let alone sending such a message to
> waste other people's time.

Why so harsh?

> What alternatives do you want to implement?  Certainly not silently
> creating "nul-garbage" file instead and pretend that nothing bad happened,
> as that would lead to madness.

Or instead we could have a '-f' switch or such with checkout that
allows you to checkout a revision that contains a bad file, but with
that file missing. Of course this needn't happen silently, a big
warning saying "could not checkout file %s because of %s" may still be
issued.

-- 
Cheers,

Sverre Rabbelier

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Avery Pennarun]

On 5/28/08, Junio C Hamano <gitster@pobox.com> wrote:
>  Please learn to think before typing, let alone sending such a message to
>  waste other people's time.

Phew, just mentioning Windows on this list seems to get people flamed.

Reading over my message, I'm not sure what I would have written to
have it interpreted as being negative.  I was simply suggesting that
git could fail more gracefully here.

>  We give the user an error message, and signal error by exiting with
>  non-zero.  You cannot have that path on the system, and we are being
>  honest about it.  It is not like we are suddenly painting the screen in
>  blue and refusing to get any more user input when you try to check out
>  such a tree.  Which part of that is _not_ surviving?

The part that's not surviving is git-bisect, which is a valid problem
that resulted in someone asking to rewrite the history.  Clearly
rewriting the history is not a good solution, thus no good solution
has yet been proposed, which is why I wrote my message.

>  The system with a *fatal* error is not git but the one that does not want
>  an not-so-unreasonable name "NUL" on it.

That is clearly true.  But knowing that doesn't seem to be making this
user's problem go away.

>  What alternatives do you want to implement?  Certainly not silently
>  creating "nul-garbage" file instead and pretend that nothing bad happened,
>  as that would lead to madness.

If the file failed to be created (with a warning), but we treated it
as having been deleted in 'git status' instead of throwing an error,
it would work much like the case sensitivity problem: the index is
annoyingly dirty, but you can still get work done.  I think that
(perhaps with a little patch) would allow git-bisect to work.

Have fun,

Avery

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Junio C Hamano]

"Avery Pennarun" <apenwarr@gmail.com> writes:

> On 5/28/08, Junio C Hamano <gitster@pobox.com> wrote:
>>  Please learn to think before typing, let alone sending such a message to
>>  waste other people's time.
>
> Phew, just mentioning Windows on this list seems to get people flamed.

I did not flame Windows.

> Reading over my message, I'm not sure what I would have written to
> have it interpreted as being negative....
> The part that's not surviving is git-bisect, which is a valid problem
> that resulted in someone asking to rewrite the history.  Clearly
> rewriting the history is not a good solution, thus no good solution
> has yet been proposed, which is why I wrote my message.

Here is what you wrote:

    As an unhappy Windows user myself (sometimes), I think it might be
    better to simply fix git to *survive* failing to create files like
    'nul' on Win32, rather than trying to *fix* such files in the repo.
    It sounds (from the original post) like git has a *fatal* error
    ("cannot be checked out on cygwin") when trying to create 'nul', which
    might be overkill.

"Survive failing to create files"?  "Fatal error when trying to create
'nul', which might be overkill"?  

If you cannot faithfully recreate the work tree state, you cannot test
that revision.  You cannot bisect and test that revision on that system.
Too bad, but you still have "git reset --hard".  You reset to some other
revision that you _can_ checkout and do sensible tests and move on.

How would you know to reset to recover from your system's deficiency
unless git gives "fatal error"?

That's why I asked you to think before type, which again you did not do,
and that is what got you get "flamed", even though I do not think it was
particularly harsh.

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Daniel Barkalow]

On Wed, 28 May 2008, Junio C Hamano wrote:

> "Avery Pennarun" <apenwarr@gmail.com> writes:
> 
> > As an unhappy Windows user myself (sometimes), I think it might be
> > better to simply fix git to *survive* failing to create files like
> > 'nul' on Win32, rather than trying to *fix* such files in the repo.
> > It sounds (from the original post) like git has a *fatal* error
> > ("cannot be checked out on cygwin") when trying to create 'nul'.
> 
> Please learn to think before typing, let alone sending such a message to
> waste other people's time.
> 
> We give the user an error message, and signal error by exiting with
> non-zero.  You cannot have that path on the system, and we are being
> honest about it.  It is not like we are suddenly painting the screen in
> blue and refusing to get any more user input when you try to check out
> such a tree.  Which part of that is _not_ surviving?
> 
> The system with a *fatal* error is not git but the one that does not want
> an not-so-unreasonable name "NUL" on it.  Git survives on such a system
> and tells you what happened --- you cannot do certain things, such as
> checking out such a tree.  You live with it, or get a better system ;-)
> 
> What alternatives do you want to implement?  Certainly not silently
> creating "nul-garbage" file instead and pretend that nothing bad happened,
> as that would lead to madness.

Report a non-fatal error, mark in the index that that entry is not 
reflected in the working directory, and allow the user to manipulate it 
with commands that don't really need the working directory content.

$ git checkout origin/master
Warning: couldn't create 't/t5100/nul' in your working directory; ignoring 
working directory for this filename.
$ git mv t/t5100/nul t/t5100/nul-plain
$ ls t/t5100/nul-plain
t/t5100/nul-plain

The working directory doesn't really have to be absolutely vital to git's 
functioning (of course, the project you've checked out is going to have 
problems unless you fix things). In particular, it should be possible, on 
a machine with a broken filesystem, to modify a project that triggers the 
filesystem breakage to not trigger it, rather than having to rely on 
either read-tree/update-index --cachinfo/write-tree or a helpful user of a 
non-broken system to get things working again.

Git should, I think, even be able to figure out by itself when to not 
trust the filesystem; if open gives EEXIST, but readdir doesn't list it, 
it's a filesystem problem and we should work around it.

	-Daniel
*This .sig left intentionally blank*

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> Report a non-fatal error, mark in the index that that entry is not 
> reflected in the working directory, and allow the user to manipulate it 
> with commands that don't really need the working directory content.
>
> $ git checkout origin/master
> Warning: couldn't create 't/t5100/nul' in your working directory; ignoring 
> working directory for this filename.
> $ git mv t/t5100/nul t/t5100/nul-plain
> $ ls t/t5100/nul-plain
> t/t5100/nul-plain
>
> The working directory doesn't really have to be absolutely vital to git's 
> functioning (of course, the project you've checked out is going to have 
> problems unless you fix things). In particular, it should be possible, on 
> a machine with a broken filesystem, to modify a project that triggers the 
> filesystem breakage to not trigger it,...

Now that is somebody who thinks before types.

Marking that the filesystem does not match what's in index is already
done, so you could argue that an alternative would be not to stop in the
middle of checkout_entry() loop and instead check out as much as we could,
write out the index perhaps, and signal error, _AFTER_ updating everything
else, including the HEAD.  We try to be atomic when able (e.g. on a broken
patch, "apply" does not apply early half the patch and fail but rejects
the whole thing), but checkout_entry() loop is not something you can
sanely make atomic (it needs to first remove existing files and even
directories before writing new files), so that alternative approach might
be easier to work with.

Care to follow it up with a patch?

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Daniel Barkalow]

On Wed, 28 May 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > Report a non-fatal error, mark in the index that that entry is not 
> > reflected in the working directory, and allow the user to manipulate it 
> > with commands that don't really need the working directory content.
> >
> > $ git checkout origin/master
> > Warning: couldn't create 't/t5100/nul' in your working directory; ignoring 
> > working directory for this filename.
> > $ git mv t/t5100/nul t/t5100/nul-plain
> > $ ls t/t5100/nul-plain
> > t/t5100/nul-plain
> >
> > The working directory doesn't really have to be absolutely vital to git's 
> > functioning (of course, the project you've checked out is going to have 
> > problems unless you fix things). In particular, it should be possible, on 
> > a machine with a broken filesystem, to modify a project that triggers the 
> > filesystem breakage to not trigger it,...
> 
> Now that is somebody who thinks before types.

Well, and I've got the background to know what's possible and how git can 
keep things straight.

> Marking that the filesystem does not match what's in index is already
> done, so you could argue that an alternative would be not to stop in the
> middle of checkout_entry() loop and instead check out as much as we could,
> write out the index perhaps, and signal error, _AFTER_ updating everything
> else, including the HEAD.  We try to be atomic when able (e.g. on a broken
> patch, "apply" does not apply early half the patch and fail but rejects
> the whole thing), but checkout_entry() loop is not something you can
> sanely make atomic (it needs to first remove existing files and even
> directories before writing new files), so that alternative approach might
> be easier to work with.

Ah, yes, CE_VALID. But it doesn't quite work as well as I'd like, because 
it doesn't ignore fstat/readdir not finding anything on the filesystem, so 
it comes out looking deleted, at least if you're actually on Linux (with a 
hack in create_file to refuse to create certain filenames for testing).

Also "git mv CE_VALID-source dest" doesn't ignore the filesystem like it 
should (for this use, anyway). Perhaps we need an additional flag for "the 
filesystem is irrelevant for this entry".

> Care to follow it up with a patch?

Unfortunately, I don't think I'll have a chance for a while to actually 
work on git code. But if someone else (Avery?) wants to try it, I think 
giving a big warning, setting CE_VALID, and returning 0 in entry.c before 
the "unable to create file" message is the right thing to start with. And 
someone with Windows access should figure out what happens next.

I think the right test for this is if create_file() returns EEXIST, but 
readdir doesn't show anything. For that matter, it might be useful to have 
logic that notes the situation where you seem to have file A instead of 
file B, but fstat("B") returns A's inode, and marks the index to say that 
entry B is listed in the filesystem as A instead.

	-Daniel
*This .sig left intentionally blank*

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> Ah, yes, CE_VALID. But it doesn't quite work as well as I'd like, because 

No, I do not think we should involve CE_VALID here.  It means something
completely different.  What I meant was that through "git status" the user
can tell there is an unexpected breakage in the work tree, _if_ we make
checkout to finish with "best effort" (and still report an error).

> I think the right test for this is if create_file() returns EEXIST, but 
> readdir doesn't show anything.

I think relying on EEXIST is too specific for this particular breakage,
even though such a test may catch it.  A checkout may fail in the middle
if a filesystem refuses to create a pathname that has certain characters
in it (e.g. dosen't NTFS refuse a path with :|<"?*> in it, or is it just
the Explorer UI layer rejecting them?), or perhaps one leading directory
may be unwritable.  We would want to catch and cope with such a brokage
the same way.

The checkout "unpack-trees" codepath does:

 - Make sure things can be checked out safely with the internal data
   before doing anything to the filesystem, i.e. no lost local changes, no
   lost untracked files, etc.

 - For each path:

   - make room for it, removing directory at the place as necessary where
     a blob must sit and removing an existing blob as needed;

   - create a new file or symlink;

And currently I think we stop on any failure.  The thing is, stopping on a
failure during the internal checking is fine --- no external damage has
been made yet.  But once we started updating the work tree, we _are_
committed and not aborting in the middle for a single failure would be the
saner thing to do.  In addition, even after such a failure after we are
committed, we probably should update the HEAD and the index.

"status" would then show the difference between what should have been
checked out and what is.  It might be enough to improve the issue of "git
bisect hitting a checkout failure --- the work tree is half checked-out
state, and the index, the HEAD, and the work tree are in a very
inconsistent state".

We would probably signal such an error from git-checkout differently from
an early refusal that does not do anything, to tell the callers, such as
"git-bisect", that the checkout _has been_ already done, even though there
may be breakages in the work tree.

> ... that notes the situation where you seem to have file A instead of 
> file B, but fstat("B") returns A's inode, and marks the index to say that 
> entry B is listed in the filesystem as A instead.

I personally do not think such auto-substution is a way to go --- what
makes you trust inode information from such an untrustworthy filesystem
that does not do what it was told to do?  I suspect that stopping at the
error site and not automatically making the damage yet larger by doing
such magic would keep the recovery procedure simpler.

But I wouldn't keep people from experimenting.  Perhaps the end result
could be even readable and mergeable, although I am quite pessimistic.

[PATCH] "git checkout -- paths..." should signal error

[Junio C Hamano]

When "git checkout -- paths..." cannot update work tree for whatever
reason, checkout_entry() correctly issued an error message for the path to
the end user, but the command ignored the error, causing the entire
command to succeed.  This fixes it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Now that we detect this failure, post-checkout-hook _might_ want to
   know that what we have is an incomplete checkout.  Not calling the hook
   in such a case may be another option, but we always called the hook and
   that would be a change in behaviour.

 builtin-checkout.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 1ea017f..00dc8ca 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -84,6 +84,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec)
 	unsigned char rev[20];
 	int flag;
 	struct commit *head;
+	int errs = 0;
 
 	int newfd;
 	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
@@ -106,13 +107,14 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec)
 	if (report_path_error(ps_matched, pathspec, 0))
 		return 1;
 
+	/* Now we are committed to check them out */
 	memset(&state, 0, sizeof(state));
 	state.force = 1;
 	state.refresh_cache = 1;
 	for (pos = 0; pos < active_nr; pos++) {
 		struct cache_entry *ce = active_cache[pos];
 		if (pathspec_match(pathspec, NULL, ce->name, 0)) {
-			checkout_entry(ce, &state, NULL);
+			errs |= checkout_entry(ce, &state, NULL);
 		}
 	}
 
@@ -123,7 +125,8 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec)
 	resolve_ref("HEAD", rev, 0, &flag);
 	head = lookup_commit_reference_gently(rev, 1);
 
-	return post_checkout_hook(head, head, 0);
+	errs |= post_checkout_hook(head, head, 0);
+	return errs;
 }
 
 static void show_local_changes(struct object *head)

Re: [PATCH] "git checkout -- paths..." should signal error

[Marius Storm-Olsen]

[-- Attachment #1: Type: text/plain, Size: 2043 bytes --]

Junio C Hamano said the following on 28.05.2008 23:19:
> When "git checkout -- paths..." cannot update work tree for whatever
> reason, checkout_entry() correctly issued an error message for the path to
> the end user, but the command ignored the error, causing the entire
> command to succeed.  This fixes it.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> 
>  * Now that we detect this failure, post-checkout-hook _might_ want to
>    know that what we have is an incomplete checkout.  Not calling the hook
>    in such a case may be another option, but we always called the hook and
>    that would be a change in behaviour.
> 
>  builtin-checkout.c |    7 +++++--
>  1 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin-checkout.c b/builtin-checkout.c

I'd like to just chip in that you actually _can_ create those files 
(nul, con, prn, all which are "special" filenames on Windows), but 
need a slight different construct to create them. That, and most tools 
will break on _using_ them, since they's also have to refer to them in 
a certain way.

So, certainly msysgit could write a workaround to handle such cases. 
(Git would also know how to remove these files on Windows)

Though, I still think that your patch is a Good Thing(tm).


For example, if you:

D:\some\path> echo foo > nul    # Ok, an no output, nor file created
D:\some\path> echo foo > .\nul  # Same thing
D:\some\path> echo foo > \\.\d:\some\path\nul  # Creates the nul file

  Directory of D:\some\path

29.05.2008  08:23    <DIR>          .
29.05.2008  08:23    <DIR>          ..
29.05.2008  08:23                 6 con
29.05.2008  08:22                 6 nul
29.05.2008  08:23                 6 prn
                3 File(s)             18 bytes

D:\some\path> del .\nul
The filename, directory name, or volume label syntax is incorrect.

D:\some\path> del \\.\d:\some\path\nul  # Success

-- 
.marius [@trolltech.com]
'if you know what you're doing, it's not research'


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 187 bytes --]

Re: [PATCH] "git checkout -- paths..." should signal error

[Daniel Barkalow]

On Wed, 28 May 2008, Junio C Hamano wrote:

> When "git checkout -- paths..." cannot update work tree for whatever
> reason, checkout_entry() correctly issued an error message for the path to
> the end user, but the command ignored the error, causing the entire
> command to succeed.  This fixes it.

Acked-by: Daniel Barkalow <barkalow@iabervon.org>

Re: Commit cce8d6fdb introduces file t/t5100/nul, git tree is now incompatible with Cygwin (and probably Windows)

[Daniel Barkalow]

On Wed, 28 May 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > Ah, yes, CE_VALID. But it doesn't quite work as well as I'd like, because 
> 
> No, I do not think we should involve CE_VALID here.  It means something
> completely different.  What I meant was that through "git status" the user
> can tell there is an unexpected breakage in the work tree, _if_ we make
> checkout to finish with "best effort" (and still report an error).

I think there are two separate issues here: (1) you've gotten partway 
through a checkout, and something fails, and we want to leave things in as 
good shape as possible; (2) you're trying to check out a tree that can't 
be accurately represented on your filesystem. In case (2), there's a good 
chance that you want to leave the unrepresentable stuff alone or modify it 
in the index without going through the filesystem.

Case (2) is very much like working on a filesystem without +x bits, where 
you'd really like to ignore what the filesystem reports and only diverge 
from what was read into the index from a tree object when the user makes 
explicit modifications to the index, rather than updating the index from 
the working directory state.

> > I think the right test for this is if create_file() returns EEXIST, but 
> > readdir doesn't show anything.
> 
> I think relying on EEXIST is too specific for this particular breakage,
> even though such a test may catch it. 

It's not just EEXIST; it's EEXIST for a filename with no struct dirent.

> A checkout may fail in the middle if a filesystem refuses to create a 
> pathname that has certain characters in it (e.g. dosen't NTFS refuse a 
> path with :|<"?*> in it, or is it just the Explorer UI layer rejecting 
> them?), or perhaps one leading directory may be unwritable.  We would 
> want to catch and cope with such a brokage the same way.

I think the leading directory thing is qualitatively different; you're 
probably not going to want to create a patch to rename all of the files in 
that directory to be in a directory that's not owned by a different user 
on your machine, while you are likely to try to get a project to use less 
odd names if there's a filename with a : in it or something. I'm not sure 
what error open() will give you for that.

The other interesting case is when the filesystem is case insensative and 
the project contains files that differ only in case; again, the filesystem 
will report EEXIST on open for a path that readdir doesn't list.

> The checkout "unpack-trees" codepath does:
> 
>  - Make sure things can be checked out safely with the internal data
>    before doing anything to the filesystem, i.e. no lost local changes, no
>    lost untracked files, etc.
> 
>  - For each path:
> 
>    - make room for it, removing directory at the place as necessary where
>      a blob must sit and removing an existing blob as needed;
> 
>    - create a new file or symlink;
> 
> And currently I think we stop on any failure.  The thing is, stopping on a
> failure during the internal checking is fine --- no external damage has
> been made yet.  But once we started updating the work tree, we _are_
> committed and not aborting in the middle for a single failure would be the
> saner thing to do.  In addition, even after such a failure after we are
> committed, we probably should update the HEAD and the index.
> 
> "status" would then show the difference between what should have been
> checked out and what is.  It might be enough to improve the issue of "git
> bisect hitting a checkout failure --- the work tree is half checked-out
> state, and the index, the HEAD, and the work tree are in a very
> inconsistent state".

For case (1), things are in an inconsistant state; for case (2), the index 
and HEAD agree, and there are known gaps in the work tree.

> We would probably signal such an error from git-checkout differently from
> an early refusal that does not do anything, to tell the callers, such as
> "git-bisect", that the checkout _has been_ already done, even though there
> may be breakages in the work tree.
> 
> > ... that notes the situation where you seem to have file A instead of 
> > file B, but fstat("B") returns A's inode, and marks the index to say that 
> > entry B is listed in the filesystem as A instead.
> 
> I personally do not think such auto-substution is a way to go --- what
> makes you trust inode information from such an untrustworthy filesystem
> that does not do what it was told to do?  I suspect that stopping at the
> error site and not automatically making the damage yet larger by doing
> such magic would keep the recovery procedure simpler.

People seem to use Windows and OS X despite the filesystems being broken. 
There seems to be a space of filesystems which aren't corrupted, but do 
unexpected things with respect to filenames. If we can find appropriate 
filenames, the content is stored reliably (or the open O_EXCL is refused). 
We should be able to identify that the user isn't trying to make a change 
by way of the working directory when the difference we see is something 
that isn't clearly representable by the filesystem.

> But I wouldn't keep people from experimenting.  Perhaps the end result
> could be even readable and mergeable, although I am quite pessimistic.

I think, in any case, that it should be pretty clean to have a "filesystem 
is inadaquate" CE flag, which mean that we just ignore the filesystem. 
Finding things that are reported with the wrong name is probably harder.

	-Daniel
*This .sig left intentionally blank*