fixed merging of changes from adjusted branch + a remote

Annex/AdjustedBranch.hs

@@ -205,15 +205,18 @@ preventCommits = bracket setup cleanup
  - metadata is based on the parent.
  -}
 commitAdjustedTree :: Sha -> Ref -> Annex Sha
-commitAdjustedTree treesha parent = go =<< catCommit parent
+commitAdjustedTree treesha parent = commitAdjustedTree' treesha parent [parent]
+
+commitAdjustedTree' :: Sha -> Ref -> [Ref] -> Annex Sha
+commitAdjustedTree' treesha basis parents = go =<< catCommit basis
   where
 	go Nothing = inRepo mkcommit
-	go (Just parentcommit) = inRepo $ commitWithMetaData
+	go (Just basiscommit) = inRepo $ commitWithMetaData
-		(commitAuthorMetaData parentcommit)
+		(commitAuthorMetaData basiscommit)
-		(commitCommitterMetaData parentcommit)
+		(commitCommitterMetaData basiscommit)
 		mkcommit
 	mkcommit = Git.Branch.commitTree Git.Branch.AutomaticCommit
-		adjustedBranchCommitMessage [parent] treesha
+		adjustedBranchCommitMessage parents treesha
 
 adjustedBranchCommitMessage :: String
 adjustedBranchCommitMessage = "git-annex adjusted branch"
@@ -222,13 +225,14 @@ adjustedBranchCommitMessage = "git-annex adjusted branch"
  - branch into it. -}
 updateAdjustedBranch :: Branch -> (OrigBranch, Adjustment) -> Git.Branch.CommitMode -> Annex Bool
 updateAdjustedBranch tomerge (origbranch, adj) commitmode = catchBoolIO $
-	join $ preventCommits $ \_ -> go =<< (,)
+	join $ preventCommits $ \commitsprevented -> go commitsprevented =<< (,)
 		<$> inRepo (Git.Ref.sha tomerge)
 		<*> inRepo Git.Branch.current
   where
-	go (Just mergesha, Just currbranch) =
+	go commitsprevented (Just mergesha, Just currbranch) =
 		ifM (inRepo $ Git.Branch.changed currbranch mergesha)
 			( do
+				void $ propigateAdjustedCommits' origbranch (adj, currbranch) commitsprevented
 				adjustedtomerge <- adjust adj mergesha
 				ifM (inRepo $ Git.Branch.changed currbranch adjustedtomerge)
 					( return $
@@ -242,24 +246,56 @@ updateAdjustedBranch tomerge (origbranch, adj) commitmode = catchBoolIO $
 					)
 			, nochangestomerge
 			)
-	go _ = return $ return False
+	go _ _ = return $ return False
 	nochangestomerge = return $ return True
