From 0101363eb8f49f34223edf221c24bd373ea011c6 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Mon, 7 Jun 2021 12:52:36 -0400 Subject: [PATCH] correctly update keys db in merge conflict This is quite a subtle edge case, see the bug report for full details. The second git diff is needed only when there's a merge conflict. It would be possible to speed it up marginally by using --diff-filter=Unmerged, but probably not enough to bother with. Sponsored-by: Graham Spencer on Patreon --- Database/Keys.hs | 71 ++++++++++++++----- doc/bugs/case_where_keys_db_lags_reality.mdwn | 39 ++++++++-- 2 files changed, 87 insertions(+), 23 deletions(-) diff --git a/Database/Keys.hs b/Database/Keys.hs index 3154118fdb..633b654c1c 100644 --- a/Database/Keys.hs +++ b/Database/Keys.hs @@ -245,7 +245,8 @@ reconcileStaged qh = do go cur indexcache (Just newtree) = do oldtree <- getoldtree when (oldtree /= newtree) $ do - updatetodiff (fromRef oldtree) (fromRef newtree) + updatetodiff (Just (fromRef oldtree)) (fromRef newtree) procdiff + >>= flushdb . fst liftIO $ writeFile indexcache $ showInodeCache cur -- Storing the tree in a ref makes sure it does not -- get garbage collected, and is available to diff @@ -253,22 +254,34 @@ reconcileStaged qh = do inRepo $ update' lastindexref newtree -- git write-tree will fail if the index is locked or when there is -- a merge conflict. To get up-to-date with the current index, - -- diff --cached with the old index tree. The current index tree + -- diff --staged with the old index tree. The current index tree -- is not known, so not recorded, and the inode cache is not updated, -- so the next time git-annex runs, it will diff again, even -- if the index is unchanged. + -- + -- When there is a merge conflict, that will not see the new local + -- version of the files that are conflicted. So a second diff + -- is done, with --staged but no old tree. go _ _ Nothing = do oldtree <- getoldtree - updatetodiff (fromRef oldtree) "--cached" + (changed, conflicted) <- updatetodiff + (Just (fromRef oldtree)) "--staged" procdiff + changed' <- if conflicted + then fst <$> updatetodiff Nothing "--staged" + procmergeconflictdiff + else pure False + flushdb (changed || changed') - updatetodiff old new = do + updatetodiff old new processor = do (l, cleanup) <- inRepo $ pipeNullSplit' $ diff old new - changed <- procdiff l False - void $ liftIO cleanup - -- Flush database changes immediately - -- so other processes can see them. - when changed $ - liftIO $ H.flushDbQueue qh + processor l False False + `finally` void (liftIO cleanup) + + -- Flush database changes immediately + -- so other processes can see them. + flushdb changed + | changed = liftIO $ H.flushDbQueue qh + | otherwise = noop -- Avoid running smudge clean filter, which would block trying to -- access the locked database. git write-tree sometimes calls it, @@ -288,8 +301,8 @@ reconcileStaged qh = do -- (The -G option may make it be used otherwise.) [ Param "-c", Param "diff.external=" , Param "diff" - , Param old - , Param new + ] ++ maybeToList (Param <$> old) ++ + [ Param new , Param "--raw" , Param "-z" , Param "--no-abbrev" @@ -307,12 +320,13 @@ reconcileStaged qh = do , Param "--no-ext-diff" ] - procdiff (info:file:rest) changed + procdiff (info:file:rest) changed conflicted | ":" `S.isPrefixOf` info = case S8.words info of (_colonsrcmode:dstmode:srcsha:dstsha:status:[]) -> do + let conflicted' = status == "U" -- avoid removing associated file when -- there is a merge conflict - removed <- if status /= "U" + removed <- if not conflicted' then catKey (Ref srcsha) >>= \case Just oldkey -> do liftIO $ SQL.removeAssociatedFile oldkey @@ -330,9 +344,32 @@ reconcileStaged qh = do reconcilerace (asTopFilePath file) key return True Nothing -> return False - procdiff rest (changed || removed || added) - _ -> return changed -- parse failed - procdiff _ changed = return changed + procdiff rest + (changed || removed || added) + (conflicted || conflicted') + _ -> return (changed, conflicted) -- parse failed + procdiff _ changed conflicted = return (changed, conflicted) + + -- Processing a diff --index when there is a merge conflict. + -- This diff will have the new local version of a file as the + -- first sha, and a null sha as the second sha, and we only + -- care about files that are in conflict. + procmergeconflictdiff (info:file:rest) changed conflicted + | ":" `S.isPrefixOf` info = case S8.words info of + (_colonmode:_mode:sha:_sha:status:[]) -> do + let conflicted' = status == "U" + added <- catKey (Ref sha) >>= \case + Just key -> do + liftIO $ SQL.addAssociatedFile key + (asTopFilePath file) + (SQL.WriteHandle qh) + return True + Nothing -> return False + procmergeconflictdiff rest + (changed || added) + (conflicted || conflicted') + _ -> return (changed, conflicted) -- parse failed + procmergeconflictdiff _ changed conflicted = return (changed, conflicted) reconcilerace file key = do caches <- liftIO $ SQL.getInodeCaches key (SQL.ReadHandle qh) diff --git a/doc/bugs/case_where_keys_db_lags_reality.mdwn b/doc/bugs/case_where_keys_db_lags_reality.mdwn index 17a799d2f4..87ea7fa675 100644 --- a/doc/bugs/case_where_keys_db_lags_reality.mdwn +++ b/doc/bugs/case_where_keys_db_lags_reality.mdwn @@ -1,10 +1,10 @@ Found a case where the associated files in the keys db end up out-of-date. -Make a repo with an unlocked file, clone it to a second repo, and set up a +Make a repo with an locked file, clone it to a second repo, and set up a conflict involving that file in both repos, using git-annex add to add the -conflicting version, and not running other git-annex commands after that, -before pulling the conflicting branch. When the associated files db -gets updated in the conflict situation, only 1 key has the conflicting file -associated with it, rather than 2 or 3. +conflicting version, committing, and not running other git-annex commands +after that, before pulling the conflicting branch. When the associated +files db gets updated in the conflict situation, only 1 key has the +conflicting file associated with it, rather than 2 or 3. The original key before the conflict has the file associated with it, but the new local key and new remote key do not. @@ -21,8 +21,35 @@ git-annex updates the keys db. So, one solution to this bug will be for git-annex to also update the keys db when staging locked files. (Unfortunately this would make mass adds somewhat slower.) -Or, possibly, for reconcileStaged to not use git diff --index in this case, +Or, possibly, for reconcileStaged to not use git diff --cached in this case, but git diff with -1 and -3. That lets both sides of the merge conflict be accessed, and it could then add the file to both keys. As well as not slowing down git-annex add, this would let it honor the preferred content of the conflicting file for all 3 keys. --[[Joey]] + +> On second thought, it's not really necessary that all 3 keys have the +> conflicted file associated with them. The original key doesn't because +> the user has already changed the file to use the new key. The new remote +> key does not really need to, and there might not even be any effect if it +> did. The new local key is the one that this bug is really about. +> +> Consider that checkDrop uses catKeyFile to double-check the associated +> files. And that will see the file pointing to the new local key. So +> if the original key or new remote key are also associated with the file, +> it will ignore them and drop anyway. And that's ok, from the user's +> perspective the one it needs to retain is the one that the file in the +> working tree uses, which is the new local key. +> +> > Hmm, -1 and -3 are not what's needed to get the new local key. +> > It's using `git diff oldtree --cached`, and the code preserves the old +> > key when it sees a merge conflict. Using instead +> > `git diff HEAD --cached` has the new key as the src sha, and nullsha as +> > the dst sha. +> > +> > However, the diff with the old tree is needed to incrementally +> > update when it's not in the middle of a merge conflict. +> > So what can be done is do the diff as now; when it sees a merge +> > conflict, run diff a second time with `HEAD --cached` to get the new +> > key. +> > +> > > [[done]] --[[Joey]] -- 2.30.2