Fix bug that prevented transfer locks from working when run on SMB or other filesystem that does not support fcntl locks and hard links.

This commit was sponsored by Ethan Aubin.

4 files changed, 73 insertions, 6 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 6ace216f3..4991c8d33 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -19,6 +19,9 @@ git-annex (6.20170520) UNRELEASED; urgency=medium
   * Avoid error about git-annex-shell not being found when
     syncing with -J with a git remote where git-annex-shell is not
     installed.
+  * Fix bug that prevented transfer locks from working when
+    run on SMB or other filesystem that does not support fcntl locks
+    and hard links.
 
  -- Joey Hess <id@joeyh.name>  Wed, 24 May 2017 14:03:40 -0400
 
diff --git a/Utility/LockFile/PidLock.hs b/Utility/LockFile/PidLock.hs
index 23560fa57..0db9d1f3f 100644
--- a/Utility/LockFile/PidLock.hs
+++ b/Utility/LockFile/PidLock.hs
@@ -122,18 +122,21 @@ tryLock lockfile = trySideLock lockfile $ \sidelock -> do
 	setFileMode tmp (combineModes readModes)
 	hPutStr h . show =<< mkPidLock
 	hClose h
-	st <- getFileStatus tmp
-	let failedlock = do
+	let failedlock st = do
 		dropLock $ LockHandle tmp st sidelock
 		return Nothing
