smudge: check for known annexed inodes before checking annex.largefiles

smudge: Fix a case where an unlocked annexed file that annex.largefiles
does not match could get its unchanged content checked into git, due to git
running the smudge filter unecessarily.

When the file has the same inodecache as an already annexed file,
we can assume that the user is not intending to change how it's stored in
git.

Note that checkunchangedgitfile already handled the inverse case, where the
file was added to git previously. That goes further and actually sha1
hashes the new file and checks if it's the same hash in the index.

It would be possible to generate a key for the file and see if it's the
same as the old key, however that could be considerably more expensive than
sha1 of a small file is, and it is not necessary for the case I have, at
least, where the file is not modified or touched, and so its inode will
match the cache.

git-annex add was changed, when adding a small file, to remove the inode
cache for it. This is necessary to keep the recipe in
doc/tips/largefiles.mdwn for converting from annex to git working.
It also avoids bugs/case_where_using_pathspec_with_git-commit_leaves_s.mdwn
which the earlier try at this change introduced.

diff --git a/CHANGELOG b/CHANGELOG
index b5d2290e4342a151f64ed274244befecace37905..9173616e1259c17d9c4d516b20beff0040cac551 100644 (file)
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -5,6 +5,9 @@ git-annex (8.20210429) UNRELEASED; urgency=medium
   * Fix behavior of several commands, including reinject, addurl, and rmurl
     when given an absolute path to an unlocked file, or a relative path
     that leaves and re-enters the repository.
+  * smudge: Fix a case where an unlocked annexed file that annex.largefiles
+    does not match could get its unchanged content checked into git,
+    due to git running the smudge filter unecessarily.
   * reinject: Error out when run on a file that is not annexed, rather
     than silently skipping it.
 

diff --git a/Command/Add.hs b/Command/Add.hs
index 246a2242f77f1054928a2db853274f7d7b9eaaf1..3422e065fb3d6d20063ef18020c808b77562818e 100644 (file)
--- a/Command/Add.hs
+++ b/Command/Add.hs
@@ -22,6 +22,8 @@ import Git.FilePath
 import Config.GitConfig
 import Config.Smudge
 import Utility.OptParse
+import Utility.InodeCache
+import Annex.InodeSentinal
 import qualified Utility.RawFilePath as R
 
 cmd :: Command
@@ -129,13 +131,24 @@ data SmallOrLarge = Small | Large
 addFile :: SmallOrLarge -> CheckGitIgnore -> RawFilePath -> Annex Bool
 addFile smallorlarge ci file = do
        ps <- gitAddParams ci
+       cps <- case smallorlarge of
+               -- In case the file is being converted from an annexed file
+               -- to be stored in git, remove the cached inode, so that
+               -- if the smudge clean filter later runs on the file,
+               -- it will not remember it was annexed.
+               --
+               -- The use of bypassSmudgeConfig prevents the smudge
+               -- filter from being run. So the changes to the database
+               -- can be queued up and not flushed to disk immediately.
+               Small -> do
+                       withTSDelta (liftIO . genInodeCache file) >>= \case
+                               Just ic -> Database.Keys.removeInodeCache ic
+                               Nothing -> return ()
+                       return bypassSmudgeConfig
+               Large -> return []
        Annex.Queue.addCommand cps "add" (ps++[Param "--"])
                [fromRawFilePath file]
        return True
-  where
-       cps = case smallorlarge of
-               Large -> []
-               Small -> bypassSmudgeConfig
 
 start :: AddOptions -> SeekInput -> RawFilePath -> AddUnlockedMatcher -> CommandStart
 start o si file addunlockedmatcher = do

diff --git a/Command/Smudge.hs b/Command/Smudge.hs
index 4903ce4a4f6469530f352962d7d9267ef7776772..cbecd055f695a0bbbadaedb66531ed6bad993fa5 100644 (file)
--- a/Command/Smudge.hs
+++ b/Command/Smudge.hs
@@ -168,25 +168,26 @@ clean file = do
                filepath <- liftIO $ absPath file
                return $ not $ dirContains repopath filepath
 
--- If annex.largefiles is configured, matching files are added to the
--- annex. But annex.gitaddtoannex can be set to false to disable that.
+-- If annex.largefiles is configured (and not disabled by annex.gitaddtoannex
+-- being set to false), matching files are added to the annex and the rest to
+-- git.
 --
 -- When annex.largefiles is not configured, files are normally not
--- added to the annex, so will be added to git. But some heuristics
--- are used to avoid bad behavior:
+-- added to the annex, so will be added to git. However, if the file
+-- is annexed in the index, keep it annexed. This prevents accidental
+-- conversions when previously annexed files get modified and added.
 --
