threads / discuss / 10368

git on afs

Subject: git on afs

## tl;dr

12 messages between Oct 18, 2007 and Oct 20, 2007.

replies: 11people: 5as markdown or json

Todd T. Fries· Oct 18, 2007, 20:31 UTC · lore
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
Brandon Casey· Oct 18, 2007, 21:28 UTC · re: Todd T. Fries · lore

Re: git on afs

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
Brandon Casey· Oct 18, 2007, 21:57 UTC · re: Todd T. Fries · lore

Re: git on afs

On Thu, 18 Oct 2007, Todd T. Fries wrote:
Show 18 quoted lines
> 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             ^^^
Show 13 quoted lines
> +				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
Linus Torvalds· Oct 18, 2007, 22:47 UTC · re: Todd T. Fries · lore

Re: git on afs

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
Shawn O. Pearce· Oct 19, 2007, 06:06 UTC · re: Linus Torvalds · lore

Re: git on afs

Linus Torvalds <torvalds@linux-foundation.org> wrote:
Show 14 quoted lines
> 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.
Todd T. Fries· Oct 19, 2007, 12:19 UTC · re: Shawn O. Pearce · lore

Re: git on afs

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:
Show 18 quoted lines
> 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?
[..]
Show 20 quoted lines
> 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







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
Linus Torvalds· Oct 19, 2007, 17:59 UTC · re: Todd T. Fries · lore

Re: git on afs

On Fri, 19 Oct 2007, Todd T. Fries wrote:
Show 14 quoted lines
> 
> 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++;
Linus Torvalds· Oct 19, 2007, 19:06 UTC · re: Shawn O. Pearce · lore

Re: git on afs

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
Shawn O. Pearce· Oct 19, 2007, 05:48 UTC · re: Todd T. Fries · lore

Re: git on afs

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
...
Show 22 quoted lines
> 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))
Show 26 quoted lines
> 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.
Todd T. Fries· Oct 19, 2007, 12:42 UTC · re: Shawn O. Pearce · lore

Re: git on afs

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:
Show 86 quoted lines
> 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




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
Daniel Barkalow· Oct 19, 2007, 20:19 UTC · re: Todd T. Fries · lore

Re: git on afs

On Fri, 19 Oct 2007, Todd T. Fries wrote:
Show 5 quoted lines
> 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*
Shawn O. Pearce· Oct 20, 2007, 05:29 UTC · re: Todd T. Fries · lore

Re: git on afs

"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.
Show 17 quoted lines
> --- 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.

← back to recent threads