-	{- Once a merge commit has been made, re-do it, removing
-	 - the old version of the adjusted branch as a parent, and
-	 - making the only parent be the branch that was merged in.
-	 -
-	 - Doing this ensures that the same commit Sha is
-	 - always arrived at for a given commit from the merged in branch.
 
-	 - Also, update the origbranch.
+	{- A merge commit has been made on the adjusted branch.
+	 - Now, re-do it, removing the old version of the adjusted branch
+	 - from its history.
+	 -
+	 - There are two possible scenarios; either some commits
+	 - were made on top of the adjusted branch's adjusting commit,
+	 - or not. Those commits have already been propigated to the
+	 - orig branch, so we can just check if there are commits in the
+	 - orig branch that are not present in tomerge.
 	 -}
-	recommit currbranch parent (Just commit) = do
+	recommit currbranch mergedsha (Just mergecommit) =
-		commitsha <- commitAdjustedTree (commitTree commit) parent
+		ifM (inRepo $ Git.Branch.changed tomerge origbranch)
-		inRepo $ Git.Branch.update "updating original branch" origbranch parent
+			( remerge currbranch mergedsha mergecommit
-		inRepo $ Git.Branch.update "rebasing adjusted branch on top of updated original branch after merge" currbranch commitsha
+				=<< inRepo (Git.Ref.sha origbranch)
-		return True
+			, fastforward currbranch mergedsha mergecommit
+			)
 	recommit _ _ Nothing = return False
 
+	{- Fast-forward scenario. The mergecommit is changed to a non-merge
+	 - commit, with its parent being the mergedsha. 
+	 - The orig branch can simply be pointed at the mergedsha.
+	 -}
+	fastforward currbranch mergedsha mergecommit = do
+		commitsha <- commitAdjustedTree (commitTree mergecommit) mergedsha
+		inRepo $ Git.Branch.update "fast-forward update of adjusted branch" currbranch commitsha
+		inRepo $ Git.Branch.update "updating original branch" origbranch mergedsha
+		return True
+		
+	{- True merge scenario. -}
+	remerge currbranch mergedsha mergecommit (Just origsha) = do
+	 	-- Update origbranch by reverse adjusting the mergecommit,
+		-- yielding a merge between orig and tomerge.
+		treesha <- reverseAdjustedTree origsha adj
+			-- get 1-parent commit because 
+			-- reverseAdjustedTree does not support merges
+			=<< commitAdjustedTree (commitTree mergecommit) origsha
+		revadjcommit <- inRepo $
+			Git.Branch.commitTree Git.Branch.AutomaticCommit
+				("Merge branch " ++ fromRef tomerge) [origsha, mergedsha] treesha
+		inRepo $ Git.Branch.update "updating original branch" origbranch revadjcommit
+		-- Update currbranch, reusing mergedsha, but making its
+		-- parent be the updated origbranch.
+		adjcommit <- commitAdjustedTree' (commitTree mergecommit) revadjcommit [revadjcommit]
+		inRepo $ Git.Branch.update rebaseOnTopMsg currbranch adjcommit
+		return True
+	remerge _ _ _ Nothing = return False
+
 {- Check for any commits present on the adjusted branch that have not yet
  - been propigated to the orig branch, and propigate them.
  -
@@ -268,9 +304,16 @@ updateAdjustedBranch tomerge (origbranch, adj) commitmode = catchBoolIO $
  -}
 propigateAdjustedCommits :: OrigBranch -> (Adjustment, AdjBranch) -> Annex ()
 propigateAdjustedCommits origbranch (adj, currbranch) = 
-	preventCommits $ propigateAdjustedCommits' origbranch (adj, currbranch)
+	preventCommits $ \commitsprevented -> do
+		join $ propigateAdjustedCommits' origbranch (adj, currbranch) commitsprevented
 		
-propigateAdjustedCommits' :: OrigBranch -> (Adjustment, AdjBranch) -> CommitsPrevented -> Annex ()
+{- Returns action which will rebase the adjusted branch on top of the
+ - updated orig branch. -}
+propigateAdjustedCommits'
+	:: OrigBranch
+	-> (Adjustment, AdjBranch)
+	-> CommitsPrevented
+	-> Annex (Annex ())
 propigateAdjustedCommits' origbranch (adj, currbranch) _commitsprevented = do
 	ov <- inRepo $ Git.Ref.sha (Git.Ref.under "refs/heads" origbranch)
 	case ov of
@@ -282,11 +325,11 @@ propigateAdjustedCommits' origbranch (adj, currbranch) _commitsprevented = do
 					case v of
 						Left e -> do
 							warning e
-							return ()
+							return $ return ()
-						Right newparent ->
+						Right newparent -> return $
 							rebase currcommit newparent
-				Nothing -> return ()
+				Nothing -> return $ return ()
-		Nothing -> return ()
+		Nothing -> return $ return ()
   where
 	newcommits = inRepo $ Git.Branch.changedCommits origbranch currbranch
 		-- Get commits oldest first, so they can be processed
@@ -312,41 +355,53 @@ propigateAdjustedCommits' origbranch (adj, currbranch) _commitsprevented = do
 		-- and reparent it on top of the new
 		-- version of the origbranch.
 		commitAdjustedTree (commitTree currcommit) newparent
-			>>= inRepo . Git.Branch.update "rebasing adjusted branch on top of updated original branch" currbranch
+			>>= inRepo . Git.Branch.update rebaseOnTopMsg currbranch
 
-{- Reverses an adjusted commit, and commit on top of the provided newparent,
+rebaseOnTopMsg :: String
+rebaseOnTopMsg = "rebasing adjusted branch on top of updated original branch"
+
+{- Reverses an adjusted commit, and commit with provided commitparent,
  - yielding a commit sha.
  -
- - Adjust the tree of the newparent, changing only the files that the
+ - Adjusts the tree of the commitparent, changing only the files that the
  - commit changed, and reverse adjusting those changes.
  -
- - Note that the commit message, and the author and committer metadata are
+ - The commit message, and the author and committer metadata are
- - copied over. However, any gpg signature will be lost, and any other
+ - copied over from the basiscommit. However, any gpg signature
- - headers are not copied either. -}
+ - will be lost, and any other headers are not copied either. -}
 reverseAdjustedCommit :: Sha -> Adjustment -> (Sha, Commit) -> OrigBranch -> Annex (Either String Sha)
-reverseAdjustedCommit newparent adj (csha, c) origbranch
+reverseAdjustedCommit commitparent adj (csha, basiscommit) origbranch
-	-- commitDiff does not support merge commits
+	| length (commitParent basiscommit) > 1 = return $
-	| length (commitParent c) > 1 = return $
 		Left $ "unable to propigate merge commit " ++ show csha ++ " back to " ++ show origbranch
 	| otherwise = do
-		(diff, cleanup) <- inRepo (Git.DiffTree.commitDiff csha)
+		treesha <- reverseAdjustedTree commitparent adj csha
-		let (adds, others) = partition (\dti -> Git.DiffTree.srcsha dti == nullSha) diff
-		let (removes, changes) = partition (\dti -> Git.DiffTree.dstsha dti == nullSha) others
-		adds' <- catMaybes <$>
-			mapM (adjustTreeItem reverseadj) (map diffTreeToTreeItem adds)
-		treesha <- Git.Tree.adjustTree
-			(propchanges changes)
-			adds'
-			(map Git.DiffTree.file removes)
-			newparent
-			=<< Annex.gitRepo
-		void $ liftIO cleanup
 		revadjcommit <- inRepo $ commitWithMetaData
-			(commitAuthorMetaData c)
+			(commitAuthorMetaData basiscommit)
-			(commitCommitterMetaData c) $
+			(commitCommitterMetaData basiscommit) $
 				Git.Branch.commitTree Git.Branch.AutomaticCommit
-					(commitMessage c) [newparent] treesha
+					(commitMessage basiscommit) [commitparent] treesha
 		return (Right revadjcommit)
+
+{- Adjusts the tree of the basis, changing only the files that the
+ - commit changed, and reverse adjusting those changes.
+ -
+ - commitDiff does not support merge commits, so the csha must not be a
+ - merge commit. -}
+reverseAdjustedTree :: Sha -> Adjustment -> Sha -> Annex Sha
+reverseAdjustedTree basis adj csha = do
+	(diff, cleanup) <- inRepo (Git.DiffTree.commitDiff csha)
+	let (adds, others) = partition (\dti -> Git.DiffTree.srcsha dti == nullSha) diff
+	let (removes, changes) = partition (\dti -> Git.DiffTree.dstsha dti == nullSha) others
+	adds' <- catMaybes <$>
+		mapM (adjustTreeItem reverseadj) (map diffTreeToTreeItem adds)
+	treesha <- Git.Tree.adjustTree
+		(propchanges changes)
+		adds'
+		(map Git.DiffTree.file removes)
+		basis
+		=<< Annex.gitRepo
+	void $ liftIO cleanup
+	return treesha
   where
 	reverseadj = reverseAdjustment adj
 	propchanges changes ti@(TreeItem f _ _) =

Command/Sync.hs

@@ -243,9 +243,7 @@ commitStaged commitmode commitmessage = do
 	return True
 
 mergeLocal :: CurrBranch -> CommandStart
-mergeLocal currbranch@(Just branch, madj) = do
+mergeLocal currbranch@(Just branch, madj) = go =<< needmerge
-	proptoorig
-	go =<< needmerge
   where
 	syncbranch = syncBranch branch
 	needmerge = ifM isBareRepo
@@ -260,11 +258,6 @@ mergeLocal currbranch@(Just branch, madj) = do
 		showStart "merge" $ Git.Ref.describe syncbranch
 		next $ next $ merge currbranch Git.Branch.ManualCommit syncbranch
 	branch' = maybe branch (originalToAdjusted branch) madj
-	-- When in an adjusted branch, propigate any changes made to it
-	-- back to the original branch.
-	proptoorig = case madj of
-		Just adj -> propigateAdjustedCommits branch (adj, branch')
-		Nothing -> return ()
 mergeLocal (Nothing, _) = stop
 
 pushLocal :: CurrBranch -> CommandStart
@@ -274,7 +267,13 @@ pushLocal b = do
 
 updateSyncBranch :: CurrBranch -> Annex ()
 updateSyncBranch (Nothing, _) = noop
-updateSyncBranch (Just branch, _) = do
+updateSyncBranch (Just branch, madj) = do
+	-- When in an adjusted branch, propigate any changes made to it
+	-- back to the original branch.
+	case madj of
+		Just adj -> propigateAdjustedCommits branch
+			(adj, originalToAdjusted branch adj)
+		Nothing -> return ()
 	-- Update the sync branch to match the new state of the branch
 	inRepo $ updateBranch (syncBranch branch) branch
 	-- In direct mode, we're operating on some special direct mode

doc/design/adjusted_branches.mdwn

@@ -109,10 +109,10 @@ beginning the merge. There may be staged changes, or changes in the work tree.
 
 First filter the new commit:
 
-	origin/master    adjusted/master
+	origin/master    adjusted/master      master
-	A
+	A                                     A
-	|--------------->A'
+	|--------------->A'                   |
-	|                |
+	|                |                    |
 	|                |
 	B
 	|                
@@ -120,10 +120,10 @@ First filter the new commit:
 
 Then, merge that into adjusted/master:
 
-	origin/master    adjusted/master
+	origin/master    adjusted/master     master
-	A
+	A                                    A
-	|--------------->A'
+	|--------------->A'                  |
-	|                |
+	|                |                   |
 	|                |
 	B                |
 	|                |
@@ -136,35 +136,13 @@ conflict should only affect the work tree/index, so can be resolved without
 making a commit, but B'' may end up being made to resolve a merge
 conflict.)
 
-------
+Once the merge is done, we have a merge commit B'' on adjusted/master.
+To finish, redo that commit so it does not have A' as its parent.
 
-TODO FIXME: When an adjusted unlocked branch has gotten a file, and a new
+	origin/master    adjusted/master     master
-commit is merged in, that does not touch that file, there is a false merge
+	A                                    A
-conflict on the file. It's auto-resolved by creating a .variant file.
+	|--------------->A'                  |
-This is probably a bug in the auto-resolve code for v6 files.
+	|                |                   |
-
-Test case:
-
-	git clone ~/lib/tmp
-	cd tmp
-	git annex upgrade
-	git annex adjust
-	git annex get t/foo
-	# make change in ~/lib/tmp and commit
-	git annex sync
-	# t/foo.variant-* is there
-
-------
-
-
-
-Once the merge is done, we have a commit B'' on adjusted/master. To finish,
-adjust that commit so it does not have adjusted/master as its parent.
-
-	origin/master    adjusted/master
-	A
-	|--------------->A'
-	|                |
 	|                |
 	B
 	|                
@@ -173,6 +151,16 @@ adjust that commit so it does not have adjusted/master as its parent.
 
 Finally, update master, by reverse filtering B''.
 	
+	origin/master    adjusted/master    master
+	A                                   A
+	|--------------->A'                 |
+	|                |                  |
+	|                |                  |
+	B                                   |
+	|                                   |
+	|--------------->B'' - - - - - - -> B
+	|                |
+
 Notice how similar this is to the commit graph. So, "fast-forward" 
 merging the same B commit from origin/master will lead to an identical
 sha for B' as the original committer got.
@@ -191,6 +179,66 @@ between the adjusted work tree and pulled changes. A post-merge hook would
 be needed to re-adjust the work tree, and there would be a window where eg,
 not present files would appear in the work tree.]
 
