Skip to content

Introduce basic merging #579

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
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 42 additions & 2 deletions LibGit2Sharp.Tests/MergeFixture.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
using System.Linq;
using LibGit2Sharp.Tests.TestHelpers;
using LibGit2Sharp.Tests.TestHelpers;
Copy link
Member

Choose a reason for hiding this comment

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

small style point: Normally, we have sorted the System using directives above all other directives. This default sorting behavior has changed in Visual Studio 2013.

Copy link
Member

Choose a reason for hiding this comment

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

Man, it just looks wrong to have System anywhere but sorted first. :(

Copy link
Member

Choose a reason for hiding this comment

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

yeah - at least there is a option to sort these first.

using System.Linq;
using Xunit;

namespace LibGit2Sharp.Tests
Expand Down Expand Up @@ -80,5 +80,45 @@ public void CanRetrieveTheBranchBeingMerged()
Assert.Null(mergedHeads[1].Tip);
}
}

[Fact]
public void CanMergeRepos()
{
string path = CloneStandardTestRepo();
using (var repo = new Repository(path))
{
var firstBranch = repo.CreateBranch("FirstBranch");
firstBranch.Checkout();
var originalTreeCount = firstBranch.Tip.Tree.Count;

//Commit with ONE new file to both first & second branch (SecondBranch is created on this commit).
Copy link
Member

Choose a reason for hiding this comment

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

Another small style point: We usually put a space between the // and the start of the comment.

AddFileCommitToRepo(repo, "first+second branch file");

var secondBranch = repo.CreateBranch("SecondBranch");
//Commit with ONE new file to first branch (FirstBranch moves forward as it is checked out, SecondBranch stays back one).
var firstBranchCommit = AddFileCommitToRepo(repo, "first branch file");

secondBranch.Checkout();
//Commit with ONE new file to second branch (FirstBranch and SecondBranch now point to separate commits that both have the same parent commit).
var secondBranchCommit = AddFileCommitToRepo(repo, "second branch file");

MergeResult mergeResult = repo.Merge(repo.Branches["FirstBranch"].Tip);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we assert anything regarding the state of MergeResult here?


var mergeCommit = repo.Commit("Merge First+Second", Constants.Signature, Constants.Signature);

Assert.Equal(mergeCommit.Tree.Count, originalTreeCount + 3); //Expecting original tree count plussed by the 3 added files.
Assert.Equal(mergeCommit.Parents.Count(), 2); //Merge commit should have 2 parents
}
}

private Commit AddFileCommitToRepo(IRepository repository, string filename, string content = null)
{
filename = filename + ".txt";
Touch(repository.Info.WorkingDirectory, filename, content);

repository.Index.Stage(filename);

return repository.Commit("New commit", Constants.Signature, Constants.Signature);
}
}
}
17 changes: 17 additions & 0 deletions LibGit2Sharp/Core/GitMergeHead.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
using LibGit2Sharp.Core.Handles;
using System;
using System.Runtime.InteropServices;

namespace LibGit2Sharp.Core
{
[StructLayout(LayoutKind.Sequential)]
Copy link
Member

Choose a reason for hiding this comment

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

This is internal to libgit2. Is there some reason you need it exposed?

internal struct GitMergeHead
Copy link
Member

Choose a reason for hiding this comment

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

I can't find any code leveraging this type. Am I missing anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be removed, I just pumped out a bunch of types I thought I'd need when I first started and missed removing it on the first commit, its mentioned by @ethomson above also. The reason I haven't removed it just yet though is I wasn't sure whether its what was being referenced in the comment above: #579 (diff)

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 sorry, I was super vague there. To rewind a bit: libgit2 has some types of structures that are publicly visible (defined in the public header files) - some examples of that are the git_merge_opts and git_merge_tree_opts. These are used when we need consumers to be able to set a lot of options and pass it as a single argument to a function. On the other hand, we have a bunch of "opaque objects" - structures that are defined internally, but in the public header files you will only see a forward declaration:

typedef struct git_merge_head git_merge_head;

Consumers can't create one of these because the compiler doesn't know anything about it -- but you can talk about a pointer to that structure: git_merge_head * -- since the compiler knows how big a pointer will be. Consumers can't deference it, but they can pass it to libgit2. This allows libgit2 to provide an object to consumers that the library knows about, but consumers don't, allowing us to hide some implementation details and be able to change the details of that structure without breaking compatibility for our consumers.

Since MergeHead is an opaque object in libgit2, we don't need to build a serializable structure. Instead we can call git_merge_head_from_oid, git_merge_head_from_ref or git_merge_head_from_fetchhead, get the opaque GitMergeHeadHandle back and pass that to git_merge.

Copy link
Member

Choose a reason for hiding this comment

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

I can't find any code leveraging this type. Am I missing anything?

It still looks unused. Are there any reason to keep it around?

{
[MarshalAs(UnmanagedType.CustomMarshaler, MarshalCookie = UniqueId.UniqueIdentifier, MarshalTypeRef = typeof(StrictUtf8Marshaler))] string RefName;
[MarshalAs(UnmanagedType.CustomMarshaler, MarshalCookie = UniqueId.UniqueIdentifier, MarshalTypeRef = typeof(StrictUtf8Marshaler))] string RemoteUrl;

GitOid Oid;
[MarshalAs(UnmanagedType.CustomMarshaler, MarshalCookie = UniqueId.UniqueIdentifier, MarshalTypeRef = typeof(StrictUtf8Marshaler))] string OidStr;
GitObjectSafeHandle Commit;
}
}
22 changes: 22 additions & 0 deletions LibGit2Sharp/Core/GitMergeOpts.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
using System;
using System.Runtime.InteropServices;

