From ec12537774a98afe63d21bc61270375f39db0fb7 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Thu, 2 Sep 2021 13:45:21 -0400 Subject: [PATCH] defer write permissions checking in import until after copy to repo This should complete the fix started in 6329997ac44691937f1d7fe6a71da3184237b13b, fixing the actual cause of the test suite failure this time. Sponsored-by: Dartmouth College's Datalad project --- Annex/Ingest.hs | 25 +++++++----- Assistant/Threads/Committer.hs | 2 + CHANGELOG | 5 ++- Command/Add.hs | 1 + Command/Import.hs | 40 +++++++++++++++---- Command/Smudge.hs | 1 + ...d_FS__58___Unable_to_remove_all_write.mdwn | 2 + ..._46acb87752c0f0574d8f3b91fdfb1697._comment | 7 ++++ 8 files changed, 64 insertions(+), 19 deletions(-) create mode 100644 doc/bugs/2_mac_crippled_FS__58___Unable_to_remove_all_write/comment_5_46acb87752c0f0574d8f3b91fdfb1697._comment diff --git a/Annex/Ingest.hs b/Annex/Ingest.hs index 8c09243abc..4b4290fded 100644 --- a/Annex/Ingest.hs +++ b/Annex/Ingest.hs @@ -9,6 +9,7 @@ module Annex.Ingest ( LockedDown(..), LockDownConfig(..), lockDown, + checkLockedDownWritePerms, ingestAdd, ingestAdd', ingest, @@ -61,7 +62,9 @@ data LockDownConfig = LockDownConfig { lockingFile :: Bool -- ^ write bit removed during lock down , hardlinkFileTmpDir :: Maybe RawFilePath - -- ^ hard link to temp directory + -- ^ hard link to temp directorya + , checkWritePerms :: Bool + -- ^ check that write perms are successfully removed } deriving (Show) @@ -79,7 +82,7 @@ data LockDownConfig = LockDownConfig - Lockdown can fail if a file gets deleted, or if it's unable to remove - write permissions, and Nothing will be returned. -} -lockDown :: LockDownConfig -> FilePath -> Annex (Maybe LockedDown) +lockDown :: LockDownConfig-> FilePath -> Annex (Maybe LockedDown) lockDown cfg file = either (\e -> warning (show e) >> return Nothing) (return . Just) @@ -128,13 +131,17 @@ lockDown' cfg file = tryNonAsync $ ifM crippledFileSystem setperms = when (lockingFile cfg) $ do freezeContent file' - checkContentWritePerm file' >>= \case - Just False -> giveup $ unwords - [ "Unable to remove all write permissions from" - , file - , "-- perhaps it has an xattr or ACL set." - ] - _ -> return () + when (checkWritePerms cfg) $ + maybe noop giveup =<< checkLockedDownWritePerms file' file' + +checkLockedDownWritePerms :: RawFilePath -> RawFilePath -> Annex (Maybe String) +checkLockedDownWritePerms file displayfile = checkContentWritePerm file >>= return . \case + Just False -> Just $ unwords + [ "Unable to remove all write permissions from" + , fromRawFilePath displayfile + , "-- perhaps it has an xattr or ACL set." + ] + _ -> Nothing {- Ingests a locked down file into the annex. Updates the work tree and - index. -} diff --git a/Assistant/Threads/Committer.hs b/Assistant/Threads/Committer.hs index b7651917be..7b06e7d762 100644 --- a/Assistant/Threads/Committer.hs +++ b/Assistant/Threads/Committer.hs @@ -265,6 +265,7 @@ handleAdds lockdowndir havelsof largefilematcher delayadd cs = returnWhen (null let lockdownconfig = LockDownConfig { lockingFile = False , hardlinkFileTmpDir = Just (toRawFilePath lockdowndir) + , checkWritePerms = True } (postponed, toadd) <- partitionEithers <$> safeToAdd lockdowndir lockdownconfig havelsof delayadd pending inprocess @@ -310,6 +311,7 @@ handleAdds lockdowndir havelsof largefilematcher delayadd cs = returnWhen (null let cfg = LockDownConfig { lockingFile = False , hardlinkFileTmpDir = Just (toRawFilePath lockdowndir) + , checkWritePerms = True } if M.null m then forM toadd (addannexed' cfg) diff --git a/CHANGELOG b/CHANGELOG index 7cb757be5f..2f232da31f 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -22,8 +22,9 @@ git-annex (8.20210804) UNRELEASED; urgency=medium * Run cp -a with --no-preserve=xattr, to avoid problems with copied xattrs, including them breaking permissions setting on some NFS servers. - * add: Detect when xattrs or perhaps ACLs prevent locking down - a file's content, and fail with an informative message. + * add, import: Detect when xattrs or perhaps ACLs prevent removing + write permissions from an annexed file, and fail with an informative + message. * Fix support for readonly git remotes. (Reversion in version 8.20210621) * When downloading urls fail, explain which urls failed for which diff --git a/Command/Add.hs b/Command/Add.hs index 56495825da..0fe4351ab0 100644 --- a/Command/Add.hs +++ b/Command/Add.hs @@ -188,6 +188,7 @@ perform o file addunlockedmatcher = withOtherTmp $ \tmpdir -> do let cfg = LockDownConfig { lockingFile = lockingfile , hardlinkFileTmpDir = Just tmpdir + , checkWritePerms = True } ld <- lockDown cfg (fromRawFilePath file) let sizer = keySource <$> ld diff --git a/Command/Import.hs b/Command/Import.hs index b0a1b0898a..f31cdb18fb 100644 --- a/Command/Import.hs +++ b/Command/Import.hs @@ -1,6 +1,6 @@ {- git-annex command - - - Copyright 2012-2020 Joey Hess + - Copyright 2012-2021 Joey Hess - - Licensed under the GNU AGPL version 3 or higher. -} @@ -200,13 +200,31 @@ startLocal o addunlockedmatcher largematcher mode (srcfile, destfile) = -- Move or copy the src file to the dest file. -- The dest file is what will be ingested. createWorkTreeDirectory (parentDir destfile) - liftIO $ if mode == Duplicate || mode == SkipDuplicates - then void $ copyFileExternal CopyAllMetaData - (fromRawFilePath srcfile) - (fromRawFilePath destfile) - else moveFile - (fromRawFilePath srcfile) - (fromRawFilePath destfile) + unwind <- liftIO $ if mode == Duplicate || mode == SkipDuplicates + then do + void $ copyFileExternal CopyAllMetaData + (fromRawFilePath srcfile) + (fromRawFilePath destfile) + return $ removeWhenExistsWith R.removeLink destfile + else do + moveFile + (fromRawFilePath srcfile) + (fromRawFilePath destfile) + return $ moveFile + (fromRawFilePath destfile) + (fromRawFilePath srcfile) + -- Make sure that the dest file has its write permissions + -- removed; the src file normally already did, but may + -- have imported it from a filesystem that does not allow + -- removing write permissions, to a repo on a filesystem + -- that does. + when (lockingFile (lockDownConfig ld)) $ do + freezeContent destfile + checkLockedDownWritePerms destfile srcfile >>= \case + Just err -> do + liftIO unwind + giveup err + Nothing -> noop -- Get the inode cache of the dest file. It should be -- weakly the same as the originally locked down file's -- inode cache. (Since the file may have been copied, @@ -249,6 +267,12 @@ startLocal o addunlockedmatcher largematcher mode (srcfile, destfile) = let cfg = LockDownConfig { lockingFile = lockingfile , hardlinkFileTmpDir = Nothing + -- The write perms of the file may not be able to be + -- removed, if it's being imported from a crippled + -- filesystem. So lockDown is asked to not check + -- the write perms. They will be checked later, after + -- the file gets copied into the repository. + , checkWritePerms = False } v <- lockDown cfg (fromRawFilePath srcfile) case v of diff --git a/Command/Smudge.hs b/Command/Smudge.hs index c242b7e1b5..eb7b8b49c0 100644 --- a/Command/Smudge.hs +++ b/Command/Smudge.hs @@ -160,6 +160,7 @@ clean file = do cfg = LockDownConfig { lockingFile = False , hardlinkFileTmpDir = Nothing + , checkWritePerms = True } -- git diff can run the clean filter on files outside the diff --git a/doc/bugs/2_mac_crippled_FS__58___Unable_to_remove_all_write.mdwn b/doc/bugs/2_mac_crippled_FS__58___Unable_to_remove_all_write.mdwn index 749e980edc..b4f2645591 100644 --- a/doc/bugs/2_mac_crippled_FS__58___Unable_to_remove_all_write.mdwn +++ b/doc/bugs/2_mac_crippled_FS__58___Unable_to_remove_all_write.mdwn @@ -38,3 +38,5 @@ Both look like [[!meta author=yoh]] [[!tag projects/datalad]] + +> [[fixed|done]] (provisionally, waiting on test run) --[[Joey]] diff --git a/doc/bugs/2_mac_crippled_FS__58___Unable_to_remove_all_write/comment_5_46acb87752c0f0574d8f3b91fdfb1697._comment b/doc/bugs/2_mac_crippled_FS__58___Unable_to_remove_all_write/comment_5_46acb87752c0f0574d8f3b91fdfb1697._comment new file mode 100644 index 0000000000..09332c3e0d --- /dev/null +++ b/doc/bugs/2_mac_crippled_FS__58___Unable_to_remove_all_write/comment_5_46acb87752c0f0574d8f3b91fdfb1697._comment @@ -0,0 +1,7 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 5""" + date="2021-09-02T17:39:20Z" + content=""" +Ok, fixed some more, hopefully all the way this time.. +"""]]