{"thread":{"id":"10368","subject":"git on afs","startedAt":"2007-10-18T20:31:08Z","lastAt":"2007-10-20T05:29:06Z","messageCount":12,"participants":["Todd T. Fries","Brandon Casey","Linus Torvalds","Shawn O. Pearce","Daniel Barkalow"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"56427","messageId":"20071018203106.GA13518@fries.net","threadId":"10368","inReplyTo":null,"subject":"git on afs","fromName":"Todd T. Fries","fromEmail":"todd@fries.net","sentAt":"2007-10-18T20:31:08Z","receivedAt":"2007-10-18T20:31:08Z","isPatch":false,"sender":{"key":"todd@fries.net","avatar":null},"body":"I have always been frustrated by trying git on afs.\n\nThe other day I finally decided to look into why.\n\nLooks like two reasons caused my frustrations.\n\n1) git presumes that link() works fine across subdirs; in afs land,\n hardlinks do not succeed ever\n\n2) git presumes that DTYPE(de) != DT_DIR .. means the dirent is not a dir\n this is not true for afs\n\nI have been using this to sync several git trees, including linux-2.6 for the\npast week without issues writing to a local afs server.\n\nWhat do you guys think?\n\ndiff --git a/dir.c b/dir.c\nindex eb6c3ab..a3e53a5 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -487,9 +487,19 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co\n \t\t\t && in_pathspec(fullname, baselen + len, simplify))\n \t\t\t\tdir_add_ignored(dir, fullname, baselen + len);\n \t\t\tif (exclude != dir->show_ignored) {\n-\t\t\t\tif (!dir->show_ignored || DTYPE(de) != DT_DIR) {\n+\t\t\t\tif (!dir->show_ignored)\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n+\t\t\t\tif (DTYPE(de) == DT_UNKNOWN) {\n+\t\t\t\t\tstruct stat st;\n+\t\t\t\t\tif (lstat(fullname, &st))\n+\t\t\t\t\t\tcontinue;\n+\t\t\t\t\tif (!S_ISDIR(st.st_mode))\n+\t\t\t\t\t\tcontinue;\n+\t\t\t\t} else {\n+\t\t\t\t\tif (DTYPE(de) != DT_DIR)\n+\t\t\t\t\t\tcontinue;\n+\t\t\t\t}\n \t\t\t}\n \n \t\t\tswitch (DTYPE(de)) {\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 83a06a7..1b93322 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1961,7 +1961,7 @@ static int link_temp_to_file(const char *tmpfile, const char *filename)\n \tint ret;\n \tchar *dir;\n \n-\tif (!link(tmpfile, filename))\n+\tif (!rename(tmpfile, filename))\n \t\treturn 0;\n \n \t/*\n@@ -1980,7 +1980,7 @@ static int link_temp_to_file(const char *tmpfile, const char *filename)\n \t\t\treturn -2;\n \t\t}\n \t\t*dir = '/';\n-\t\tif (!link(tmpfile, filename))\n+\t\tif (!rename(tmpfile, filename))\n \t\t\treturn 0;\n \t\tret = errno;\n \t}\n-- \nTodd Fries .. todd@fries.net\n\n _____________________________________________\n| \\ 1.636.410.0632 (voice)\n| Free Daemon Consulting, LLC \\ 1.405.227.9094 (voice)\n| http://FreeDaemonConsulting.com \\ 1.866.792.3418 (FAX)\n| \"..in support of free software solutions.\" \\ 250797 (FWD)\n| \\\n \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\n \n 37E7 D3EB 74D0 8D66 A68D B866 0326 204E 3F42 004A\n http://todd.fries.net/pgp.txt\n"},{"id":"56429","messageId":"Pine.LNX.4.64.0710181613420.30020@torch.nrlssc.navy.mil","threadId":"10368","inReplyTo":"20071018203106.GA13518@fries.net","subject":"Re: git on afs","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2007-10-18T21:28:01Z","receivedAt":"2007-10-18T21:28:01Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Thu, 18 Oct 2007, Todd T. Fries wrote:\n\n> 1) git presumes that link() works fine across subdirs; in afs land,\n> hardlinks do not succeed ever\n\nI think this already falls back to rename() on failure.\nTake a look at move_temp_to_file(), this is the only\nplace that calls link_temp_to_file().\n\nlink() on afs returns non-zero? and not EEXIST right?\n\n-brandon\n"},{"id":"56430","messageId":"Pine.LNX.4.64.0710181645580.30020@torch.nrlssc.navy.mil","threadId":"10368","inReplyTo":"20071018203106.GA13518@fries.net","subject":"Re: git on afs","fromName":"Brandon Casey","fromEmail":"casey@nrlssc.navy.mil","sentAt":"2007-10-18T21:57:32Z","receivedAt":"2007-10-18T21:57:32Z","isPatch":false,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Thu, 18 Oct 2007, Todd T. Fries wrote:\n\n> 2) git presumes that DTYPE(de) != DT_DIR .. means the dirent is not a dir\n> this is not true for afs\n>\n> I have been using this to sync several git trees, including linux-2.6 for the\n> past week without issues writing to a local afs server.\n>\n> What do you guys think?\n>\n> diff --git a/dir.c b/dir.c\n> index eb6c3ab..a3e53a5 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -487,9 +487,19 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co\n> \t\t\t && in_pathspec(fullname, baselen + len, simplify))\n> \t\t\t\tdir_add_ignored(dir, fullname, baselen + len);\n> \t\t\tif (exclude != dir->show_ignored) {\n> -\t\t\t\tif (!dir->show_ignored || DTYPE(de) != DT_DIR) {\n> +\t\t\t\tif (!dir->show_ignored)\n\nyou're missing an open bracket ^^^\n\n> \t\t\t\t\tcontinue;\n> \t\t\t\t}\n\nor delete this one ^^^\n\n> +\t\t\t\tif (DTYPE(de) == DT_UNKNOWN) {\n> +\t\t\t\t\tstruct stat st;\n> +\t\t\t\t\tif (lstat(fullname, &st))\n> +\t\t\t\t\t\tcontinue;\n> +\t\t\t\t\tif (!S_ISDIR(st.st_mode))\n> +\t\t\t\t\t\tcontinue;\n> +\t\t\t\t} else {\n> +\t\t\t\t\tif (DTYPE(de) != DT_DIR)\n> +\t\t\t\t\t\tcontinue;\n> +\t\t\t\t}\n> \t\t\t}\n>\n> \t\t\tswitch (DTYPE(de)) {\n\nseems sane to me.\n\nIf no one else brings up any issues, you should retest and submit\na signed patch with a good comment. Use -s with git-commit.\nThen git-format-patch will produce the proper output.\n\n-brandon\n"},{"id":"56438","messageId":"alpine.LFD.0.999.0710181543380.26902@woody.linux-foundation.org","threadId":"10368","inReplyTo":"20071018203106.GA13518@fries.net","subject":"Re: git on afs","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-10-18T22:47:00Z","receivedAt":"2007-10-18T22:47:00Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 18 Oct 2007, Todd T. Fries wrote:\n> \n> 2) git presumes that DTYPE(de) != DT_DIR .. means the dirent is not a dir\n> this is not true for afs\n\nThat's a major bug, and has nothing to do with AFS. Oops. \n\nIf you look just a bit lower, you'll see that just a few lines down, git \nhandles DT_UNKNOWN correctly, and just does a lstat() on it as required. I \nguess that logic should be moved up, or alternatively the exclude logic \nshould be moved down.\n\nYour patch looks ok, but at the same time, I don't think it's really the \nright thing to do, since it now does that lstat() twice.\n\n\t\tLinus\n"},{"id":"56522","messageId":"20071019054814.GJ14735@spearce.org","threadId":"10368","inReplyTo":"20071018203106.GA13518@fries.net","subject":"Re: git on afs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-10-19T05:48:14Z","receivedAt":"2007-10-19T05:48:14Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"There's two different issues here so I'm going to split the thread\ninto two to simplify the discussion. Well, for me anyway. ;-)\n\n\"Todd T. Fries\" wrote:\n> 1) git presumes that link() works fine across subdirs; in afs land,\n> hardlinks do not succeed ever\n...\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 83a06a7..1b93322 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1961,7 +1961,7 @@ static int link_temp_to_file(const char *tmpfile, const char *filename)\n> \tint ret;\n> \tchar *dir;\n> \n> -\tif (!link(tmpfile, filename))\n> +\tif (!rename(tmpfile, filename))\n> \t\treturn 0;\n> \n> \t/*\n> @@ -1980,7 +1980,7 @@ static int link_temp_to_file(const char *tmpfile, const char *filename)\n> \t\t\treturn -2;\n> \t\t}\n> \t\t*dir = '/';\n> -\t\tif (!link(tmpfile, filename))\n> +\t\tif (!rename(tmpfile, filename))\n> \t\t\treturn 0;\n> \t\tret = errno;\n> \t}\n\nThese cases should already be handled lower, see l.1997-2012 of\nsha1_file.c:\n\n /*\n * Coda hack - coda doesn't like cross-directory links,\n * so we fall back to a rename, which will mean that it\n * won't be able to check collisions, but that's not a\n * big deal.\n *\n * The same holds for FAT formatted media.\n *\n * When this succeeds, we just return 0. We have nothing\n * left to unlink.\n */\n if (ret && ret != EEXIST) {\n if (!rename(tmpfile, filename))\n\n> Brandon Casey wrote:\n> On Thu, 18 Oct 2007, Todd T. Fries wrote:\n> \n> > link() returns -1 errno 17 File exists on afs.\n> >\n> > To further muddy the waters, linking within the same dir is ok,\n> > linking outside the same dir is not:\n> >\n> > todd@ispdesk/p6 ~/tmp=A661$ mkdir dir\n> > todd@ispdesk/p6 ~/tmp=A662$ touch a\n> > todd@ispdesk/p6 ~/tmp=A663$ ln a b\n> > todd@ispdesk/p6 ~/tmp=A664$ ls -l a b\n> > -rw-r--r-- 2 4 wheel 0 Oct 18 17:09 a\n> > -rw-r--r-- 2 4 wheel 0 Oct 18 17:09 b\n> > todd@ispdesk/p6 ~/tmp=A665$ ls -li a b\n> > 2068032 -rw-r--r-- 2 4 wheel 0 Oct 18 17:09 a\n> > 2068032 -rw-r--r-- 2 4 wheel 0 Oct 18 17:09 b\n> > todd@ispdesk/p6 ~/tmp=A666$ ln a dir/b\n> > ln: dir/b: File exists\n> > todd@ispdesk/p6 ~/tmp=A667$ echo $?\n> \n> That error is just bogus on afs. If it returned a sane\n> error, things would just work.\n> \n> But, looks like afs only supports linking within the same\n> directory: http://www.angelfire.com/hi/plutonic/afs-faq.html\n\nSo according to that error message from \"ln\" we really should have\nfallen into that Coda hack I just mentioned. Did we instead get\na different errno here and not use that hack?\n\n\nWe probably could just use rename as you do above but then we would\nwant to remove the unlink(tmpfile) call on l.2013 in sha1_file.c.\nOtherwise we're always incurring a syscall for no reason. I'm not\nagainst a change here, I just want to make sure we make the right\nchange for AFS. :-)\n\n-- \nShawn.\n"},{"id":"56523","messageId":"20071019060624.GK14735@spearce.org","threadId":"10368","inReplyTo":"alpine.LFD.0.999.0710181543380.26902@woody.linux-foundation.org","subject":"Re: git on afs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-10-19T06:06:24Z","receivedAt":"2007-10-19T06:06:24Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Linus Torvalds wrote:\n> On Thu, 18 Oct 2007, Todd T. Fries wrote:\n> > \n> > 2) git presumes that DTYPE(de) != DT_DIR .. means the dirent is not a dir\n> > this is not true for afs\n> \n> That's a major bug, and has nothing to do with AFS. Oops. \n> \n> If you look just a bit lower, you'll see that just a few lines down, git \n> handles DT_UNKNOWN correctly, and just does a lstat() on it as required. I \n> guess that logic should be moved up, or alternatively the exclude logic \n> should be moved down.\n> \n> Your patch looks ok, but at the same time, I don't think it's really the \n> right thing to do, since it now does that lstat() twice.\n\nWhat about this instead? It avoids the double lstat() of Todd's\noriginal patch but seems like it would fix the issue here. Or did\nI misunderstand the problem?\n\n\n--8>--\nFrom f290fc5f6ec5042b7c0393a300e4d3738b9090b8 Mon Sep 17 00:00:00 2001\nFrom: Shawn O. Pearce \nDate: Fri, 19 Oct 2007 02:03:55 -0400\nSubject: [PATCH] Correctly scan directories when DTYPE(de) is unknown\n\nWe have been presuming that if DTYPE(de) != DT_DIR than the item we\nobtained from readdir() isn't a directory. This is actually not\ntrue, it could still be a directory, especially if the underlying\nsystem we were compiled for doesn't have the d_type member of struct\ndirent and we have enabled our compatability definition of DTYPE(de)\nto always be DT_UNKNOWN.\n\nIf DTYPE(de) == DT_UNKNOWN we need to fall through and perform an\nactual lstat() call to determine what the type of this item is so\nwe know how to proceed with our scanning.\n\nReported by Todd T. Fries while trying to use Git on AFS, which\napparently has DTYPE(de) != DT_DIR for directories.\n\nSigned-off-by: Shawn O. Pearce \n---\n dir.c | 5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex eb6c3ab..d2597ff 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -487,9 +487,10 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co\n \t\t\t && in_pathspec(fullname, baselen + len, simplify))\n \t\t\t\tdir_add_ignored(dir, fullname, baselen + len);\n \t\t\tif (exclude != dir->show_ignored) {\n-\t\t\t\tif (!dir->show_ignored || DTYPE(de) != DT_DIR) {\n+\t\t\t\tif (!dir->show_ignored)\n+\t\t\t\t\tcontinue;\n+\t\t\t\telse if (DTYPE(de) != DT_DIR && DTYPE(de) != DT_UNKNOWN)\n \t\t\t\t\tcontinue;\n-\t\t\t\t}\n \t\t\t}\n \n \t\t\tswitch (DTYPE(de)) {\n-- \n1.5.3.4.1249.g895be\n\n-- \nShawn.\n"},{"id":"56564","messageId":"200710190719.21660.todd@fries.net","threadId":"10368","inReplyTo":"20071019060624.GK14735@spearce.org","subject":"Re: git on afs","fromName":"Todd T. Fries","fromEmail":"todd@fries.net","sentAt":"2007-10-19T12:19:19Z","receivedAt":"2007-10-19T12:19:19Z","isPatch":false,"sender":{"key":"todd@fries.net","avatar":null},"body":"Shawn,\n\nIf DT_UNKNOWN exists, then we have to do a stat() of some form to\nfind out the right type.\n\nThis is difficult to fix properly to avoid the extra stat() since\ninside the switch logic we do the recursion, but we might have\navoided it earlier because of the exclusion.\n\nI'll send a separate diff for an updated link() vs rename() diff.\n\nI've attached an updated diff that should address concerns of everyone\nwho gave me feedback on my dir.c changes.\n\nBetter?\n\nThanks,\n\nOn Friday 19 October 2007 01:06:24 Shawn O. Pearce wrote:\n> Linus Torvalds wrote:\n> > On Thu, 18 Oct 2007, Todd T. Fries wrote:\n> > > 2) git presumes that DTYPE(de) != DT_DIR .. means the dirent is not a\n> > > dir this is not true for afs\n> >\n> > That's a major bug, and has nothing to do with AFS. Oops.\n> >\n> > If you look just a bit lower, you'll see that just a few lines down, git\n> > handles DT_UNKNOWN correctly, and just does a lstat() on it as required.\n> > I guess that logic should be moved up, or alternatively the exclude logic\n> > should be moved down.\n> >\n> > Your patch looks ok, but at the same time, I don't think it's really the\n> > right thing to do, since it now does that lstat() twice.\n>\n> What about this instead? It avoids the double lstat() of Todd's\n> original patch but seems like it would fix the issue here. Or did\n> I misunderstand the problem?\n[..]\n> diff --git a/dir.c b/dir.c\n> index eb6c3ab..d2597ff 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -487,9 +487,10 @@ static int read_directory_recursive(struct dir_struct\n> *dir, const char *path, co && in_pathspec(fullname, baselen + len,\n> simplify))\n> \t\t\t\tdir_add_ignored(dir, fullname, baselen + len);\n> \t\t\tif (exclude != dir->show_ignored) {\n> -\t\t\t\tif (!dir->show_ignored || DTYPE(de) != DT_DIR) {\n> +\t\t\t\tif (!dir->show_ignored)\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\telse if (DTYPE(de) != DT_DIR && DTYPE(de) != DT_UNKNOWN)\n> \t\t\t\t\tcontinue;\n> -\t\t\t\t}\n> \t\t\t}\n>\n> \t\t\tswitch (DTYPE(de)) {\n> --\n> 1.5.3.4.1249.g895be\n\n\n\n-- \nTodd Fries .. todd@fries.net\n\n _____________________________________________\n| \\ 1.636.410.0632 (voice)\n| Free Daemon Consulting \\ 1.405.227.9094 (voice)\n| http://FreeDaemonConsulting.com \\ 1.866.792.3418 (FAX)\n| \"..in support of free software solutions.\" \\ 250797 (FWD)\n| \\\n \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\n \n 37E7 D3EB 74D0 8D66 A68D B866 0326 204E 3F42 004A\n http://todd.fries.net/pgp.txt\n\n\n\n\n\n\n\nFrom f7bca472645db70b52824e1be827ba99195198d2 Mon Sep 17 00:00:00 2001\nFrom: Todd T. Fries \nDate: Fri, 19 Oct 2007 06:26:49 -0500\nSubject: [PATCH] If readdir() returns a DTYPE() of DT_UNKNOWN, this warrants further\ninvestigation, i.e. a stat() or lstat(). We have been presuming\nanything not DT_DIR is not a dir, which is not the case for\nall filesystems (e.g. afs).\n\nDo the lstat() once, and use the results twice.\n\nSigned-off-by: Todd T. Fries \n---\n dir.c | 16 +++++++++++-----\n 1 files changed, 11 insertions(+), 5 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex eb6c3ab..0ed4739 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -468,6 +468,7 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co\n \t\twhile ((de = readdir(fdir)) != NULL) {\n \t\t\tint len;\n \t\t\tint exclude;\n+\t\t\tstruct stat st;\n \n \t\t\tif ((de->d_name[0] == '.') &&\n \t\t\t (de->d_name[1] == 0 ||\n@@ -486,19 +487,24 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co\n \t\t\tif (exclude && dir->collect_ignored\n \t\t\t && in_pathspec(fullname, baselen + len, simplify))\n \t\t\t\tdir_add_ignored(dir, fullname, baselen + len);\n+\t\t\tif (DTYPE(de) == DT_UNKNOWN) {\n+\t\t\t\tif (lstat(fullname, &st))\n+\t\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (exclude != dir->show_ignored) {\n-\t\t\t\tif (!dir->show_ignored || DTYPE(de) != DT_DIR) {\n+\t\t\t\tif (!dir->show_ignored))\n \t\t\t\t\tcontinue;\n-\t\t\t\t}\n+\t\t\t\tif (DTYPE(de) == DT_UNKNOWN && !S_ISDIR(st.st_mode))\n+\t\t\t\t\tcontinue;\n+\t\t\t\telse\n+\t\t\t\t\tif (DTYPE(de) != DT_DIR)\n+\t\t\t\t\t\tcontinue;\n \t\t\t}\n \n \t\t\tswitch (DTYPE(de)) {\n-\t\t\tstruct stat st;\n \t\t\tdefault:\n \t\t\t\tcontinue;\n \t\t\tcase DT_UNKNOWN:\n-\t\t\t\tif (lstat(fullname, &st))\n-\t\t\t\t\tcontinue;\n \t\t\t\tif (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode))\n \t\t\t\t\tbreak;\n \t\t\t\tif (!S_ISDIR(st.st_mode))\n-- \n1.5.2.5\n\n"},{"id":"56566","messageId":"200710190742.08174.todd@fries.net","threadId":"10368","inReplyTo":"20071019054814.GJ14735@spearce.org","subject":"Re: git on afs","fromName":"Todd T. Fries","fromEmail":"todd@fries.net","sentAt":"2007-10-19T12:42:07Z","receivedAt":"2007-10-19T12:42:07Z","isPatch":false,"sender":{"key":"todd@fries.net","avatar":null},"body":"You're the second one to point out that I should check the errno of link() on\nafs.\n\nIt should not matter, but I'm using arla's afs client on OpenBSD; the errno\nis 17 (EEXIST), the very errno that bypasses the coda hack's rename():\n\n 8484 git-index-pack CALL mkdir(0xcfbdc750,0x1ff)\n 8484 git-index-pack NAMI \".git/objects/pack\"\n 8484 git-index-pack RET mkdir -1 errno 17 File exists\n 8484 git-index-pack CALL link(0x829e7040,0xcfbdc750)\n 8484 git-index-pack NAMI \".git/objects/tmp_pack_BU8484\"\n 8484 git-index-pack \nNAMI \".git/objects/pack/pack-db66c6d25d6e0044093252434b4aa2a7d67e386a.pack\"\n 8484 git-index-pack RET link -1 errno 17 File exists\n\n.. and does not go on to do a rename(), which based on the code makes sense.\n\nI can assure you that the 2nd argument to link does not exist ;-)\n\nSo, I've made the following change and it seems to fix the problem, much\ncleaner IMHO, and get the following change in behavior:\n\n 5933 git-index-pack CALL mkdir(0xcfbc0070,0x1ff)\n 5933 git-index-pack NAMI \".git/objects/pack\"\n 5933 git-index-pack RET mkdir -1 errno 17 File exists\n 5933 git-index-pack CALL link(0x860c0040,0xcfbc0070)\n 5933 git-index-pack NAMI \".git/objects/tmp_pack_EV5933\"\n 5933 git-index-pack \nNAMI \".git/objects/pack/pack-db66c6d25d6e0044093252434b4aa2a7d67e386a.pack\"\n 5933 git-index-pack RET link -1 errno 17 File exists\n[..]\n 5933 git-index-pack CALL rename(0x860c0040,0xcfbc0070)\n 5933 git-index-pack NAMI \".git/objects/tmp_pack_EV5933\"\n 5933 git-index-pack \nNAMI \".git/objects/pack/pack-db66c6d25d6e0044093252434b4aa2a7d67e386a.pack\"\n 5933 git-index-pack RET rename 0\n\nThe only downside is that either on coda or if the file already exists, it\nwill try a spurrous rename(), in which case it will fail with EEXIST again,\nso I hope this is not a big negative.\n\nIf this is ok, an appropriate commit message might be something like:\n\n AFS inconveniently returns EEXIST from link() to say it\n doesn't like cross directory link()'s. Do a little\n extra work to fix this by ignoring EEXIST and trying\n a rename() anyway.\n\n\nThanks,\n\nOn Friday 19 October 2007 00:48:14 Shawn O. Pearce wrote:\n> There's two different issues here so I'm going to split the thread\n> into two to simplify the discussion. Well, for me anyway. ;-)\n>\n> \"Todd T. Fries\" wrote:\n> > 1) git presumes that link() works fine across subdirs; in afs land,\n> > hardlinks do not succeed ever\n>\n> ...\n>\n> > diff --git a/sha1_file.c b/sha1_file.c\n> > index 83a06a7..1b93322 100644\n> > --- a/sha1_file.c\n> > +++ b/sha1_file.c\n> > @@ -1961,7 +1961,7 @@ static int link_temp_to_file(const char *tmpfile,\n> > const char *filename) int ret;\n> > \tchar *dir;\n> >\n> > -\tif (!link(tmpfile, filename))\n> > +\tif (!rename(tmpfile, filename))\n> > \t\treturn 0;\n> >\n> > \t/*\n> > @@ -1980,7 +1980,7 @@ static int link_temp_to_file(const char *tmpfile,\n> > const char *filename) return -2;\n> > \t\t}\n> > \t\t*dir = '/';\n> > -\t\tif (!link(tmpfile, filename))\n> > +\t\tif (!rename(tmpfile, filename))\n> > \t\t\treturn 0;\n> > \t\tret = errno;\n> > \t}\n>\n> These cases should already be handled lower, see l.1997-2012 of\n> sha1_file.c:\n>\n> /*\n> * Coda hack - coda doesn't like cross-directory links,\n> * so we fall back to a rename, which will mean that it\n> * won't be able to check collisions, but that's not a\n> * big deal.\n> *\n> * The same holds for FAT formatted media.\n> *\n> * When this succeeds, we just return 0. We have nothing\n> * left to unlink.\n> */\n> if (ret && ret != EEXIST) {\n> if (!rename(tmpfile, filename))\n>\n> > Brandon Casey wrote:\n> >\n> > On Thu, 18 Oct 2007, Todd T. Fries wrote:\n> > > link() returns -1 errno 17 File exists on afs.\n> > >\n> > > To further muddy the waters, linking within the same dir is ok,\n> > > linking outside the same dir is not:\n> > >\n> > > todd@ispdesk/p6 ~/tmp=A661$ mkdir dir\n> > > todd@ispdesk/p6 ~/tmp=A662$ touch a\n> > > todd@ispdesk/p6 ~/tmp=A663$ ln a b\n> > > todd@ispdesk/p6 ~/tmp=A664$ ls -l a b\n> > > -rw-r--r-- 2 4 wheel 0 Oct 18 17:09 a\n> > > -rw-r--r-- 2 4 wheel 0 Oct 18 17:09 b\n> > > todd@ispdesk/p6 ~/tmp=A665$ ls -li a b\n> > > 2068032 -rw-r--r-- 2 4 wheel 0 Oct 18 17:09 a\n> > > 2068032 -rw-r--r-- 2 4 wheel 0 Oct 18 17:09 b\n> > > todd@ispdesk/p6 ~/tmp=A666$ ln a dir/b\n> > > ln: dir/b: File exists\n> > > todd@ispdesk/p6 ~/tmp=A667$ echo $?\n> >\n> > That error is just bogus on afs. If it returned a sane\n> > error, things would just work.\n> >\n> > But, looks like afs only supports linking within the same\n> > directory: http://www.angelfire.com/hi/plutonic/afs-faq.html\n>\n> So according to that error message from \"ln\" we really should have\n> fallen into that Coda hack I just mentioned. Did we instead get\n> a different errno here and not use that hack?\n>\n>\n> We probably could just use rename as you do above but then we would\n> want to remove the unlink(tmpfile) call on l.2013 in sha1_file.c.\n> Otherwise we're always incurring a syscall for no reason. I'm not\n> against a change here, I just want to make sure we make the right\n> change for AFS. :-)\n\n\n\n-- \nTodd Fries .. todd@fries.net\n\n _____________________________________________\n| \\ 1.636.410.0632 (voice)\n| Free Daemon Consulting \\ 1.405.227.9094 (voice)\n| http://FreeDaemonConsulting.com \\ 1.866.792.3418 (FAX)\n| \"..in support of free software solutions.\" \\ 250797 (FWD)\n| \\\n \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\n \n 37E7 D3EB 74D0 8D66 A68D B866 0326 204E 3F42 004A\n http://todd.fries.net/pgp.txt\n\n\n\n\nFrom e3fe1eae139dccb9738cf0c8f6818136be96657b Mon Sep 17 00:00:00 2001\nFrom: Todd T. Fries \nDate: Fri, 19 Oct 2007 07:38:16 -0500\nSubject: [PATCH] AFS inconveniently returns EEXIST from link() to say it\ndoesn't like cross directory link()'s. Do a little\nextra work to fix this by ignoring EEXIST and trying\na rename() anyway.\n\nSigned-off-by: Todd T. Fries \n---\n sha1_file.c | 7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 83a06a7..9c7d74e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2004,8 +2004,13 @@ int move_temp_to_file(const char *tmpfile, const char *filename)\n \t *\n \t * When this succeeds, we just return 0. We have nothing\n \t * left to unlink.\n+\t *\n+\t * AFS hack - afs is similar to coda, but inconveniently\n+\t * set errno to EEXIST, so call rename() if the link()\n+\t * above fails unconditionally. Small bit of extra work\n+\t * so afs functions properly.\n \t */\n-\tif (ret && ret != EEXIST) {\n+\tif (ret) {\n \t\tif (!rename(tmpfile, filename))\n \t\t\treturn 0;\n \t\tret = errno;\n-- \n1.5.2.5\n\n"},{"id":"56607","messageId":"alpine.LFD.0.999.0710191049400.26902@woody.linux-foundation.org","threadId":"10368","inReplyTo":"200710190719.21660.todd@fries.net","subject":"Re: git on afs","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-10-19T17:59:22Z","receivedAt":"2007-10-19T17:59:22Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 19 Oct 2007, Todd T. Fries wrote:\n> \n> If DT_UNKNOWN exists, then we have to do a stat() of some form to\n> find out the right type.\n> \n> This is difficult to fix properly to avoid the extra stat() since\n> inside the switch logic we do the recursion, but we might have\n> avoided it earlier because of the exclusion.\n> \n> I'll send a separate diff for an updated link() vs rename() diff.\n> \n> I've attached an updated diff that should address concerns of everyone\n> who gave me feedback on my dir.c changes.\n> \n> Better?\n\nYes. I think this is ok, although I also did this alternate patch that is \nanal about not bothering to call \"lstat()\" if it can decide that the path \nis ignored even before that.\n\nThat happened in the case of a pathname that was ignored, and we did not \nask for \"dir->show_ignored\". That test used to be *together* with the \n\"DTYPE(de) != DT_DIR\", but splitting the two tests up means that we can do \nthat (common) test before we even bother to calculate the real dtype.\n\nOf course, that optimization only matters for systems that don't have, or \ndon't fill in DTYPE properly, but I get a bit anal about these kinds of \noptimizations.\n\nI also clarified the real relationship between \"exclude\" and \n\"dir->show_ignored\". It used to do\n\n\tif (exclude != dir->show_ignored) {\n\t\t..\n\nwhich wasn't exactly obvious, because it triggers for two different cases:\n\n - the path is marked excluded, but we are not interested in ignored \n files: ignore it\n\n - the path is *not* excluded, but we *are* interested in ignored files: \n ignore it unless it's a directory, in which case we might have ignored \n files inside the directory and need to recurse into it).\n\nso this splits them into those two cases, since the first case doesn't \neven care about the type.\n\nI also made a the DT_UNKNOWN case a separate helper function, and added \nsome commentary to the cases.\n\nDoes this patch work for you?\n\n\t\tLinus\n\n---\n dir.c | 52 ++++++++++++++++++++++++++++++++++++++--------------\n 1 files changed, 38 insertions(+), 14 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex eb6c3ab..f843c4d 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -443,6 +443,24 @@ static int in_pathspec(const char *path, int len, const struct path_simplify *si\n \treturn 0;\n }\n \n+static int get_dtype(struct dirent *de, const char *path)\n+{\n+\tint dtype = DTYPE(de);\n+\tstruct stat st;\n+\n+\tif (dtype != DT_UNKNOWN)\n+\t\treturn dtype;\n+\tif (lstat(path, &st))\n+\t\treturn dtype;\n+\tif (S_ISREG(st.st_mode))\n+\t\treturn DT_REG;\n+\tif (S_ISDIR(st.st_mode))\n+\t\treturn DT_DIR;\n+\tif (S_ISLNK(st.st_mode))\n+\t\treturn DT_LNK;\n+\treturn dtype;\n+}\n+\n /*\n * Read a directory tree. We currently ignore anything but\n * directories, regular files and symlinks. That's because git\n@@ -466,7 +484,7 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co\n \t\texclude_stk = push_exclude_per_directory(dir, base, baselen);\n \n \t\twhile ((de = readdir(fdir)) != NULL) {\n-\t\t\tint len;\n+\t\t\tint len, dtype;\n \t\t\tint exclude;\n \n \t\t\tif ((de->d_name[0] == '.') &&\n@@ -486,24 +504,30 @@ static int read_directory_recursive(struct dir_struct *dir, const char *path, co\n \t\t\tif (exclude && dir->collect_ignored\n \t\t\t && in_pathspec(fullname, baselen + len, simplify))\n \t\t\t\tdir_add_ignored(dir, fullname, baselen + len);\n-\t\t\tif (exclude != dir->show_ignored) {\n-\t\t\t\tif (!dir->show_ignored || DTYPE(de) != DT_DIR) {\n+\n+\t\t\t/*\n+\t\t\t * Excluded? If we don't explicitly want to show\n+\t\t\t * ignored files, ignore it\n+\t\t\t */\n+\t\t\tif (exclude && !dir->show_ignored)\n+\t\t\t\tcontinue;\n+\n+\t\t\tdtype = get_dtype(de, fullname);\n+\n+\t\t\t/*\n+\t\t\t * Do we want to see just the ignored files?\n+\t\t\t * We still need to recurse into directories,\n+\t\t\t * even if we don't ignore them, since the\n+\t\t\t * directory may contain files that we do..\n+\t\t\t */\n+\t\t\tif (!exclude && dir->show_ignored) {\n+\t\t\t\tif (dtype != DT_DIR)\n \t\t\t\t\tcontinue;\n-\t\t\t\t}\n \t\t\t}\n \n-\t\t\tswitch (DTYPE(de)) {\n-\t\t\tstruct stat st;\n+\t\t\tswitch (dtype) {\n \t\t\tdefault:\n \t\t\t\tcontinue;\n-\t\t\tcase DT_UNKNOWN:\n-\t\t\t\tif (lstat(fullname, &st))\n-\t\t\t\t\tcontinue;\n-\t\t\t\tif (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode))\n-\t\t\t\t\tbreak;\n-\t\t\t\tif (!S_ISDIR(st.st_mode))\n-\t\t\t\t\tcontinue;\n-\t\t\t\t/* fallthrough */\n \t\t\tcase DT_DIR:\n \t\t\t\tmemcpy(fullname + baselen + len, \"/\", 2);\n \t\t\t\tlen++;\n"},{"id":"56611","messageId":"alpine.LFD.0.999.0710191204550.26902@woody.linux-foundation.org","threadId":"10368","inReplyTo":"20071019060624.GK14735@spearce.org","subject":"Re: git on afs","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-10-19T19:06:33Z","receivedAt":"2007-10-19T19:06:33Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Fri, 19 Oct 2007, Shawn O. Pearce wrote:\n> \n> What about this instead? It avoids the double lstat() of Todd's\n> original patch but seems like it would fix the issue here. Or did\n> I misunderstand the problem?\n\nNo. As far as I can tell without testing it, this patch will make it show \nignored regular files if they have DT_UNKNOWN in the directory entry (or \nthe filesystem doesn't support it). Not good.\n\n\t\tLinus\n"},{"id":"56628","messageId":"Pine.LNX.4.64.0710191541360.32497@iabervon.org","threadId":"10368","inReplyTo":"200710190742.08174.todd@fries.net","subject":"Re: git on afs","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-10-19T20:19:45Z","receivedAt":"2007-10-19T20:19:45Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Fri, 19 Oct 2007, Todd T. Fries wrote:\n\n> You're the second one to point out that I should check the errno of link() on\n> afs.\n> \n> It should not matter, but I'm using arla's afs client on OpenBSD; the errno\n> is 17 (EEXIST), the very errno that bypasses the coda hack's rename():\n\nAccording to a quick web search, OpenAFS gives EXDEV (unless there are \nother errors that apply). Getting EEXIST when the new path doesn't exist \nsounds like a bug in arla to me, although it could also be an AFS server \nissue, AFAICT.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"56654","messageId":"20071020052906.GT14735@spearce.org","threadId":"10368","inReplyTo":"200710190742.08174.todd@fries.net","subject":"Re: git on afs","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-10-20T05:29:06Z","receivedAt":"2007-10-20T05:29:06Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"\"Todd T. Fries\" wrote:\n> It should not matter, but I'm using arla's afs client on OpenBSD; the errno\n> is 17 (EEXIST), the very errno that bypasses the coda hack's rename():\n...\n> I can assure you that the 2nd argument to link does not exist ;-)\n...\n> The only downside is that either on coda or if the file already exists, it\n> will try a spurrous rename(), in which case it will fail with EEXIST again,\n> so I hope this is not a big negative.\n\nActually there is a really big downside. rename() is defined to\nunlink the destination if it already exists, so you'll never get\nEEXIST from a rename() call.\n\nThis means that an existing (known to be good) object can be\noverwritten as a result of a rename with another copy of the object.\nWe've never really had that behavior as part of our security model\nhas been to always trust what is already in the repository and\nrefuse to replace something we already have.\n \n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2004,8 +2004,13 @@ int move_temp_to_file(const char *tmpfile, const char *filename)\n> \t *\n> \t * When this succeeds, we just return 0. We have nothing\n> \t * left to unlink.\n> +\t *\n> +\t * AFS hack - afs is similar to coda, but inconveniently\n> +\t * set errno to EEXIST, so call rename() if the link()\n> +\t * above fails unconditionally. Small bit of extra work\n> +\t * so afs functions properly.\n> \t */\n> -\tif (ret && ret != EEXIST) {\n> +\tif (ret) {\n> \t\tif (!rename(tmpfile, filename))\n> \t\t\treturn 0;\n> \t\tret = errno;\n\nThis is very unfortunate. There's no way to tell that the file\nalready exists. This whole AFS link() returning EEXIST is sort of\nlike the unlink() call on Solaris UFS working on directories as root\nand leaving corrupted filesystems. At some point the application\njust cannot be reasonably expected to work on a system that acts\nthis insane.\n\nI think I would rather change sha1_file.c to write temporary files\ninto the destination directory, rather than one level up and try to\nhardlink them into position. At least there we can rely on hardlinks\nwithin Coda and AFS, and only need this rename special case for FAT.\n\n-- \nShawn.\n"}]}