defer write permissions checking in import until after copy to repo

This should complete the fix started in
6329997ac4, fixing the actual cause of the
test suite failure this time.

Sponsored-by: Dartmouth College's Datalad project

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. -}

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)

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

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

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

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

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]]

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..
+"""]]