--- If the file is annexed in the index, keep it annexed.
--- This prevents accidental conversions.
---
--- Otherwise, when the file's inode is the same as one that was used for
--- annexed content before, annex it. This handles cases such as renaming an
--- unlocked annexed file followed by git add, which the user naturally
--- expects to behave the same as git mv.
+-- In either case, if the file's inode is the same as one that was used
+-- for annexed content before, annex it. And if the file is not annexed
+-- in the index, and has the same content, leave it in git.
+-- This handles cases such as renaming a file followed by git add,
+-- which the user naturally expects to behave the same as git mv.
 shouldAnnex :: RawFilePath -> Maybe (Sha, FileSize, ObjectType) -> Maybe Key -> Annex Bool
-shouldAnnex file indexmeta moldkey = ifM (annexGitAddToAnnex <$> Annex.getGitConfig)
-       ( checkunchangedgitfile $ checkmatcher checkheuristics
-       , checkunchangedgitfile checkheuristics
-       )
+shouldAnnex file indexmeta moldkey = do
+       ifM (annexGitAddToAnnex <$> Annex.getGitConfig)
+               ( checkunchanged $ checkmatcher checkwasannexed
+               , checkunchanged checkwasannexed
+               )
   where
        checkmatcher d
                | dotfile file = ifM (getGitConfigVal annexDotFiles)
@@ -199,14 +200,21 @@ shouldAnnex file indexmeta moldkey = ifM (annexGitAddToAnnex <$> Annex.getGitCon
                        matcher <- largeFilesMatcher
                        checkFileMatcher' matcher file d
        
-       checkheuristics = case moldkey of
-               Just _ -> return True
-               Nothing -> checkknowninode
+       checkwasannexed = pure $ isJust moldkey
 
-       checkknowninode = withTSDelta (liftIO . genInodeCache file) >>= \case
+       isknownannexedinode = withTSDelta (liftIO . genInodeCache file) >>= \case
                Nothing -> pure False
                Just ic -> Database.Keys.isInodeKnown ic =<< sentinalStatus
 
+       -- If the inode matches one known used for annexed content,
+       -- keep the file annexed. This handles a case where the file
+       -- has been annexed before, and the git is running the clean filter
+       -- again on it for whatever reason.
+       checkunchanged cont = ifM isknownannexedinode
+               ( return True
+               , checkunchangedgitfile cont
+               )
+
        -- This checks for a case where the file had been added to git
        -- previously, not to the annex before, and its content is not
        -- changed, but git is running the clean filter again on it

diff --git a/Database/Keys.hs b/Database/Keys.hs
index ebaee739a599973f3deaa27bb4b6801ae607ab9e..c05000261770545d37c83fa60ee81263263b987b 100644 (file)
--- a/Database/Keys.hs
+++ b/Database/Keys.hs
@@ -20,6 +20,7 @@ module Database.Keys (
        addInodeCaches,
        getInodeCaches,
        removeInodeCaches,
+       removeInodeCache,
        isInodeKnown,
        runWriter,
 ) where
@@ -179,9 +180,14 @@ addInodeCaches k is = runWriterIO $ SQL.addInodeCaches k is
 getInodeCaches :: Key -> Annex [InodeCache]
 getInodeCaches = runReaderIO . SQL.getInodeCaches
 
+{- Remove all inodes cached for a key. -}
 removeInodeCaches :: Key -> Annex ()
 removeInodeCaches = runWriterIO . SQL.removeInodeCaches
 
+{- Remove cached inodes, for any key. -}
+removeInodeCache :: InodeCache -> Annex ()
+removeInodeCache = runWriterIO . SQL.removeInodeCache
+
 isInodeKnown :: InodeCache -> SentinalStatus -> Annex Bool
 isInodeKnown i s = or <$> runReaderIO ((:[]) <$$> SQL.isInodeKnown i s)
 

diff --git a/Database/Keys/SQL.hs b/Database/Keys/SQL.hs
index 26108b19650c53ecef1433ded42a5f2fe4b36604..7d191bfb4c1d35601cd5c94682b9f5b04fc325ff 100644 (file)
--- a/Database/Keys/SQL.hs
+++ b/Database/Keys/SQL.hs
@@ -144,6 +144,11 @@ removeInodeCaches :: Key -> WriteHandle -> IO ()
 removeInodeCaches k = queueDb $
        deleteWhere [ContentKey ==. k]
 
+removeInodeCache :: InodeCache -> WriteHandle -> IO ()
+removeInodeCache i = queueDb $ deleteWhere
+       [ ContentInodecache ==. i
+       ]
+
 {- Check if the inode is known to be used for an annexed file. -}
 isInodeKnown :: InodeCache -> SentinalStatus -> ReadHandle -> IO Bool
 isInodeKnown i s = readDb (isJust <$> selectFirst q [])