-	let tooklock = return $ Just $ LockHandle lockfile' st sidelock
+	let tooklock st = return $ Just $ LockHandle lockfile' st sidelock
 	ifM (linkToLock sidelock tmp lockfile')
 		( do
 			nukeFile tmp
-			tooklock
+			-- May not have made a hard link, so stat
+			-- the lockfile
+			lckst <- getFileStatus lockfile'
+			tooklock lckst
 		, do
 			v <- readPidLock lockfile'
 			hn <- getHostName
+			tmpst <- getFileStatus tmp
 			case v of
 				Just pl | isJust sidelock && hn == lockingHost pl -> do
 					-- Since we have the sidelock,
@@ -142,8 +145,8 @@ tryLock lockfile = trySideLock lockfile $ \sidelock -> do
 					-- we know that the pidlock is
 					-- stale, and can take it over.
 					rename tmp lockfile'
-					tooklock
-				_ -> failedlock
+					tooklock tmpst
+				_ -> failedlock tmpst
 		)
 
 -- Linux's open(2) man page recommends linking a pid lock into place,
diff --git a/doc/bugs/SMB__58___git_annex_clone_works__44___get_fails_on_transfer_lock.mdwn b/doc/bugs/SMB__58___git_annex_clone_works__44___get_fails_on_transfer_lock.mdwn
index 3077bd017..ef08680d4 100644
--- a/doc/bugs/SMB__58___git_annex_clone_works__44___get_fails_on_transfer_lock.mdwn
+++ b/doc/bugs/SMB__58___git_annex_clone_works__44___get_fails_on_transfer_lock.mdwn
@@ -245,3 +245,5 @@ and the relevant share:
 I'm an extremely regular user of git-annex on OS X and Linux, for several years, using it as a podcatcher and to manage most of my "large file" media.  It's one of those "couldn't live without" tools.  Thanks for writing it.
 
 (Touched, to ask for comments to be emailed to me)
+
+> [[fixed|done]] --[[Joey]]
diff --git a/doc/bugs/SMB__58___git_annex_clone_works__44___get_fails_on_transfer_lock/comment_5_8b5d03f64127aa6b4073ff10796b9771._comment b/doc/bugs/SMB__58___git_annex_clone_works__44___get_fails_on_transfer_lock/comment_5_8b5d03f64127aa6b4073ff10796b9771._comment
new file mode 100644
index 000000000..64b1feb8b
--- /dev/null
+++ b/doc/bugs/SMB__58___git_annex_clone_works__44___get_fails_on_transfer_lock/comment_5_8b5d03f64127aa6b4073ff10796b9771._comment
@@ -0,0 +1,59 @@
+[[!comment format=mdwn
+ username="joey"
+ subject="""comment 5"""
+ date="2017-06-06T17:54:42Z"
+ content="""
+Here's the relevant parts of the dtruss:
+
+	 7003/0x16c3126:    121483    3371    127 open("/Volumes/music/ga-test/.git/annex/locktmp16807282475249\0", 0x20A06, 0x180)              = 18 0
+	...
+	 7003/0x16c3126:    121792      10      6 stat64("/Volumes/music/ga-test/.git/annex/locktmp16807282475249\0", 0x200004940, 0x33)                 = 0 0
+	 7003/0x16c3126:    122041    2341    235 link("/Volumes/music/ga-test/.git/annex/locktmp16807282475249\0", "/Volumes/music/ga-test/.git/annex/locktmp16807282475249.lnk\0")             = -1 Err#45
+	 7003/0x16c3126:    122115     885     49 unlink("/Volumes/music/ga-test/.git/annex/locktmp16807282475249.lnk\0", 0x200004D80, 0x33)             = -1 Err#2
+	 7003/0x16c3126:    122126       5      1 sigreturn(0x7FFF56C9E390, 0x1E, 0x33)          = 0 Err#-2
+	 7003/0x16c3126:    122247    1050     90 open("/Volumes/music/ga-test/.git/annex/pidlock\0", 0xA01, 0x124)              = 18 0
+	 7003/0x16c3126:    122256       4      1 fcntl(0x12, 0x3, 0x0)          = 1 0
+	 7003/0x16c3126:    122260       5      2 fstat64(0x12, 0x20001A120, 0x0)                = 0 0
+	 7003/0x16c3126:    122273       4      1 ioctl(0x12, 0x4004667A, 0x7FFF56C9E3FC)                = -1 Err#25
+	 7003/0x16c3126:    122274       2      0 ioctl(0x12, 0x40487413, 0x7FFF56C9E400)                = -1 Err#25
+	 7003/0x16c3126:    122314     677     32 open("/Volumes/music/ga-test/.git/annex/locktmp16807282475249\0", 0x20004, 0x1B6)              = 19 0
+	 7003/0x16c3126:    122319       5      2 fstat64(0x13, 0x20001A310, 0x1B6)              = 0 0
+	 7003/0x16c3126:    122326       2      0 ioctl(0x13, 0x4004667A, 0x7FFF56C9E3FC)                = -1 Err#25
+	 7003/0x16c3126:    122327       2      0 ioctl(0x13, 0x40487413, 0x7FFF56C9E400)                = -1 Err#25
+	 7003/0x16c3126:    122381    1455     46 read(0x13, "PidLock {lockingPid = 7003, lockingHost = \"ashram\"}\0", 0x1FA0)          = 51 0
+	 7003/0x16c3126:    122392       5      2 read(0x13, "\0", 0x1FA0)               = 0 0
+	 7003/0x16c3126:    122427    1339     27 close(0x13)            = 0 0
+	 7003/0x16c3126:    122441      10      4 select(0x13, 0x7FFF56C9E340, 0x7FFF56C9E3C0, 0x0, 0x7FFF56C9E330)              = 1 0
+	 7003/0x16c3126:    122469     661     23 write(0x12, "PidLock {lockingPid = 7003, lockingHost = \"ashram\"}\0", 0x33)           = 51 0
+	 7003/0x16c3126:    122577    6028     99 close(0x12)            = 0 0
+
+Avove shows hard linking failed, and it fell back to using open to create
+the pidlock file. That seemed to finish successfully.
+
+	 7003/0x16c3126:    122583       4      0 sigreturn(0x7FFF56C9E3B0, 0x1E, 0x33)          = 0 Err#-2
+
+Unsure what this is?
+
+	 7003/0x16c3126:    122697    2126    101 unlink("/Volumes/music/ga-test/.git/annex/locktmp16807282475249\0", 0x1E, 0x33)                = 0 0
+
+Here linkToLock must has succeeded, because it deletes the tmp file that was passed to that function.
+So, it looks like tryLock succeeded.
+
+	 7003/0x16c3126:    122746    1423     28 stat64(".git/annex/pidlock\0", 0x20001A510, 0x33)              = 0 0
+	 7003/0x16c3126:    122775      10      5 select(0x2, 0x7FFF56C9E340, 0x7FFF56C9E3C0, 0x0, 0x7FFF56C9E330)               = 1 0
+	 7003/0x16c3126:    122786      10      8 write(0x1, "(transfer already in progress, or unable to take transfer lock) \0", 0x40)                 = 64 0
+
+I think this is a call to checkSaneLock, which calls getFileStatus,
+and since the pid lock was created earlier, that succeeds.
+runTransfer does call checkSaneLock, and if it fails 
+we get the "transfer already in progress, or unable to take transfer lock"
+message.
+
+So, why did checkSaneLock fail?
+
+Well, the device and inode of the lock file
+are compared with the device and inode of the temp file
+that was passed to linkToLock. However, since the hard link didn't
+happen, the device and inode will be different!
+And that's the bug.
+"""]]