support git 2.34.0's handling of merge conflict between annexed and non-annexed file

This version of git -- or its new default "ort" resolver -- handles such
a conflict by staging two files, one with the original name and the other
named file~ref. Use unmergedSiblingFile when the latter is detected.

(It doesn't do that when the conflict is between a directory and a file
or symlink though, so see previous commit for how that case is handled.)

The sibling file has to be deleted separately, because cleanConflictCruft
may not delete it -- that only handles files that are annex links,
but the sibling file may be the non-annexed file side of the conflict.

The graftin code had assumed that, when the other side of a conclict
is a symlink, the file in the work tree will contain the non-annexed
content that we want it to contain. But that is not the case with the new
git; the file may be the annex link and needs to be replaced with the
content, while the annex link will be written as a -variant file.

(The weird doesDirectoryExist check in graftin turns out to still be
needed, test suite failed when I tried to remove it.)

Test suite passes with new git with ort resolver default. Have not tried it
with old git or other defaults.

Sponsored-by: Noam Kremen on Patreon

Annex/AutoMerge.hs

@@ -137,7 +137,7 @@ resolveMerge us them inoverlay = do
 	(fs, cleanup) <- inRepo (LsFiles.unmerged [top])
 	srcmap <- if inoverlay
 		then pure M.empty