+## another merge scenario
+
+Another merge scenario is when there's a new commit C on adjusted/master,
+and also a new commit B on origin/master.
+
+Start by adjusting B':
+
+	origin/master    adjusted/master      master
+	A                                     A
+	|--------------->A'                   |
+	|                |                    |
+	|                C'
+	B
+	|                
+	|---------->B'
+
+Then, merge B' into adjusted/master:
+
+	origin/master    adjusted/master     master
+	A                                    A
+	|--------------->A'                  |
+	|                |                   |
+	|                C'
+	B                |
+	|                |
+	|----------->B'->M'
+
+Here M' is the correct tree, but it has A' as its grandparent,
+which is the adjusted branch commit, so needs to be dropped in order to 
+get a commit that can be put on master.
+
+We don't want to lose commit C', but it's an adjusted
+commit, so needs to be de-adjusted.
+
+	origin/master    adjusted/master     master
+	A                                    A
+	|--------------->A'                  |
+	|                |                   |
+	|                C'- - - - - - - - > C
+	B                |
+	|                |
+	|----------->B'->M'
+	|               
+
+Now, we generate a merge commit, between B and C, with known result M'
+(so no actual merging done here).
+
+	origin/master    adjusted/master     master
+	A                                    A
+	|--------------->A'                  |
+	|                |                   |
+	|                C'- - - - - - - - > C
+	B                                    |
+	|                                    |
+	|--------------->M'<-----------------|
+	|               
+
+Finally, update master, by reverse filtering M'. The resulting commit
+on master will also be a merge between B and C.
+
 ## annex object add/remove
 
 When objects are added/removed from the annex, the associated file has to
