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

diff --git a/Annex/Ingest.hs b/Annex/Ingest.hs
index 8c09243abcd830fc44341753a03a52413d9f32e6..4b4290fded2dd61279af4e004f77e4598e63e8d3 100644 (file)
--- 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 b7651917becd8e32099c5603c0d588c46ca57355..7b06e7d76231acafb03a08c73ccce2efdef0ff63 100644 (file)
--- 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 7cb757be5ffa9a507f898d657089d8945b2b1a59..2f232da31f7a9991015cee93d2cbf9641f7a9ba4 100644 (file)
--- 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 56495825dad027e42b01f1e56961c3546f853002..0fe4351ab08153f84e508f93b676ca34f33cdc8b 100644 (file)
--- 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 b0a1b0898aa9c7f1ead6042ba73d89e0d494b846..f31cdb18fb3f07a000d5477c6db8fb2a2a89a0e8 100644 (file)
--- a/Command/Import.hs
+++ b/Command/Import.hs
@@ -1,6 +1,6 @@
 {- git-annex command
  -
- - Copyright 2012-2020 Joey Hess <id@joeyh.name>
+ - Copyright 2012-2021 Joey Hess <id@joeyh.name>
  -
  - 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 c242b7e1b5c7ac9878c11dcf0e565204479db567..eb7b8b49c0c43736d9495acbea27ab150fe89026 100644 (file)
--- 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 749e980edc5fd85c0f80221a4f89d49a2656c6d4..b4f26455911d4b99872a39c059c619bb8f133b32 100644 (file)
--- 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 (file)
index 0000000..09332c3
--- /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..
+"""]]