-		else inodeMap $ pure (map LsFiles.unmergedFile fs, return True)
+		else inodeMap $ pure (concatMap getunmergedfiles fs, return True)
 	(mergedks, mergedfs) <- unzip <$> mapM (resolveMerge' srcmap us them inoverlay) fs
 	let mergedks' = concat mergedks
 	let mergedfs' = catMaybes mergedfs
@@ -160,6 +160,11 @@ resolveMerge us them inoverlay = do
 			cleanConflictCruft mergedks' mergedfs' unstagedmap
 		showLongNote "Merge conflict was automatically resolved; you may want to examine the result."
 	return merged
+  where
+	getunmergedfiles u = catMaybes
+		[ Just (LsFiles.unmergedFile u)
+		, LsFiles.unmergedSiblingFile u
+		]
 
 resolveMerge' :: InodeMap -> Maybe Git.Ref -> Git.Ref -> Bool -> LsFiles.Unmerged -> Annex ([Key], Maybe FilePath)
 resolveMerge' _ Nothing _ _ _ = return ([], Nothing)
@@ -177,7 +182,7 @@ resolveMerge' unstagedmap (Just us) them inoverlay u = do
 				unless inoverlay $
 					unless (islocked LsFiles.valUs) $
 						liftIO $ removeWhenExistsWith R.removeLink (toRawFilePath file)
-			| otherwise -> do
+			| otherwise -> resolveby [keyUs, keyThem] $
 				-- Only resolve using symlink when both
 				-- were locked, otherwise use unlocked
 				-- pointer.
@@ -185,13 +190,12 @@ resolveMerge' unstagedmap (Just us) them inoverlay u = do
 				if islocked LsFiles.valUs && islocked LsFiles.valThem
 					then makesymlink keyUs file
 					else makepointer keyUs file (combinedmodes)
-				return ([keyUs, keyThem], Just file)
 		-- Our side is annexed file, other side is not.
 		-- Make the annexed file into a variant file and graft in the
 		-- other file/directory as it was.
 		(Just keyUs, Nothing) -> resolveby [keyUs] $ do
 			graftin them file LsFiles.valThem LsFiles.valThem LsFiles.valUs
-			makevariantannexlink keyUs LsFiles.valUs 
+			makevariantannexlink keyUs LsFiles.valUs
 		-- Our side is not annexed file, other side is.
 		(Nothing, Just keyThem) -> resolveby [keyThem] $ do
 			graftin us file LsFiles.valUs LsFiles.valUs LsFiles.valThem
@@ -200,6 +204,7 @@ resolveMerge' unstagedmap (Just us) them inoverlay u = do
 		(Nothing, Nothing) -> return ([], Nothing)
   where
 	file = fromRawFilePath $ LsFiles.unmergedFile u
+	sibfile = fromRawFilePath <$> LsFiles.unmergedSiblingFile u
 
 	getkey select = 
 		case select (LsFiles.unmergedSha u) of
@@ -256,21 +261,27 @@ resolveMerge' unstagedmap (Just us) them inoverlay u = do
 	graftin b item selectwant selectwant' selectunwant = do
 		Annex.Queue.addUpdateIndex
 			=<< fromRepo (UpdateIndex.lsSubTree b item)
+				
+		let replacefile isexecutable = case selectwant' (LsFiles.unmergedSha u) of
+			Nothing -> noop
+			Just sha -> replaceWorkTreeFile item $ \tmp -> do
+				c <- catObject sha
+				liftIO $ L.writeFile tmp c
+				when isexecutable $
+					liftIO $ void $ tryIO $ 
+						modifyFileMode (toRawFilePath tmp) $
+							addModes executeModes
 
 		-- Update the work tree to reflect the graft.
 		unless inoverlay $ case (selectwant (LsFiles.unmergedTreeItemType u), selectunwant (LsFiles.unmergedTreeItemType u)) of
-			-- Symlinks are never left in work tree when
-			-- there's a conflict with anything else.
-			-- So, when grafting in a symlink, we must create it:
 			(Just TreeSymlink, _) -> do
 				case selectwant' (LsFiles.unmergedSha u) of
 					Nothing -> noop
 					Just sha -> do
 						link <- catSymLinkTarget sha
 						replacewithsymlink item link
-			-- And when grafting in anything else vs a symlink,
+			(Just TreeFile, Just TreeSymlink) -> replacefile False
-			-- the work tree already contains what we want.
+			(Just TreeExecutable, Just TreeSymlink) -> replacefile True
-			(_, Just TreeSymlink) -> noop
 			_ -> ifM (liftIO $ doesDirectoryExist item)
 				-- a conflict between a file and a directory
 				-- leaves the directory, so since a directory
@@ -278,23 +289,24 @@ resolveMerge' unstagedmap (Just us) them inoverlay u = do
 				( noop
 				-- probably a file with conflict markers is
 				-- in the work tree; replace with grafted
-				-- file content
+				-- file content (this is needed when
-				, case selectwant' (LsFiles.unmergedSha u) of
+				-- the annexed file is unlocked)
-					Nothing -> noop
+				, replacefile False
-					Just sha -> replaceWorkTreeFile item $ \tmp -> do
-						c <- catObject sha
-						liftIO $ L.writeFile tmp c
 				)
 	
 	resolveby ks a = do
-		{- Remove conflicted file from index so merge can be resolved. -}
+		{- Remove conflicted file from index so merge can be resolved.
+		 - If there's a sibling conflicted file, remove it too. -}
 		Annex.Queue.addCommand [] "rm"
 			[ Param "--quiet"
 			, Param "-f"
 			, Param "--cached"
 			, Param "--"
 			]
-			[file]
+			(catMaybes [Just file, sibfile])
+		liftIO $ maybe noop
+			(removeWhenExistsWith R.removeLink . toRawFilePath)
+			sibfile
 		void a
 		return (ks, Just file)
 

CHANGELOG

@@ -3,6 +3,9 @@ git-annex (8.20211118) UNRELEASED; urgency=medium
   * Bugfix: When -J was enabled, getting files could leak an
     ever-growing number of git cat-file processes.
   * importfeed: Display url before starting youtube-dl download.
+  * Support git's new "ort" resolver, which became the default in git 2.34.0,
+    and broke the test suite and automatic merge resolution of a conflict
+    between an annexed file and a non-annexed file.
 
  -- Joey Hess <id@joeyh.name>  Wed, 17 Nov 2021 13:18:44 -0400
 

Git/LsFiles.hs

@@ -5,6 +5,8 @@
  - Licensed under the GNU AGPL version 3 or higher.
  -}
 
+{-# LANGUAGE OverloadedStrings #-}
+
 module Git.LsFiles (
 	Options(..),
 	inRepo,
@@ -236,7 +238,14 @@ data Unmerged = Unmerged
 	{ unmergedFile :: RawFilePath
 	, unmergedTreeItemType :: Conflicting TreeItemType
 	, unmergedSha :: Conflicting Sha
-	}
+	, unmergedSiblingFile :: Maybe RawFilePath
+	-- ^ Normally this is Nothing, because a
+	-- merge conflict is represented as a single file with two
+	-- stages. However, git resolvers sometimes choose to stage
+	-- two files, one for each side of the merge conflict. In such a case,
+	-- this is used for the name of the second file, which is related
+	-- to the first file. (Eg, "foo" and "foo~ref")
+	} deriving (Show)
 
 {- Returns a list of the files in the specified locations that have
  - unresolved merge conflicts.
@@ -246,7 +255,7 @@ data Unmerged = Unmerged
  -   1 = old version, can be ignored
  -   2 = us
  -   3 = them
- - If a line is omitted, that side removed the file.
+ - If line 2 or 3 is omitted, that side removed the file.
  -}
 unmerged :: [RawFilePath] -> Repo -> IO ([Unmerged], IO Bool)
 unmerged l repo = guardSafeForLsFiles repo $ do
@@ -265,7 +274,7 @@ data InternalUnmerged = InternalUnmerged
 	, ifile :: RawFilePath
 	, itreeitemtype :: Maybe TreeItemType
 	, isha :: Maybe Sha
-	}
+	} deriving (Show)
 
 parseUnmerged :: String -> Maybe InternalUnmerged
 parseUnmerged s
