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

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

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

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

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

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

Hi,

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

Ciao,
Dscho

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

Hi,

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

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

[...]

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

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

---

[...]

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

deleted file mode 100644
index 8f7a1d3..0000000
@@ -1,2 +0,0 @@
-Märchen
-M�rchen

deleted file mode 100644
index f108543..0000000
@@ -1,4 +0,0 @@
-This
-filename
-contains
-spaces.

deleted file mode 100644
index fd05278..0000000
@@ -1,6 +0,0 @@
-This
-filename
-contains
-+
-plus
-chars.

Ack.
Wincent

Hi,

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

Ciao,
Dscho

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

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

I had no idea. Thanks for letting me know.

Wincent

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

Done.

Jakub Narebski
Poland

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

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.

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.

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.

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

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

I did not flame Windows.

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.

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*

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?

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

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

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*

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

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.

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.

---

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

index 1ea017f..00dc8ca 100644
@@ -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)

Junio C Hamano said the following on 28.05.2008 23:19:

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

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

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.

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

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.

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.

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.

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*

Why so harsh?

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

Hi,

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

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

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

---

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

index dbfa2b6..aced19b 100644
@@ -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

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

Hi,

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.

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

index ef5ce5e..ab358b2 100644
@@ -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

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

Hi,

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

Sorry. Was worth a try.

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:

error: git-checkout-index: unable to create file t/t5100/nul (File exists)

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

Hi,

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

Hi,

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.

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

index a8b78eb..577ecc2 100755
@@ -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)
 

similarity index 100%
rename from t/t5100/nul
rename to t/t5100/nul-plain

1.5.6.rc0.175.gdd78

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