Skip to content

Fix for FastForward detach issue + test. #617

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

angusbjones
Copy link
Contributor

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

@angusbjones angusbjones mentioned this pull request Jan 30, 2014
@@ -1101,7 +1101,7 @@ private void FastForward(Commit fastForwardCommit)

CheckoutTree(fastForwardCommit.Tree, null, checkoutOpts);

Refs.UpdateTarget("HEAD", fastForwardCommit.Id.Sha);
Refs.UpdateTarget(this.Head.CanonicalName, fastForwardCommit.Id.Sha);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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

@ethomson
Copy link
Member

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

Definitely.

@angusbjones
Copy link
Contributor Author

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.

repo.Checkout(secondBranch.Tip);
}
else
secondBranch.Checkout();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ } around the else statement?

@ethomson
Copy link
Member

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.

repo.Checkout(secondBranch.Tip);
}
else
secondBranch.Checkout();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ } around else statement?

@jamill
Copy link
Member

jamill commented Jan 30, 2014

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
Copy link
Member

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
Copy link
Contributor Author

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

@nulltoken
Copy link
Member

👍 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
Copy link
Member

jamill commented Feb 3, 2014

👍 Thanks @CrumblyCake, looks good!

@nulltoken
Copy link
Member

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

@angusbjones
Copy link
Contributor Author

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
Copy link
Member

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

That would have been easier for you 😉

@nulltoken nulltoken added this to the UnmergedOrDoNotRequireAFix milestone Feb 6, 2014
@nulltoken
Copy link
Member

Let's close this in favor of #620

@nulltoken nulltoken closed this Feb 6, 2014
@angusbjones angusbjones deleted the FastForward-Merge-Fix-+-Test-Fix branch February 6, 2014 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants