From 9d603850018904153f46e6a41c122c32df78282f Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Tue, 20 Dec 2022 15:17:50 -0400 Subject: [PATCH] convert renameFile to moveFile to support cross-device moves Improve handling of some .git/annex/ subdirectories being on other filesystems, in the bittorrent special remote, and youtube-dl integration, and git-annex addurl. The only one of these that I've confirmed to be a problem is in the bittorrent special remote when .git/annex/tmp and .git/annex/othertmp are on different filesystems. As well as auditing for renameFile, also audited for createLink, all of those are ok as are the other remaining renameFile calls. Also audited all code paths that use .git/annex/othertmp, and did not find any other cross-device problems. So, removing mention of othertmp needing to be on the same device. Sponsored-by: Dartmouth College's Datalad project --- Annex/YoutubeDl.hs | 2 +- CHANGELOG | 3 +++ Command/AddUrl.hs | 8 ++------ Remote/BitTorrent.hs | 6 +++--- doc/internals.mdwn | 4 ---- ...18_1adce7945940b9c384c2383261388dd9._comment | 17 +++++++++++++++++ 6 files changed, 26 insertions(+), 14 deletions(-) create mode 100644 doc/internals/comment_18_1adce7945940b9c384c2383261388dd9._comment diff --git a/Annex/YoutubeDl.hs b/Annex/YoutubeDl.hs index c6d0f2253e..854acb9dad 100644 --- a/Annex/YoutubeDl.hs +++ b/Annex/YoutubeDl.hs @@ -144,7 +144,7 @@ youtubeDlTo key url dest p = do res <- withTmpWorkDir key $ \workdir -> youtubeDl url (fromRawFilePath workdir) p >>= \case Right (Just mediafile) -> do - liftIO $ renameFile mediafile dest + liftIO $ moveFile (toRawFilePath mediafile) (toRawFilePath dest) return (Just True) Right Nothing -> return (Just False) Left msg -> do diff --git a/CHANGELOG b/CHANGELOG index 709790fdb5..75d1b6eaa4 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -3,6 +3,9 @@ git-annex (10.20221213) UNRELEASED; urgency=medium * Change --metadata comparisons < > <= and >= to fall back to lexicographical comparisons when one or both values being compared are not numbers. + * Improve handling of some .git/annex/ subdirectories being on other + filesystems, in the bittorrent special remote, and youtube-dl + integration, and git-annex addurl. -- Joey Hess Mon, 12 Dec 2022 13:04:54 -0400 diff --git a/Command/AddUrl.hs b/Command/AddUrl.hs index 5961a18eb4..65dc43bc2c 100644 --- a/Command/AddUrl.hs +++ b/Command/AddUrl.hs @@ -467,9 +467,7 @@ addWorkTree _ addunlockedmatcher u url file key mtmp = case mtmp of -- Move to final location for large file check. pruneTmpWorkDirBefore tmp $ \_ -> do createWorkTreeDirectory (P.takeDirectory file) - liftIO $ renameFile - (fromRawFilePath tmp) - (fromRawFilePath file) + liftIO $ moveFile tmp file largematcher <- largeFilesMatcher large <- checkFileMatcher largematcher file if large @@ -477,9 +475,7 @@ addWorkTree _ addunlockedmatcher u url file key mtmp = case mtmp of -- Move back to tmp because addAnnexedFile -- needs the file in a different location -- than the work tree file. - liftIO $ renameFile - (fromRawFilePath file) - (fromRawFilePath tmp) + liftIO $ moveFile file tmp go else Command.Add.addSmall (DryRun False) file s >>= maybe noop void diff --git a/Remote/BitTorrent.hs b/Remote/BitTorrent.hs index 3ce18f584c..b67d438335 100644 --- a/Remote/BitTorrent.hs +++ b/Remote/BitTorrent.hs @@ -217,7 +217,7 @@ downloadTorrentFile u = do ok <- Url.withUrlOptions $ Url.download nullMeterUpdate Nothing u f when ok $ - liftIO $ renameFile f (fromRawFilePath torrent) + liftIO $ moveFile (toRawFilePath f) torrent return ok ) @@ -228,7 +228,7 @@ downloadMagnetLink u metadir dest = ifM download <$> dirContents metadir case ts of (t:[]) -> do - renameFile t dest + moveFile (toRawFilePath t) (toRawFilePath dest) return True _ -> return False , return False @@ -256,7 +256,7 @@ downloadTorrentContent k u dest filenum p = do showOutput ifM (download torrent downloaddir <&&> liftIO (doesFileExist dlf)) ( do - liftIO $ renameFile dlf dest + liftIO $ moveFile (toRawFilePath dlf) (toRawFilePath dest) -- The downloaddir is not removed here, -- so if aria downloaded parts of other -- files, and this is called again, it will diff --git a/doc/internals.mdwn b/doc/internals.mdwn index 6a1cf9f2d6..47fd0b55cb 100644 --- a/doc/internals.mdwn +++ b/doc/internals.mdwn @@ -28,10 +28,6 @@ This directory contains partially transferred objects. This is a temp directory for miscellaneous other temp files. -While .git/annex/objects and .git/annex/tmp can be put on different -filesystems if desired, .git/annex/othertmp -has to be on the same filesystem as the work tree and git repository. - ### `.git/annex/bad/` git-annex fsck puts any bad objects it finds in here. diff --git a/doc/internals/comment_18_1adce7945940b9c384c2383261388dd9._comment b/doc/internals/comment_18_1adce7945940b9c384c2383261388dd9._comment new file mode 100644 index 0000000000..533b703735 --- /dev/null +++ b/doc/internals/comment_18_1adce7945940b9c384c2383261388dd9._comment @@ -0,0 +1,17 @@ +[[!comment format=mdwn + username="joey" + subject="""re: why othertmp to be on the same file system?""" + date="2022-12-20T18:39:35Z" + content=""" +I've audited the code and the only place I could find where it did not work +to have othertmp on a different filesystem is in the bittorrent special +remote when it downloads a torrent file. But that also failed when +`.git/annex/tmp` was on a different filesystem! (Since it was moving between +the two directories.) I've fixed that. + +It's still best to keep things on the same filesystem because +cross-filesystem moves can be expensive and it sometimes falls back to less +ideal behavior in other ways too when operating across filesystems. Also +of course, you avoid being the one who gets to find and report breakage +like the above.. +"""]]