@@ -296,16 +305,25 @@ reduceUnmerged c (i:is) = reduceUnmerged (new:c) rest
 		{ unmergedFile = ifile i
 		, unmergedTreeItemType = Conflicting treeitemtypeA treeitemtypeB
 		, unmergedSha = Conflicting shaA shaB
+		, unmergedSiblingFile = if ifile sibi == ifile i
+			then Nothing
+			else Just (ifile sibi)
 		}
 	findsib templatei [] = ([], removed templatei)
 	findsib templatei (l:ls)
-		| ifile l == ifile templatei = (ls, l)
+		| ifile l == ifile templatei || issibfile templatei l = (ls, l)
 		| otherwise = (l:ls, removed templatei)
 	removed templatei = templatei
 		{ isus = not (isus templatei)
 		, itreeitemtype = Nothing
 		, isha = Nothing
 		}
+	-- foo~<ref> are unmerged sibling files of foo
+	-- Some versions or resolvers of git stage the sibling files,
+	-- other versions or resolvers do not.
+	issibfile x y = (ifile x <> "~") `S.isPrefixOf` ifile y
+		&& isus x || isus y
+		&& not (isus x && isus y)
 
 {- Gets the InodeCache equivilant information stored in the git index.
  -

doc/bugs/git_2.34__58___some_conflict_resolution_unit_tests_fail.mdwn

@@ -137,3 +137,5 @@ I'm also building select versions of git-annex for both Windows native and WSL1/
 development on master. A fine piece of software it definitely is.
 
 [[!meta author=jkniiv]]
+
+> [[fixed|done]] --[[Joey]]

doc/bugs/git_2.34__58___some_conflict_resolution_unit_tests_fail/comment_3_84a9988ee5ac6fb4b8ded7aa3164846f._comment

@@ -0,0 +1,38 @@
+[[!comment format=mdwn
+ username="joey"
+ subject="""comment 3"""
+ date="2021-11-22T17:31:54Z"
+ content="""
+On to the second test failure. When there's a conflict between
+a symlink and a regular file, the ort resolver uses
+the names `foo` and `foo~<ref>`.
+(Again not using stable naming, alas), and
+the files are staged in conflict, one "added by us"
+and the other "added by them".
+
+The old resolver's behavior is to leave a single file
+`foo` in conflict state "both added" and containing the content of the
+file; the content of the symlink is staged as added by them.
+
+git-annex's merge conflict resolution does not deal with this well,
+because it doesn't know those two files are related. So it sees
+a file eg `foo~HEAD` that is in conflict, but the conflict does
+not involve an annexed file. So it does not try to resolve that merge,
+because resolving a merge not involving an annexed file is out of scope.
+
+Ugh. I think something has to be done about this, making the test suite
+use the old resolver is not sufficient because git-annex is supposed to
+recover from this kind of merge conflict.
+
+Bear in mind that a non-annexed file with a name like `foo~HEAD`
+that is in "added by us" state can also happen when a file is modified by
+us, and deleted by them. So resolving such a file by adding it makes a
+decision that git-annex does not want to make about a non-annexed file.
+
+So, it seems that to fix this, git-annex will have to somehow learn
+that `foo` and `foo~<ref>` are the two sides of a merge
+conflict. It would have to base that on the filenames that git uses
+and the fact that one is a symlink and the other is a normal file.
+
+Ok, done..
+"""]]