-
Notifications
You must be signed in to change notification settings - Fork 899
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
Fix for FastForward detach issue + test. #617
Conversation
@@ -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); |
There was a problem hiding this comment.
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...
}
BTW, shouldn't we also update the HEAD when performing a non fast forward merge? |
Definitely. |
… for non fast forward.
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ }
around the else statement?
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 |
repo.Checkout(secondBranch.Tip); | ||
} | ||
else | ||
secondBranch.Checkout(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ }
around else statement?
This should be handled by the current Commit logic that is run during the normal merge. |
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.) |
I've just added the changes suggested by @jamill. Did anyone see anything else that should be added here? |
👍 for me. @jamill ? @CrumblyCake Once @jamill 👍 this, maybe could you rewrite this PR so that it shows two commits?
|
👍 Thanks @CrumblyCake, looks good! |
@CrumblyCake Thinking about it, if that's easier for you, we could also go with only one squashed commit "Fix Fastforward merge behavior". 😉 |
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? |
That would have been easier for you 😉 |
Let's close this in favor of #620 |
This is the fix described in the comments of https://github.com/libgit2/libgit2sharp/pull/608/files.