namespace LibGit2Sharp.Core
{
[Flags]
internal enum MergeFlags
Copy link
Member

Choose a reason for hiding this comment

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

@ethomson Can one combine those flags?

IOW, would GIT_MERGE_NO_FASTFORWARD | GIT_MERGE_FASTFORWARD_ONLY make sense?

Copy link
Member

Choose a reason for hiding this comment

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

One cannot, it definitely doesn't make sense.

Copy link
Member

Choose a reason for hiding this comment

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

But(!) my thinking is that this is a bitfield and we may have other options that could be combined later. You can't combine GIT_MERGE_NO_FASTFORWARD with GIT_MERGE_FASTFORWARD_ONLY but maybe you could combine it with something else like GIT_MERGE_DONT_WRITE_CRAPPY_METADATA_FILES or something.

Copy link
Member

Choose a reason for hiding this comment

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

@ethomson - This is not currently represented as a bit field in libgit2. If that is the intention of this type, should we update the libgit2 representation?

In addition, should there be a named default value (both here and in libgit2) - GIT_MERGE_DEFAULT = 0?

{
GIT_MERGE_NO_FASTFORWARD = 1,
GIT_MERGE_FASTFORWARD_ONLY = 2,
}

[StructLayout(LayoutKind.Sequential)]
internal struct GitMergeOpts
{
public uint Version;

public MergeFlags MergeFlags;
public GitMergeTreeOpts MergeTreeOpts;
public GitCheckoutOpts CheckoutOpts;
}
}
34 changes: 34 additions & 0 deletions LibGit2Sharp/Core/GitMergeTreeOpts.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
using LibGit2Sharp.Core.Handles;
using System;
using System.Runtime.InteropServices;

namespace LibGit2Sharp.Core
{
[Flags]
internal enum MergeTreeFlags
Copy link
Member

Choose a reason for hiding this comment

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

nit: Most of the enums are of the form Git* (i.e. GitMergeTreeFlags) (although there are a couple of exceptions to this pattern).

{
GIT_MERGE_TREE_FIND_RENAMES = (1 << 0),
}
[Flags]
internal enum MergeAutomergeFlags
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, are those exclusive options?

Copy link
Member

Choose a reason for hiding this comment

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

These are indeed exclusive to one another.

Copy link
Member

Choose a reason for hiding this comment

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

If this is not an actual flag type, maybe we should suffix with Strategy - (e.g. maybe GitMergeAutomergeStrategy)

{
GIT_MERGE_AUTOMERGE_NORMAL = 0,
GIT_MERGE_AUTOMERGE_NONE = 1,
GIT_MERGE_AUTOMERGE_FAVOR_OURS = 2,
GIT_MERGE_AUTOMERGE_FAVOR_THEIRS = 3,
}

[StructLayout(LayoutKind.Sequential)]
internal struct GitMergeTreeOpts
{
public uint Version;

public MergeTreeFlags MergeTreeFlags;
public uint RenameThreshold;
public uint TargetLimit;

public UIntPtr Metric;

public MergeAutomergeFlags MergeAutomergeFlags;
}
}
13 changes: 13 additions & 0 deletions LibGit2Sharp/Core/Handles/GitMergeHeadHandle.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
using System.Runtime.InteropServices;

namespace LibGit2Sharp.Core.Handles
{
internal class GitMergeHeadHandle : SafeHandleBase
{
protected override bool ReleaseHandleImpl()
{
Proxy.git_merge_head_free(handle);
return true;
}
}
}
13 changes: 13 additions & 0 deletions LibGit2Sharp/Core/Handles/GitMergeResultHandle.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
using System.Runtime.InteropServices;

namespace LibGit2Sharp.Core.Handles
{
internal class GitMergeResultHandle : SafeHandleBase
{
protected override bool ReleaseHandleImpl()
{
Proxy.git_merge_result_free(handle);
return true;
}
}
}
49 changes: 49 additions & 0 deletions LibGit2Sharp/Core/NativeMethods.cs
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,55 @@ internal static extern int git_merge_base(
GitObjectSafeHandle one,
GitObjectSafeHandle two);

[DllImport(libgit2)]
internal static extern int git_merge_head_from_ref(
out GitMergeHeadHandle mergehead,
RepositorySafeHandle repo,
ReferenceSafeHandle reference);

[DllImport(libgit2)]
internal static extern int git_merge_head_from_fetchhead(
out GitMergeHeadHandle mergehead,
RepositorySafeHandle repo,
[MarshalAs(UnmanagedType.CustomMarshaler, MarshalCookie = UniqueId.UniqueIdentifier, MarshalTypeRef = typeof(StrictUtf8Marshaler))] string branch_name,
[MarshalAs(UnmanagedType.CustomMarshaler, MarshalCookie = UniqueId.UniqueIdentifier, MarshalTypeRef = typeof(StrictUtf8Marshaler))] string remote_url,
ref GitOid oid);

[DllImport(libgit2)]
internal static extern int git_merge_head_from_oid(
out GitMergeHeadHandle mergehead,
RepositorySafeHandle repo,
ref GitOid oid);

[DllImport(libgit2)]
internal static extern int git_merge(
out GitMergeResultHandle mergeResult,
RepositorySafeHandle repo,
[MarshalAs(UnmanagedType.LPArray, SizeParamIndex = 2)] [In] IntPtr[] their_heads,
UIntPtr their_heads_len,
ref GitMergeOpts given_opts);

[DllImport(libgit2)]
internal static extern int git_merge_result_is_uptodate(
GitMergeResultHandle merge_result);

[DllImport(libgit2)]
internal static extern int git_merge_result_is_fastforward(
GitMergeResultHandle merge_result);

[DllImport(libgit2)]
internal static extern int git_merge_result_fastforward_oid(
out GitOid oid,
GitMergeResultHandle merge_result);

[DllImport(libgit2)]
internal static extern void git_merge_result_free(
IntPtr merge_result);

[DllImport(libgit2)]
internal static extern void git_merge_head_free(
IntPtr merge_head);

[DllImport(libgit2)]
internal static extern int git_message_prettify(
byte[] message_out, // NB: This is more properly a StringBuilder, but it's UTF8
Expand Down
85 changes: 85 additions & 0 deletions LibGit2Sharp/Core/Proxy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -934,6 +934,91 @@ public static ObjectId git_merge_base(RepositorySafeHandle repo, Commit first, C
}
}

public static GitMergeHeadHandle git_merge_head_from_oid(RepositorySafeHandle repo, GitOid oid)
{
using (ThreadAffinity())
{
GitMergeHeadHandle their_head;

int res = NativeMethods.git_merge_head_from_oid(out their_head, repo, ref oid);
if (res == (int)GitErrorCode.NotFound)
Copy link
Member

Choose a reason for hiding this comment

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

When would this be triggered?

return null;

Ensure.ZeroResult(res);

return their_head;
}
}

