Support core.sharedRepository=0xxx at long last

Sponsored-by: Brett Eisenberg on Patreon

Annex/Perms.hs

@@ -44,8 +44,7 @@ import Config
 import Utility.Directory.Create
 import qualified Utility.RawFilePath as R
 
-import System.PosixCompat.Files (fileMode, intersectFileModes, nullFileMode, groupWriteMode, ownerWriteMode, ownerReadMode, groupReadMode, stdFileMode, ownerExecuteMode, groupExecuteMode)
+import System.PosixCompat.Files (fileMode, intersectFileModes, nullFileMode, groupWriteMode, ownerWriteMode, ownerReadMode, groupReadMode, otherReadMode, stdFileMode, ownerExecuteMode, groupExecuteMode, otherExecuteMode, setGroupIDMode)
-import Numeric
 
 withShared :: (SharedRepository -> Annex a) -> Annex a
 withShared a = a =<< coreSharedRepository <$> Annex.getGitConfig
@@ -78,15 +77,10 @@ setAnnexPerm' modef isdir file = unlessM crippledFileSystem $
 		Nothing -> noop
 		Just f -> void $ liftIO $ tryIO $
 			modifyFileMode file $ f []
-	go (UmaskShared n) = do
+	go (UmaskShared n) = void $ liftIO $ tryIO $ R.setFileMode file $
-		warnUmaskSharedUnsupported n
+		if isdir then umaskSharedDirectory n else n
-		go UnShared
 	modef' = fromMaybe addModes modef
 
-warnUmaskSharedUnsupported :: Int -> Annex ()
-warnUmaskSharedUnsupported n = warning $ UnquotedString $
-	"core.sharedRepository set to a umask override (0" ++ showOct n "" ++ ") is not supported by git-annex; ignoring that configuration"
-
 resetAnnexFilePerm :: RawFilePath -> Annex ()
 resetAnnexFilePerm = resetAnnexPerm False
 
@@ -109,14 +103,12 @@ resetAnnexPerm isdir file = unlessM crippledFileSystem $ do
  - taken into account; this is for use with actions that create the file
  - and apply the umask automatically. -}
 annexFileMode :: Annex FileMode
-annexFileMode = withShared go
+annexFileMode = withShared (pure . go)
   where