@@ -303,20 +351,32 @@ into adjusted view worktrees.]
 * Honor annex.thin when entering an adjusted branch.
 * Cloning a repo that has an adjusted branch checked out gets into an ugly
   state.
+* There are potentially races in code that assumes a branch like
+  master is not being changed by someone else. In particular,
+  propigateAdjustedCommits rebases the adjusted branch on top of master.
+  That is called by sync. The assumption is that any changes in master
+  have already been handled by updateAdjustedBranch. But, if another remote
+  pushed a new master at just the right time, the adjusted branch could be
+  rebased on top of a master that it doesn't incorporate, which is wrong.
 
-Bug running git-annex sync in adjusted branch when there is a local change
+------
-that gets committed (or already has been), and remote changes available.
-Both propigateAdjustedCommits and updateAdjustedBranch
-get called in this scenario. Neither order of calling the two works entirely.
 
-The reflog has:
+TODO FIXME: When an adjusted unlocked branch has gotten a file, and a new
+commit is merged in, that does not touch that file, there is a false merge
+conflict on the file. It's auto-resolved by creating a .variant file.
+This is probably a bug in the auto-resolve code for v6 files.
 
-d585d7f HEAD@{1}: rebasing adjusted branch on top of updated original branch
+Test case:
-e51daec HEAD@{2}: merge f7f2b9f3b1d1c97a1ab24f4a94d4a27d84898992: Merge made by the 'recursive' strategy.
+
-9504e7b HEAD@{3}: rebasing adjusted branch on top of updated original branch
+	git clone ~/lib/tmp
-6c6fd41 HEAD@{4}: commit: add
+	cd tmp
+	git annex upgrade
+	git annex adjust
+	git annex get t/foo
+	# make change in ~/lib/tmp and commit
+	git annex sync
+	# t/foo.variant-* is there
+
+------
 
-e51daec has ok correct history; it gets messed up in d585d7f
 
-Problem is just, that the commit made to the adjusted branch 
-is left out of the history.