public static GitMergeResultHandle git_merge(RepositorySafeHandle repo, GitMergeHeadHandle[] heads, GitMergeOpts options)
{
using (ThreadAffinity())
{
GitMergeResultHandle ret;
Copy link
Member

Choose a reason for hiding this comment

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

You may be missing some code here (or in the calling layers) taking care of releasing the GitMergeResultHandle and GitMergeHeadHandle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wrapped them up in usings in the calling layers, also if there is a better approach (or just a standard that's used) please let me know?


IntPtr[] their_heads = new IntPtr[heads.Length];
for (int i = 0; i < heads.Length; i++)
{
their_heads[i] = heads[i].DangerousGetHandle();
}

int res = NativeMethods.git_merge(
out ret,
repo,
their_heads,
(UIntPtr)their_heads.Length,
ref options);

Ensure.ZeroResult(res);

return ret;
}
}

public static bool git_merge_result_is_uptodate(GitMergeResultHandle handle)
{
using (ThreadAffinity())
{
int res = NativeMethods.git_merge_result_is_uptodate(handle);
Ensure.BooleanResult(res);

return (res == 1);
}
}

public static bool git_merge_result_is_fastforward(GitMergeResultHandle handle)
{
using (ThreadAffinity())
{
int res = NativeMethods.git_merge_result_is_fastforward(handle);
Ensure.BooleanResult(res);

return (res == 1);
}
}

public static GitOid git_merge_result_fastforward_oid(GitMergeResultHandle handle)
Copy link
Member

Choose a reason for hiding this comment

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

This looks unused as well.

Copy link
Member

Choose a reason for hiding this comment

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

I would anticipate the GitMergeResult class using this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GitMergeResult now uses it.

{
using (ThreadAffinity())
{
GitOid oid;
int res = NativeMethods.git_merge_result_fastforward_oid(out oid, handle);
Ensure.ZeroResult(res);

return oid;
}
}

public static void git_merge_result_free(IntPtr handle)
{
NativeMethods.git_merge_result_free(handle);
}

public static void git_merge_head_free(IntPtr handle)
{
NativeMethods.git_merge_head_free(handle);
}

#endregion

#region git_message_
Expand Down
5 changes: 5 additions & 0 deletions LibGit2Sharp/IRepository.cs
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,11 @@ public interface IRepository : IDisposable
/// </summary>
IEnumerable<MergeHead> MergeHeads { get; }

/// <summary>
/// Merges the given commit into HEAD.
/// </summary>
MergeResult Merge(Commit commit);
Copy link
Member

Choose a reason for hiding this comment

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

Can you please add some xml doc regarding the input/output params?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, added to Repository.cs but didn't in IRepository. Coming in next commit.


/// <summary>
/// Manipulate the currently ignored files.
/// </summary>
Expand Down
6 changes: 6 additions & 0 deletions LibGit2Sharp/LibGit2Sharp.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -77,15 +77,21 @@
<Compile Include="RefSpec.cs" />
<Compile Include="RefSpecCollection.cs" />
<Compile Include="Core\EncodingMarshaler.cs" />
<Compile Include="Core\GitMergeHead.cs" />
<Compile Include="Core\GitMergeOpts.cs" />
<Compile Include="Core\GitMergeTreeOpts.cs" />
<Compile Include="Core\Handles\BranchIteratorSafeHandle.cs" />
<Compile Include="Core\Handles\ConfigurationIteratorSafeHandle.cs" />
<Compile Include="Core\GitBlame.cs" />
<Compile Include="Core\Handles\BlameSafeHandle.cs" />
<Compile Include="Core\Handles\GitMergeHeadHandle.cs" />
<Compile Include="Core\Handles\GitMergeResultHandle.cs" />
<Compile Include="Core\PushTransferProgressCallbacks.cs" />
<Compile Include="Core\PackbuilderCallbacks.cs" />
<Compile Include="PushOptions.cs" />
<Compile Include="Core\GitBuf.cs" />
<Compile Include="FilteringOptions.cs" />
<Compile Include="MergeResult.cs" />
<Compile Include="ResetMode.cs" />
<Compile Include="NoteCollectionExtensions.cs" />
<Compile Include="RefSpecDirection.cs" />
Expand Down
37 changes: 37 additions & 0 deletions LibGit2Sharp/MergeResult.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
using LibGit2Sharp.Core;
using LibGit2Sharp.Core.Handles;
using System.Diagnostics;

namespace LibGit2Sharp
{
public class MergeResult
{
public MergeResult(){}
Copy link
Member

Choose a reason for hiding this comment

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

For consistency with other protected default ctors, perhaps:

        /// <summary>
        /// Needed for mocking purposes.
        /// </summary>
        protected MergeResult()
        { }

internal MergeResult(GitMergeResultHandle handle)
{
_isUpToDate = Proxy.git_merge_result_is_uptodate(handle);
_isFastForward = Proxy.git_merge_result_is_fastforward(handle);

if (_isFastForward)
_oid = Proxy.git_merge_result_fastforward_oid(handle);
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Please add braces here

}

private bool _isUpToDate;
Copy link
Member

Choose a reason for hiding this comment

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

You could also use automatic properties with a private setter for these.

public virtual bool IsUpToDate
{
    get;
    private set;
}

Alternatively, if you decide to use the private fields, we don't usually use the _ prefix for private fields (just camel casing).

public virtual bool IsUpToDate
{
get { return _isUpToDate; }
}

private bool _isFastForward;
public virtual bool IsFastForward
{
get { return _isFastForward; }
}

private readonly GitOid _oid;
internal GitOid FastForwardOid
{
get { return _oid; }
}
}
}
Loading