Re: Error writing loose object on Cygwin

[Shawn Pearce <spearce@spearce.org>]

Linus Torvalds <torvalds@osdl.org> wrote:
> On Tue, 11 Jul 2006, Junio C Hamano wrote:
> 
> > Shawn Pearce <spearce@spearce.org> writes:
> > 
> > > Has anyone else seen this type of behavior before?  Any suggestions
> > > on debugging this issue?
> > 
> > I would suggest raising this (politely) to Cygwin people.
> 
> Well, since it apparently works with W2000, and breaks with XP, I suspect 
> it's actually Windows that just returns the wrong error code.
> 
> It's entirely possible that we should just make that whole
> 
> 	if (ret == ENOENT)
> 
> go away. Yes, it's the right error code if a subdirectory is missing, and 
> yes, POSIX requires it, and yes, WXP is probably just a horrible piece of 
> sh*t, but on the other hand, I don't think git really has any serious 
> reason to even care. 
> 
> So we might as well say that if the link() fails for _any_ reason, we'll 
> try to see if doing the mkdir() and re-trying the link helps.


Hmm.  Its a single mkdir call before we give up and tell the user
something is wrong.  The following change appears to work OK here on
a reasonably POSIX compliant system (OK meaning it reports errors reasonably).

Given that this type of error (failed link) shouldn't happen
that often, except for on Coda or FAT (according to a comment in
move_temp_to_file), I guess the change is OK and comes with little
penalty.  But for Coda and FAT users things are going to slow down a
little bit as we try mkdir for every new loose object being created
before we try rename.

Tomorrow when I get access to my Cygwin system again I'll try to
write up a tiny test case which shows the error behavior we are
seeing and send it to the Cygwin mailing list, as this really does
seem to be a Cygwin or Windows issue.  But of course having GIT
handle this case slightly better wouldn't be bad either.  :-)


diff --git a/sha1_file.c b/sha1_file.c
index 8734d50..db4bddc 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1336,26 +1336,23 @@ static int link_temp_to_file(const char 
 		return 0;
 
 	/*
-	 * Try to mkdir the last path component if that failed
-	 * with an ENOENT.
+	 * Try to mkdir the last path component if that failed.
 	 *
 	 * Re-try the "link()" regardless of whether the mkdir
 	 * succeeds, since a race might mean that somebody
 	 * else succeeded.
 	 */
 	ret = errno;
-	if (ret == ENOENT) {
-		char *dir = strrchr(filename, '/');
-		if (dir) {
-			*dir = 0;
-			mkdir(filename, 0777);
-			if (adjust_shared_perm(filename))
-				return -2;
-			*dir = '/';
-			if (!link(tmpfile, filename))
-				return 0;
-			ret = errno;
-		}
+	char *dir = strrchr(filename, '/');
+	if (dir) {
+		*dir = 0;
+		mkdir(filename, 0777);
+		if (adjust_shared_perm(filename))
+			return -2;
+		*dir = '/';
+		if (!link(tmpfile, filename))
+			return 0;
+		ret = errno;
 	}
 	return ret;
 }