-	go GroupShared = return sharedmode
+	go GroupShared = sharedmode
-	go AllShared = return $ combineModes (sharedmode:readModes)
+	go AllShared = combineModes (sharedmode:readModes)
-	go UnShared = return stdFileMode
+	go UnShared = stdFileMode
-	go (UmaskShared n) = do
+	go (UmaskShared n) = n
-		warnUmaskSharedUnsupported n
-		go UnShared
 	sharedmode = combineModes groupSharedModes
 
 {- Creates a directory inside the gitAnnexDir (or possibly the dbdir), 
@@ -179,6 +171,7 @@ freezeContent'' sr file rv = do
 	unlessM crippledFileSystem $ go sr
 	freezeHook file
   where
+	go UnShared = liftIO $ nowriteadd [ownerReadMode]
 	go GroupShared = if versionNeedsWritableContentFiles rv
 		then liftIO $ ignoresharederr $ modmode $ addModes
 			[ownerReadMode, groupReadMode, ownerWriteMode, groupWriteMode]
@@ -189,10 +182,13 @@ freezeContent'' sr file rv = do
 			(readModes ++ writeModes)
 		else liftIO $ ignoresharederr $ 
 			nowriteadd readModes
-	go UnShared = liftIO $ nowriteadd [ownerReadMode]
+	go (UmaskShared n) = if versionNeedsWritableContentFiles rv
-	go (UmaskShared n) = do
+		-- Assume that the configured mode includes write bits
-		warnUmaskSharedUnsupported n
+		-- for all users who should be able to lock the file, so
-		go UnShared
+		-- don't need to add any write modes.
+		then liftIO $ ignoresharederr $ modmode $ const n
+		else liftIO $ ignoresharederr $ modmode $ const $
+			removeModes writeModes n
 
 	ignoresharederr = void . tryIO
 
@@ -228,6 +224,7 @@ checkContentWritePerm' :: SharedRepository -> RawFilePath -> Maybe RepoVersion -
 checkContentWritePerm' sr file rv hasfreezehook
 	| hasfreezehook = return (Just True)
 	| otherwise = case sr of
+		UnShared -> want Just (excludemodes writeModes)
 		GroupShared
 			| versionNeedsWritableContentFiles rv -> want sharedret
 				(includemodes [ownerWriteMode, groupWriteMode])
@@ -236,9 +233,11 @@ checkContentWritePerm' sr file rv hasfreezehook
 			| versionNeedsWritableContentFiles rv -> 
 				want sharedret (includemodes writeModes)
 			| otherwise -> want sharedret (excludemodes writeModes)
-		UnShared -> want Just (excludemodes writeModes)
+		UmaskShared n
-		UmaskShared _ ->
+			| versionNeedsWritableContentFiles rv -> want sharedret
-			checkContentWritePerm' UnShared file rv hasfreezehook
+				(\havemode -> havemode == n)
+			| otherwise -> want sharedret
+				(\havemode -> havemode == removeModes writeModes n)
   where
 	want mk f = catchMaybeIO (fileMode <$> R.getFileStatus file)
 		>>= return . \case
@@ -264,9 +263,7 @@ thawContent' sr file = do
 	go GroupShared = liftIO $ void $ tryIO $ groupWriteRead file
 	go AllShared = liftIO $ void $ tryIO $ groupWriteRead file
 	go UnShared = liftIO $ allowWrite file
-	go (UmaskShared n) = do
+	go (UmaskShared n) = liftIO $ void $ tryIO $ R.setFileMode file n
-		warnUmaskSharedUnsupported n
-		go UnShared
 
 {- Runs an action that thaws a file's permissions. This will probably
  - fail on a crippled filesystem. But, if file modes are supported on a
@@ -281,7 +278,7 @@ thawPerms a hook = ifM crippledFileSystem
 {- Blocks writing to the directory an annexed file is in, to prevent the
  - file accidentally being deleted. However, if core.sharedRepository
  - is set, this is not done, since the group must be allowed to delete the
- - file.
+ - file without eing able to thaw the directory.
  -}
 freezeContentDir :: RawFilePath -> Annex ()
 freezeContentDir file = do
@@ -290,19 +287,26 @@ freezeContentDir file = do
 	freezeHook dir
   where
 	dir = parentDir file
+	go UnShared = liftIO $ preventWrite dir
 	go GroupShared = liftIO $ void $ tryIO $ groupWriteRead dir
 	go AllShared = liftIO $ void $ tryIO $ groupWriteRead dir
-	go UnShared = liftIO $ preventWrite dir
+	go (UmaskShared n) = liftIO $ void $ tryIO $ R.setFileMode dir $
-	go (UmaskShared n) = do
+		umaskSharedDirectory $ 
-		warnUmaskSharedUnsupported n
+			-- If n includes group or other write mode, leave them set
-		go UnShared
+			-- to allow them to delete the file without being able to
+			-- thaw the directory.
+			removeModes [ownerWriteMode] n
 
 thawContentDir :: RawFilePath -> Annex ()
 thawContentDir file = do
 	fastDebug "Annex.Perms" ("thawing content directory " ++ fromRawFilePath dir)
-	thawPerms (liftIO $ allowWrite dir) (thawHook dir)
+	thawPerms (withShared (liftIO . go)) (thawHook dir)
   where
 	dir = parentDir file
+	go UnShared = allowWrite dir
+	go GroupShared = allowWrite dir
+	go AllShared = allowWrite dir
+	go (UmaskShared n) = R.setFileMode dir n
 
 {- Makes the directory tree to store an annexed file's content,
  - with appropriate permissions on each level. -}
@@ -354,3 +358,17 @@ thawHook p = maybe noop go =<< annexThawContentCommand <$> Annex.getGitConfig
 	go basecmd = void $ liftIO $
 		boolSystem "sh" [Param "-c", Param $ gencmd basecmd]
 	gencmd = massReplace [ ("%path", shellEscape (fromRawFilePath p)) ]
+
+{- Calculate mode to use for a directory from the mode to use for a file.
+ -
+ - This corresponds to git's handling of core.sharedRepository=0xxx
+ -}
+umaskSharedDirectory :: FileMode -> FileMode
+umaskSharedDirectory n = flip addModes n $ map snd $ filter fst
+	[ (isset ownerReadMode, ownerExecuteMode)
+	, (isset groupReadMode, groupExecuteMode)
+	, (isset otherReadMode, otherExecuteMode)
+	, (isset groupReadMode || isset groupWriteMode, setGroupIDMode)
+	]
+  where
+	isset v = checkMode v n

CHANGELOG

@@ -29,7 +29,7 @@ git-annex (10.20230408) UNRELEASED; urgency=medium
     --json-error-messages is enabled, output a JSON object indicating the
     problem. (But git ls-files --error-unmatch still displays errors about
     such files in some situations.)
-  * Warn about unsupported core.sharedRepository=0xxx when set.
+  * Support core.sharedRepository=0xxx at long last.
   * Bug fix: Create .git/annex/, .git/annex/fsckdb,
     .git/annex/sentinal, .git/annex/sentinal.cache, and
     .git/annex/journal/* with permissions configured by core.sharedRepository.

Git/ConfigTypes.hs

@@ -12,13 +12,14 @@ module Git.ConfigTypes where
 import Data.Char
 import Numeric
 import qualified Data.ByteString.Char8 as S8
+import System.PosixCompat.Types
 
 import Common
 import Git
 import Git.Types
 import qualified Git.Config
 
-data SharedRepository = UnShared | GroupShared | AllShared | UmaskShared Int
+data SharedRepository = UnShared | GroupShared | AllShared | UmaskShared FileMode
 	deriving (Eq)
 
 getSharedRepository :: Repo -> SharedRepository

doc/bugs/wrong_modes_for_some_lock_files_withcoresharedrepository.mdwn

@@ -0,0 +1,22 @@
+With core.sharedRepository=0666, some lock files get created mode 644
+(with umask 0022).
+
+With core.sharedRepository=group, some lock files get created mode 660,
+rather than 644 (with umask 0022).
+
+Root of the problem is uses of annexFileMode. 
+
+Some callers use noUmask with it, which works in cases other than these (at
+least with umask 0022). But in the core.sharedRepository=group case, the
+umask is cleared by noUmask, which is why the g+r bit is not set.
+
+Some callers don't use noUmask with it, and when
+core.sharedRepository=0666, that results in the umask being applied
+to the mode. Which it's not supposed to with an octal mode configured.
+
+The affected files are all lock files.
+
+Fix will probably involve getting rid of annexFileMode, and noUmask, and
+creating the lock file with default umask, then fixing up the mode if necessary,
+before using it. Ie, the same pattern used everywhere else in git-annex.
+--[[Joey]]

doc/todo/sharedRepository_mode_not_supported_by_git-annex.mdwn

@@ -5,3 +5,5 @@ etc is not supported.
 There's no insormountable reason why not, Joey just hates umask mode math
 stuff and nobody has sent a patch. Note that Annex.Content.freezeContent
 should remove the write bit from files, no matter what.
+
+> [[fixed|done]] --[[Joey]]

doc/todo/sharedRepository_mode_not_supported_by_git-annex/comment_3_c3dff87829e3f055881829eb4598f903._comment

@@ -0,0 +1,10 @@
+[[!comment format=mdwn
+ username="joey"
+ subject="""comment 3"""
+ date="2023-04-26T17:39:21Z"
+ content="""
+I've implemented this today!
+
+(Did notice a bug,
+[[wrong_modes_for_some_lock_files_withcoresharedrepository]])
+"""]]