git on afs

git on afs

[Todd T. Fries]

I have always been frustrated by trying git on afs.

The other day I finally decided to look into why.

Looks like two reasons caused my frustrations.

1) git presumes that link() works fine across subdirs; in afs land,
   hardlinks do not succeed ever

2) git presumes that DTYPE(de) != DT_DIR .. means the dirent is not a dir
   this is not true for afs

I have been using this to sync several git trees, including linux-2.6 for the
past week without issues writing to a local afs server.

What do you guys think?

diff --git a/dir.c b/dir.c
index eb6c3ab..a3e53a5 100644
--- a/dir.c
+++ b/dir.c
@@ -487,9 +487,19 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
 			    && in_pathspec(fullname, baselen + len, simplify))
 				dir_add_ignored(dir, fullname, baselen + len);
 			if (exclude != dir->show_ignored) {
-				if (!dir->show_ignored || DTYPE(de) != DT_DIR) {
+				if (!dir->show_ignored)
 					continue;
 				}
+				if (DTYPE(de) == DT_UNKNOWN) {
+					struct stat st;
+					if (lstat(fullname, &st))
+						continue;
+					if (!S_ISDIR(st.st_mode))
+						continue;
+				} else {
+					if (DTYPE(de) != DT_DIR)
+						continue;
+				}
 			}
 
 			switch (DTYPE(de)) {
diff --git a/sha1_file.c b/sha1_file.c
index 83a06a7..1b93322 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1961,7 +1961,7 @@ static int link_temp_to_file(const char *tmpfile, const char *filename)
 	int ret;
 	char *dir;
 
-	if (!link(tmpfile, filename))
+	if (!rename(tmpfile, filename))
 		return 0;
 
 	/*
@@ -1980,7 +1980,7 @@ static int link_temp_to_file(const char *tmpfile, const char *filename)
 			return -2;
 		}
 		*dir = '/';
-		if (!link(tmpfile, filename))
+		if (!rename(tmpfile, filename))
 			return 0;
 		ret = errno;
 	}
-- 
Todd Fries .. todd@fries.net

 _____________________________________________
|                                             \  1.636.410.0632 (voice)
| Free Daemon Consulting, LLC                 \  1.405.227.9094 (voice)
| http://FreeDaemonConsulting.com             \  1.866.792.3418 (FAX)
| "..in support of free software solutions."  \          250797 (FWD)
|                                             \
 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\
                                                 
              37E7 D3EB 74D0 8D66 A68D  B866 0326 204E 3F42 004A
                        http://todd.fries.net/pgp.txt

Re: git on afs

[Brandon Casey]

On Thu, 18 Oct 2007, Todd T. Fries wrote:

> 1) git presumes that link() works fine across subdirs; in afs land,
>   hardlinks do not succeed ever

I think this already falls back to rename() on failure.
Take a look at move_temp_to_file(), this is the only
place that calls link_temp_to_file().

link() on afs returns non-zero? and not EEXIST right?

-brandon

Re: git on afs

[Brandon Casey]

On Thu, 18 Oct 2007, Todd T. Fries wrote:

> 2) git presumes that DTYPE(de) != DT_DIR .. means the dirent is not a dir
>   this is not true for afs
>
> I have been using this to sync several git trees, including linux-2.6 for the
> past week without issues writing to a local afs server.
>
> What do you guys think?
>
> diff --git a/dir.c b/dir.c
> index eb6c3ab..a3e53a5 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -487,9 +487,19 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
> 			    && in_pathspec(fullname, baselen + len, simplify))
> 				dir_add_ignored(dir, fullname, baselen + len);
> 			if (exclude != dir->show_ignored) {
> -				if (!dir->show_ignored || DTYPE(de) != DT_DIR) {
> +				if (!dir->show_ignored)

you're missing an open bracket                          ^^^

> 					continue;
> 				}

or delete this one             ^^^

> +				if (DTYPE(de) == DT_UNKNOWN) {
> +					struct stat st;
> +					if (lstat(fullname, &st))
> +						continue;
> +					if (!S_ISDIR(st.st_mode))
> +						continue;
> +				} else {
> +					if (DTYPE(de) != DT_DIR)
> +						continue;
> +				}
> 			}
>
> 			switch (DTYPE(de)) {

seems sane to me.

If no one else brings up any issues, you should retest and submit
a signed patch with a good comment. Use -s with git-commit.
Then git-format-patch will produce the proper output.

-brandon

Re: git on afs

[Linus Torvalds]

On Thu, 18 Oct 2007, Todd T. Fries wrote:
> 
> 2) git presumes that DTYPE(de) != DT_DIR .. means the dirent is not a dir
>    this is not true for afs

That's a major bug, and has nothing to do with AFS. Oops. 

If you look just a bit lower, you'll see that just a few lines down, git 
handles DT_UNKNOWN correctly, and just does a lstat() on it as required. I 
guess that logic should be moved up, or alternatively the exclude logic 
should be moved down.

Your patch looks ok, but at the same time, I don't think it's really the 
right thing to do, since it now does that lstat() twice.

		Linus

Re: git on afs

[Shawn O. Pearce]

There's two different issues here so I'm going to split the thread
into two to simplify the discussion.  Well, for me anyway.  ;-)

"Todd T. Fries" <todd@fries.net> wrote:
> 1) git presumes that link() works fine across subdirs; in afs land,
>    hardlinks do not succeed ever
...
> diff --git a/sha1_file.c b/sha1_file.c
> index 83a06a7..1b93322 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1961,7 +1961,7 @@ static int link_temp_to_file(const char *tmpfile, const char *filename)
>  	int ret;
>  	char *dir;
>  
> -	if (!link(tmpfile, filename))
> +	if (!rename(tmpfile, filename))
>  		return 0;
>  
>  	/*
> @@ -1980,7 +1980,7 @@ static int link_temp_to_file(const char *tmpfile, const char *filename)
>  			return -2;
>  		}
>  		*dir = '/';
> -		if (!link(tmpfile, filename))
> +		if (!rename(tmpfile, filename))
>  			return 0;
>  		ret = errno;
>  	}

These cases should already be handled lower, see l.1997-2012 of
sha1_file.c:

    /*
     * Coda hack - coda doesn't like cross-directory links,
     * so we fall back to a rename, which will mean that it
     * won't be able to check collisions, but that's not a
     * big deal.
     *
     * The same holds for FAT formatted media.
     *
     * When this succeeds, we just return 0. We have nothing
     * left to unlink.
     */
    if (ret && ret != EEXIST) {
        if (!rename(tmpfile, filename))

> Brandon Casey <casey@nrlssc.navy.mil> wrote:
> On Thu, 18 Oct 2007, Todd T. Fries wrote:
> 
> > link() returns -1 errno 17 File exists on afs.
> >
> > To further muddy the waters, linking within the same dir is ok,
> > linking outside the same dir is not:
> >
> > todd@ispdesk/p6 ~/tmp=A661$ mkdir dir
> > todd@ispdesk/p6 ~/tmp=A662$ touch a
> > todd@ispdesk/p6 ~/tmp=A663$ ln a b
> > todd@ispdesk/p6 ~/tmp=A664$ ls -l a b
> > -rw-r--r--  2 4  wheel  0 Oct 18 17:09 a
> > -rw-r--r--  2 4  wheel  0 Oct 18 17:09 b
> > todd@ispdesk/p6 ~/tmp=A665$ ls -li a b
> > 2068032 -rw-r--r--  2 4  wheel  0 Oct 18 17:09 a
> > 2068032 -rw-r--r--  2 4  wheel  0 Oct 18 17:09 b
> > todd@ispdesk/p6 ~/tmp=A666$ ln a dir/b
> > ln: dir/b: File exists
> > todd@ispdesk/p6 ~/tmp=A667$ echo $?
> 
> That error is just bogus on afs. If it returned a sane
> error, things would just work.
> 
> But, looks like afs only supports linking within the same
> directory: http://www.angelfire.com/hi/plutonic/afs-faq.html

So according to that error message from "ln" we really should have
fallen into that Coda hack I just mentioned.  Did we instead get
a different errno here and not use that hack?


We probably could just use rename as you do above but then we would
want to remove the unlink(tmpfile) call on l.2013 in sha1_file.c.
Otherwise we're always incurring a syscall for no reason.  I'm not
against a change here, I just want to make sure we make the right
change for AFS.  :-)

-- 
Shawn.

Re: git on afs

[Shawn O. Pearce]

Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Thu, 18 Oct 2007, Todd T. Fries wrote:
> > 
> > 2) git presumes that DTYPE(de) != DT_DIR .. means the dirent is not a dir
> >    this is not true for afs
> 
> That's a major bug, and has nothing to do with AFS. Oops. 
> 
> If you look just a bit lower, you'll see that just a few lines down, git 
> handles DT_UNKNOWN correctly, and just does a lstat() on it as required. I 
> guess that logic should be moved up, or alternatively the exclude logic 
> should be moved down.
> 
> Your patch looks ok, but at the same time, I don't think it's really the 
> right thing to do, since it now does that lstat() twice.

