Fix for FastForward detach issue + test.

[angusbjones]

This is the fix described in the comments of https://github.com/libgit2/libgit2sharp/pull/608/files.

[nulltoken]

I'm afraid this won't work in detached HEAD mode.

Update: I've worked a bit on this. How about the following

diff --git a/LibGit2Sharp.Tests/MergeFixture.cs b/LibGit2Sharp.Tests/MergeFixture.cs
index 8a9976e..2867b9e 100644
--- a/LibGit2Sharp.Tests/MergeFixture.cs
+++ b/LibGit2Sharp.Tests/MergeFixture.cs
@@ -1,7 +1,7 @@
-using System;
-using System.Linq;
+using System.Linq;
 using LibGit2Sharp.Tests.TestHelpers;
 using Xunit;
+using Xunit.Extensions;

 namespace LibGit2Sharp.Tests
 {
@@ -143,8 +143,10 @@ public void IsUpToDateMerge()
             }
         }

-        [Fact]
-        public void CanFastForwardRepos()
+        [Theory]
+        [InlineData(true)]
+        [InlineData(false)]
+        public void CanFastForwardRepos(bool shouldMergeOccurInDetachedHeadState)
         {
             const string firstBranchFileName = "first branch file.txt";
             const string sharedBranchFileName = "first+second branch file.txt";
@@ -171,12 +173,28 @@ public void CanFastForwardRepos()

                 secondBranch.Checkout();

+                if (shouldMergeOccurInDetachedHeadState)
+                {
+                    // Detaches HEAD
+                    repo.Checkout(repo.Head.Tip);
+                }
+
+                Assert.Equal(shouldMergeOccurInDetachedHeadState, repo.Info.IsHeadDetached);
+
                 MergeResult mergeResult = repo.Merge(repo.Branches["FirstBranch"].Tip, Constants.Signature);

                 Assert.Equal(MergeStatus.FastForward, mergeResult.Status); 
-                Assert.Equal(repo.Branches["SecondBranch"].Tip, mergeResult.Commit);
-                Assert.Equal(repo.Branches["SecondBranch"].Tip, repo.Head.Tip);
+                Assert.Equal(repo.Head.Tip, mergeResult.Commit);
+
                 Assert.Equal(0, repo.Index.RetrieveStatus().Count());
+                Assert.Equal(shouldMergeOccurInDetachedHeadState, repo.Info.IsHeadDetached);
+
+                if (!shouldMergeOccurInDetachedHeadState)
+                {
+                    // Ensure HEAD is still attached and points to SecondBranch
+                    Assert.Equal(repo.Refs.Head.TargetIdentifier, secondBranch.CanonicalName);
+                }
+
             }
         }
diff --git a/LibGit2Sharp/Repository.cs b/LibGit2Sharp/Repository.cs
index 621c522..7854e8c 100644
--- a/LibGit2Sharp/Repository.cs
+++ b/LibGit2Sharp/Repository.cs
@@ -1101,7 +1101,9 @@ private void FastForward(Commit fastForwardCommit)

             CheckoutTree(fastForwardCommit.Tree, null, checkoutOpts);

-            Refs.UpdateTarget(this.Head.CanonicalName, fastForwardCommit.Id.Sha);
+            var reference = Refs.Head.ResolveToDirectReference();
+
+            Refs.UpdateTarget(reference, fastForwardCommit.Id.Sha);

             // TODO: Update Reflog...
         }

[nulltoken]

BTW, shouldn't we also update the HEAD when performing a non fast forward merge?

[ethomson]

BTW, shouldn't we also update the HEAD when performing a non fast forward merge?

Definitely.

[angusbjones]

I was under the impression the HEAD was kept up to date for a standard merge? I put similar changes to those given by @nulltoken to the nonfastforward test and it seems to pass fine without any HEAD updating although I could be missing something.

• @ethomson

[jamill]

{ } around the else statement?

[ethomson]

Well, I was sort of vague, and I apologize: if we do the commit as part of the merge completion, then we should be sure to pass HEAD as the ref to update.

[jamill]

{ } around else statement?

[jamill]

BTW, shouldn't we also update the HEAD when performing a non fast forward merge?

Well, I was sort of vague, and I apologize: if we do the commit as part of the merge completion, then we should be sure to pass HEAD as the ref to update.

This should be handled by the current Commit logic that is run during the normal merge.

[ethomson]

Yeah, sorry, that's what I get for not really paying attention to this conversation. What I meant to say - and did so very poorly - is that: if you are detached, then HEAD should point to the resultant commit (be it fast-forward or no). If you are in a normal situation where HEAD points to a ref, then you peel HEAD and update that ref with the resultant commit (be it fast-forward or no.)

(I'm sure this is very clear to everybody, but I felt bad that I dropped in to be very vague and then left.)

[angusbjones]

I've just added the changes suggested by @jamill. Did anyone see anything else that should be added here?

• @jamill / @nulltoken / @ethomson

[nulltoken]

👍 for me. @jamill ?

@CrumblyCake Once @jamill 👍 this, maybe could you rewrite this PR so that it shows two commits?

• Fix Fastforward merge behavior
  • With only the change to Repository.cs and the minimal required changes to MergeFixture so that the tests pass
• Enhance merge test coverage
  • With the rest of the changes leveraging a detached/attached HEAD

[jamill]

👍 Thanks @CrumblyCake, looks good!

[nulltoken]

@CrumblyCake Thinking about it, if that's easier for you, we could also go with only one squashed commit "Fix Fastforward merge behavior". 😉

[angusbjones]

I put your request into a new pull request at #620 @nulltoken. I'm not to sure of the process with this kind of thing, would it have been better to have just reset back on this branch and committed here?

[nulltoken]

would it have been better to have just reset back on this branch and committed here

That would have been easier for you 😉

[nulltoken]

Let's close this in favor of #620