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@xxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxx
_____________________________________________
| \ 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@xxxxxxxxx>
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@xxxxxxxxx>
---
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