What about this instead?  It avoids the double lstat() of Todd's
original patch but seems like it would fix the issue here.  Or did
I misunderstand the problem?


--8>--
From f290fc5f6ec5042b7c0393a300e4d3738b9090b8 Mon Sep 17 00:00:00 2001
From: Shawn O. Pearce <spearce@spearce.org>
Date: Fri, 19 Oct 2007 02:03:55 -0400
Subject: [PATCH] Correctly scan directories when DTYPE(de) is unknown

We have been presuming that if DTYPE(de) != DT_DIR than the item we
obtained from readdir() isn't a directory.  This is actually not
true, it could still be a directory, especially if the underlying
system we were compiled for doesn't have the d_type member of struct
dirent and we have enabled our compatability definition of DTYPE(de)
to always be DT_UNKNOWN.

If DTYPE(de) == DT_UNKNOWN we need to fall through and perform an
actual lstat() call to determine what the type of this item is so
we know how to proceed with our scanning.

Reported by Todd T. Fries while trying to use Git on AFS, which
apparently has DTYPE(de) != DT_DIR for directories.

Signed-off-by: Shawn O. Pearce <spearce@spearce.org>
---
 dir.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/dir.c b/dir.c
index eb6c3ab..d2597ff 100644
--- a/dir.c
+++ b/dir.c
@@ -487,9 +487,10 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
 			    && in_pathspec(fullname, baselen + len, simplify))
 				dir_add_ignored(dir, fullname, baselen + len);
 			if (exclude != dir->show_ignored) {
-				if (!dir->show_ignored || DTYPE(de) != DT_DIR) {
+				if (!dir->show_ignored)
+					continue;
+				else if (DTYPE(de) != DT_DIR && DTYPE(de) != DT_UNKNOWN)
 					continue;
-				}
 			}
 
 			switch (DTYPE(de)) {
-- 
1.5.3.4.1249.g895be

-- 
Shawn.

Re: git on afs

[Todd T. Fries]

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

Shawn,

If DT_UNKNOWN exists, then we have to do a stat() of some form to
find out the right type.

This is difficult to fix properly to avoid the extra stat() since
inside the switch logic we do the recursion, but we might have
avoided it earlier because of the exclusion.

I'll send a separate diff for an updated link() vs rename() diff.

I've attached an updated diff that should address concerns of everyone
who gave me feedback on my dir.c changes.

Better?

Thanks,

On Friday 19 October 2007 01:06:24 Shawn O. Pearce wrote:
> Linus Torvalds <torvalds@linux-foundation.org> wrote:
> > On Thu, 18 Oct 2007, Todd T. Fries wrote:
> > > 2) git presumes that DTYPE(de) != DT_DIR .. means the dirent is not a
> > > dir this is not true for afs
> >
> > That's a major bug, and has nothing to do with AFS. Oops.
> >
> > If you look just a bit lower, you'll see that just a few lines down, git
> > handles DT_UNKNOWN correctly, and just does a lstat() on it as required.
> > I guess that logic should be moved up, or alternatively the exclude logic
> > should be moved down.
> >
> > Your patch looks ok, but at the same time, I don't think it's really the
> > right thing to do, since it now does that lstat() twice.
>
> What about this instead?  It avoids the double lstat() of Todd's
> original patch but seems like it would fix the issue here.  Or did
> I misunderstand the problem?
[..]
> diff --git a/dir.c b/dir.c
> index eb6c3ab..d2597ff 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -487,9 +487,10 @@ static int read_directory_recursive(struct dir_struct
> *dir, const char *path, co && in_pathspec(fullname, baselen + len,
> simplify))
>  				dir_add_ignored(dir, fullname, baselen + len);
>  			if (exclude != dir->show_ignored) {
> -				if (!dir->show_ignored || DTYPE(de) != DT_DIR) {
> +				if (!dir->show_ignored)
> +					continue;
> +				else if (DTYPE(de) != DT_DIR && DTYPE(de) != DT_UNKNOWN)
>  					continue;
> -				}
>  			}
>
>  			switch (DTYPE(de)) {
> --
> 1.5.3.4.1249.g895be



-- 
Todd Fries .. todd@fries.net

 _____________________________________________
|                                             \  1.636.410.0632 (voice)
| Free Daemon Consulting                      \  1.405.227.9094 (voice)
| http://FreeDaemonConsulting.com             \  1.866.792.3418 (FAX)
| "..in support of free software solutions."  \          250797 (FWD)
|                                             \
 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\
                                                 
              37E7 D3EB 74D0 8D66 A68D  B866 0326 204E 3F42 004A
                        http://todd.fries.net/pgp.txt






[-- Attachment #2: 0001-If-readdir-returns-a-DTYPE-of-DT_UNKNOWN-this-w.patch --]
[-- Type: text/x-diff, Size: 1848 bytes --]

From f7bca472645db70b52824e1be827ba99195198d2 Mon Sep 17 00:00:00 2001
From: Todd T. Fries <todd@fries.net>
Date: Fri, 19 Oct 2007 06:26:49 -0500
Subject: [PATCH] If readdir() returns a DTYPE() of DT_UNKNOWN, this warrants further
investigation, i.e. a stat() or lstat().  We have been presuming
anything not DT_DIR is not a dir, which is not the case for
all filesystems (e.g. afs).

Do the lstat() once, and use the results twice.

Signed-off-by: Todd T. Fries <todd@fries.net>
---
 dir.c |   16 +++++++++++-----
 1 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/dir.c b/dir.c
index eb6c3ab..0ed4739 100644
--- a/dir.c
+++ b/dir.c
@@ -468,6 +468,7 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
 		while ((de = readdir(fdir)) != NULL) {
 			int len;
 			int exclude;
+			struct stat st;
 
 			if ((de->d_name[0] == '.') &&
 			    (de->d_name[1] == 0 ||
@@ -486,19 +487,24 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
 			if (exclude && dir->collect_ignored
 			    && in_pathspec(fullname, baselen + len, simplify))
 				dir_add_ignored(dir, fullname, baselen + len);
+			if (DTYPE(de) == DT_UNKNOWN) {
+				if (lstat(fullname, &st))
+					continue;
+			}
 			if (exclude != dir->show_ignored) {
-				if (!dir->show_ignored || DTYPE(de) != DT_DIR) {
+				if (!dir->show_ignored))
 					continue;
-				}
+				if (DTYPE(de) == DT_UNKNOWN && !S_ISDIR(st.st_mode))
+					continue;
+				else
+					if (DTYPE(de) != DT_DIR)
+						continue;
 			}
 
 			switch (DTYPE(de)) {
-			struct stat st;
 			default:
 				continue;
 			case DT_UNKNOWN:
-				if (lstat(fullname, &st))
-					continue;
 				if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode))
 					break;
 				if (!S_ISDIR(st.st_mode))
-- 
1.5.2.5

Re: git on afs

[Todd T. Fries]

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

You're the second one to point out that I should check the errno of link() on
afs.

It should not matter, but I'm using arla's afs client on OpenBSD; the errno
is 17 (EEXIST), the very errno that bypasses the coda hack's rename():

  8484 git-index-pack CALL  mkdir(0xcfbdc750,0x1ff)
  8484 git-index-pack NAMI  ".git/objects/pack"
  8484 git-index-pack RET   mkdir -1 errno 17 File exists
  8484 git-index-pack CALL  link(0x829e7040,0xcfbdc750)
  8484 git-index-pack NAMI  ".git/objects/tmp_pack_BU8484"
  8484 git-index-pack 
NAMI  ".git/objects/pack/pack-db66c6d25d6e0044093252434b4aa2a7d67e386a.pack"
  8484 git-index-pack RET   link -1 errno 17 File exists

.. and does not go on to do a rename(), which based on the code makes sense.

I can assure you that the 2nd argument to link does not exist ;-)

So, I've made the following change and it seems to fix the problem, much
cleaner IMHO, and get the following change in behavior:

  5933 git-index-pack CALL  mkdir(0xcfbc0070,0x1ff)
  5933 git-index-pack NAMI  ".git/objects/pack"
  5933 git-index-pack RET   mkdir -1 errno 17 File exists
  5933 git-index-pack CALL  link(0x860c0040,0xcfbc0070)
  5933 git-index-pack NAMI  ".git/objects/tmp_pack_EV5933"
  5933 git-index-pack 
NAMI  ".git/objects/pack/pack-db66c6d25d6e0044093252434b4aa2a7d67e386a.pack"
  5933 git-index-pack RET   link -1 errno 17 File exists
[..]
  5933 git-index-pack CALL  rename(0x860c0040,0xcfbc0070)
  5933 git-index-pack NAMI  ".git/objects/tmp_pack_EV5933"
  5933 git-index-pack 
NAMI  ".git/objects/pack/pack-db66c6d25d6e0044093252434b4aa2a7d67e386a.pack"
  5933 git-index-pack RET   rename 0

The only downside is that either on coda or if the file already exists, it
will try a spurrous rename(), in which case it will fail with EEXIST again,
so I hope this is not a big negative.

If this is ok, an appropriate commit message might be something like:

    AFS inconveniently returns EEXIST from link() to say it
    doesn't like cross directory link()'s.  Do a little
    extra work to fix this by ignoring EEXIST and trying
    a rename() anyway.


Thanks,

On Friday 19 October 2007 00:48:14 Shawn O. Pearce wrote:
> There's two different issues here so I'm going to split the thread
> into two to simplify the discussion.  Well, for me anyway.  ;-)
>
> "Todd T. Fries" <todd@fries.net> wrote:
> > 1) git presumes that link() works fine across subdirs; in afs land,
> >    hardlinks do not succeed ever
>
> ...
>
> > diff --git a/sha1_file.c b/sha1_file.c
> > index 83a06a7..1b93322 100644
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -1961,7 +1961,7 @@ static int link_temp_to_file(const char *tmpfile,
> > const char *filename) int ret;
> >  	char *dir;
> >
> > -	if (!link(tmpfile, filename))
> > +	if (!rename(tmpfile, filename))
> >  		return 0;
> >
> >  	/*
> > @@ -1980,7 +1980,7 @@ static int link_temp_to_file(const char *tmpfile,
> > const char *filename) return -2;
> >  		}
> >  		*dir = '/';
> > -		if (!link(tmpfile, filename))
> > +		if (!rename(tmpfile, filename))
> >  			return 0;
> >  		ret = errno;
> >  	}
>
> These cases should already be handled lower, see l.1997-2012 of
> sha1_file.c:
>
>     /*
>      * Coda hack - coda doesn't like cross-directory links,
>      * so we fall back to a rename, which will mean that it
>      * won't be able to check collisions, but that's not a
>      * big deal.
>      *
>      * The same holds for FAT formatted media.
>      *
>      * When this succeeds, we just return 0. We have nothing
>      * left to unlink.
>      */
>     if (ret && ret != EEXIST) {
>         if (!rename(tmpfile, filename))
>
> > Brandon Casey <casey@nrlssc.navy.mil> wrote:
> >
> > On Thu, 18 Oct 2007, Todd T. Fries wrote:
> > > link() returns -1 errno 17 File exists on afs.
> > >
> > > To further muddy the waters, linking within the same dir is ok,
> > > linking outside the same dir is not:
> > >
> > > todd@ispdesk/p6 ~/tmp=A661$ mkdir dir
> > > todd@ispdesk/p6 ~/tmp=A662$ touch a
> > > todd@ispdesk/p6 ~/tmp=A663$ ln a b
> > > todd@ispdesk/p6 ~/tmp=A664$ ls -l a b
> > > -rw-r--r--  2 4  wheel  0 Oct 18 17:09 a
> > > -rw-r--r--  2 4  wheel  0 Oct 18 17:09 b
> > > todd@ispdesk/p6 ~/tmp=A665$ ls -li a b
> > > 2068032 -rw-r--r--  2 4  wheel  0 Oct 18 17:09 a
> > > 2068032 -rw-r--r--  2 4  wheel  0 Oct 18 17:09 b
> > > todd@ispdesk/p6 ~/tmp=A666$ ln a dir/b
> > > ln: dir/b: File exists
> > > todd@ispdesk/p6 ~/tmp=A667$ echo $?
> >
> > That error is just bogus on afs. If it returned a sane
> > error, things would just work.
> >
> > But, looks like afs only supports linking within the same
> > directory: http://www.angelfire.com/hi/plutonic/afs-faq.html
>
> So according to that error message from "ln" we really should have
> fallen into that Coda hack I just mentioned.  Did we instead get
> a different errno here and not use that hack?
>
>
> We probably could just use rename as you do above but then we would
> want to remove the unlink(tmpfile) call on l.2013 in sha1_file.c.
> Otherwise we're always incurring a syscall for no reason.  I'm not
> against a change here, I just want to make sure we make the right
> change for AFS.  :-)



-- 
Todd Fries .. todd@fries.net

 _____________________________________________
|                                             \  1.636.410.0632 (voice)
| Free Daemon Consulting                      \  1.405.227.9094 (voice)
| http://FreeDaemonConsulting.com             \  1.866.792.3418 (FAX)
| "..in support of free software solutions."  \          250797 (FWD)
|                                             \
 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\
                                                 
              37E7 D3EB 74D0 8D66 A68D  B866 0326 204E 3F42 004A
                        http://todd.fries.net/pgp.txt



[-- Attachment #2: 0002-AFS-inconveniently-returns-EEXIST-from-link-to-say.patch --]
[-- Type: text/x-diff, Size: 1125 bytes --]

From e3fe1eae139dccb9738cf0c8f6818136be96657b Mon Sep 17 00:00:00 2001
From: Todd T. Fries <todd@fries.net>
Date: Fri, 19 Oct 2007 07:38:16 -0500
Subject: [PATCH] AFS inconveniently returns EEXIST from link() to say it
doesn't like cross directory link()'s.  Do a little
extra work to fix this by ignoring EEXIST and trying
a rename() anyway.

Signed-off-by: Todd T. Fries <todd@fries.net>
---
 sha1_file.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 83a06a7..9c7d74e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2004,8 +2004,13 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
 	 *
 	 * When this succeeds, we just return 0. We have nothing
 	 * left to unlink.
+	 *
+	 * AFS hack - afs is similar to coda, but inconveniently
+	 * set errno to EEXIST, so call rename() if the link()
+	 * above fails unconditionally.  Small bit of extra work
+	 * so afs functions properly.
 	 */
-	if (ret && ret != EEXIST) {
+	if (ret) {
 		if (!rename(tmpfile, filename))
 			return 0;
 		ret = errno;
-- 
1.5.2.5

Re: git on afs

[Linus Torvalds]

On Fri, 19 Oct 2007, Todd T. Fries wrote:
> 
> If DT_UNKNOWN exists, then we have to do a stat() of some form to
> find out the right type.
> 
> This is difficult to fix properly to avoid the extra stat() since
> inside the switch logic we do the recursion, but we might have
> avoided it earlier because of the exclusion.
> 
> I'll send a separate diff for an updated link() vs rename() diff.
> 
> I've attached an updated diff that should address concerns of everyone
> who gave me feedback on my dir.c changes.
> 
> Better?

Yes. I think this is ok, although I also did this alternate patch that is 
anal about not bothering to call "lstat()" if it can decide that the path 
is ignored even before that.

That happened in the case of a pathname that was ignored, and we did not 
ask for "dir->show_ignored". That test used to be *together* with the 
"DTYPE(de) != DT_DIR", but splitting the two tests up means that we can do 
that (common) test before we even bother to calculate the real dtype.

Of course, that optimization only matters for systems that don't have, or 
don't fill in DTYPE properly, but I get a bit anal about these kinds of 
optimizations.

I also clarified the real relationship between "exclude" and 
"dir->show_ignored". It used to do

	if (exclude != dir->show_ignored) {
		..

which wasn't exactly obvious, because it triggers for two different cases:

 - the path is marked excluded, but we are not interested in ignored 
   files: ignore it

 - the path is *not* excluded, but we *are* interested in ignored files: 
   ignore it unless it's a directory, in which case we might have ignored 
   files inside the directory and need to recurse into it).

so this splits them into those two cases, since the first case doesn't 
even care about the type.

I also made a the DT_UNKNOWN case a separate helper function, and added 
some commentary to the cases.

Does this patch work for you?

		Linus

---
 dir.c |   52 ++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/dir.c b/dir.c
index eb6c3ab..f843c4d 100644
--- a/dir.c
+++ b/dir.c
@@ -443,6 +443,24 @@ static int in_pathspec(const char *path, int len, const struct path_simplify *si
 	return 0;
 }
 
+static int get_dtype(struct dirent *de, const char *path)
+{
+	int dtype = DTYPE(de);
+	struct stat st;
+
+	if (dtype != DT_UNKNOWN)
+		return dtype;
+	if (lstat(path, &st))
+		return dtype;
+	if (S_ISREG(st.st_mode))
+		return DT_REG;
+	if (S_ISDIR(st.st_mode))
+		return DT_DIR;
+	if (S_ISLNK(st.st_mode))
+		return DT_LNK;
+	return dtype;
+}
+
 /*
  * Read a directory tree. We currently ignore anything but
  * directories, regular files and symlinks. That's because git
@@ -466,7 +484,7 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
 		exclude_stk = push_exclude_per_directory(dir, base, baselen);
 
 		while ((de = readdir(fdir)) != NULL) {
-			int len;
+			int len, dtype;
 			int exclude;
 
 			if ((de->d_name[0] == '.') &&
@@ -486,24 +504,30 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co
 			if (exclude && dir->collect_ignored
 			    && in_pathspec(fullname, baselen + len, simplify))
 				dir_add_ignored(dir, fullname, baselen + len);
-			if (exclude != dir->show_ignored) {
-				if (!dir->show_ignored || DTYPE(de) != DT_DIR) {
+
+			/*
+			 * Excluded? If we don't explicitly want to show
+			 * ignored files, ignore it
+			 */
+			if (exclude && !dir->show_ignored)
+				continue;
+
+			dtype = get_dtype(de, fullname);
+
+			/*
+			 * Do we want to see just the ignored files?
+			 * We still need to recurse into directories,
+			 * even if we don't ignore them, since the
+			 * directory may contain files that we do..
+			 */
+			if (!exclude && dir->show_ignored) {
+				if (dtype != DT_DIR)
 					continue;
-				}
 			}
 
-			switch (DTYPE(de)) {
-			struct stat st;
+			switch (dtype) {
 			default:
 				continue;
-			case DT_UNKNOWN:
-				if (lstat(fullname, &st))
-					continue;
-				if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode))
-					break;
-				if (!S_ISDIR(st.st_mode))
-					continue;
-				/* fallthrough */
 			case DT_DIR:
 				memcpy(fullname + baselen + len, "/", 2);
 				len++;

Re: git on afs

[Linus Torvalds]

On Fri, 19 Oct 2007, Shawn O. Pearce wrote:
> 
> What about this instead?  It avoids the double lstat() of Todd's
> original patch but seems like it would fix the issue here.  Or did
> I misunderstand the problem?

No. As far as I can tell without testing it, this patch will make it show 
ignored regular files if they have DT_UNKNOWN in the directory entry (or 
the filesystem doesn't support it). Not good.

		Linus

Re: git on afs

[Daniel Barkalow]

On Fri, 19 Oct 2007, Todd T. Fries wrote:

> You're the second one to point out that I should check the errno of link() on
> afs.
> 
> It should not matter, but I'm using arla's afs client on OpenBSD; the errno
> is 17 (EEXIST), the very errno that bypasses the coda hack's rename():

According to a quick web search, OpenAFS gives EXDEV (unless there are 
other errors that apply). Getting EEXIST when the new path doesn't exist 
sounds like a bug in arla to me, although it could also be an AFS server 
issue, AFAICT.

	-Daniel
*This .sig left intentionally blank*

Re: git on afs

[Shawn O. Pearce]

"Todd T. Fries" <todd@fries.net> wrote:
> It should not matter, but I'm using arla's afs client on OpenBSD; the errno
> is 17 (EEXIST), the very errno that bypasses the coda hack's rename():
...
> I can assure you that the 2nd argument to link does not exist ;-)
...
> The only downside is that either on coda or if the file already exists, it
> will try a spurrous rename(), in which case it will fail with EEXIST again,
> so I hope this is not a big negative.

Actually there is a really big downside.  rename() is defined to
unlink the destination if it already exists, so you'll never get
EEXIST from a rename() call.

This means that an existing (known to be good) object can be
overwritten as a result of a rename with another copy of the object.
We've never really had that behavior as part of our security model
has been to always trust what is already in the repository and
refuse to replace something we already have.
 
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2004,8 +2004,13 @@ int move_temp_to_file(const char *tmpfile, const char *filename)
>  	 *
>  	 * When this succeeds, we just return 0. We have nothing
>  	 * left to unlink.
> +	 *
> +	 * AFS hack - afs is similar to coda, but inconveniently
> +	 * set errno to EEXIST, so call rename() if the link()
> +	 * above fails unconditionally.  Small bit of extra work
> +	 * so afs functions properly.
>  	 */
> -	if (ret && ret != EEXIST) {
> +	if (ret) {
>  		if (!rename(tmpfile, filename))
>  			return 0;
>  		ret = errno;

This is very unfortunate.  There's no way to tell that the file
already exists.  This whole AFS link() returning EEXIST is sort of
like the unlink() call on Solaris UFS working on directories as root
and leaving corrupted filesystems.  At some point the application
just cannot be reasonably expected to work on a system that acts
this insane.

I think I would rather change sha1_file.c to write temporary files
into the destination directory, rather than one level up and try to
hardlink them into position.  At least there we can rely on hardlinks
within Coda and AFS, and only need this rename special case for FAT.

-- 
Shawn.