Introduce basic merging #579

angusbjones · 2013-12-05T11:41:27Z

Hey not sure if this would be useful, I needed it for something I was doing and figured it could, at the least, be useful as a base for starting to get merging in. I'm not sure about how a caller should interact with the merging stuff, so I just did a mix of how I would want to use it/how similar stuff is done. Please feel free to tear me apart because I screwed up somewhere, very new to libgit2sharp + git/github.

nulltoken · 2013-12-05T15:44:14Z

LibGit2Sharp.Tests/LibGit2Sharp.Tests.csproj

+    <PlatformTarget>x86</PlatformTarget>
+    <ErrorReport>prompt</ErrorReport>
+    <CodeAnalysisRuleSet>MinimumRecommendedRules.ruleset</CodeAnalysisRuleSet>
+  </PropertyGroup>


Do we really need a dedicated x86 target?

Sorry about that, a cherry-pick casualty.

ethomson · 2013-12-05T15:53:34Z

This is very exciting to see! I'm excited to see merge in libgit2#.

nulltoken · 2013-12-05T15:55:05Z

LibGit2Sharp.Tests/MergeFixture.cs

+
+                repo.MergeOnto(repo.Branches["FirstBranch"].Tip);
+
+                repo.Commit("Merge First+Second");


Could you please add some assertions here?

Maybe:

Number of parents of the merge commit

Ensuring the Commit.Tree contains the expected files

In. I'm thinking there should probably be some tests related to the merge result if it's felt that its necessary?

nulltoken · 2013-12-05T16:52:09Z

@CrumblyCake Wow! Nice one!

@ethomson Thanks for jumping in, MergeMaster™ ❤️

nulltoken · 2013-12-05T16:54:10Z

On a slightly sadder note, it looks like Travis isn't as happy as we're are.

Native stacktrace:
    /usr/bin/mono() [0x4916ba]
    /usr/bin/mono() [0x4e0d4f]
    /usr/bin/mono() [0x41bc77]
    /lib/x86_64-linux-gnu/libpthread.so.0(+0xfcb0) [0x7fb12ca7fcb0]
    /home/travis/build/libgit2/libgit2sharp/libgit2/build/libgit2-98eaf39.so(git_merge+0x3d4) [0x7fb120356204]
    [0x41221333]
Debug info from gdb:
=================================================================
Got a SIGSEGV while executing native code. This usually indicates
a fatal error in the mono runtime or one of the native libraries 
used by your application.
=================================================================
./build.libgit2sharp.sh: line 26:  5350 Aborted                 xbuild CI-build.msbuild /t:Deploy

ethomson · 2013-12-05T16:55:25Z

I suspect it will pass after some of the recommended signature cleanups.

On Thu, Dec 5, 2013 at 11:54 AM, nulltoken [email protected] wrote:

On a slightly sadder note, it looks like Travis isn't as happy as we're
are.

Native stacktrace:
/usr/bin/mono() [0x4916ba]
/usr/bin/mono() [0x4e0d4f]
/usr/bin/mono() [0x41bc77]
/lib/x86_64-linux-gnu/libpthread.so.0(+0xfcb0) [0x7fb12ca7fcb0]
/home/travis/build/libgit2/libgit2sharp/libgit2/build/libgit2-98eaf39.so(git_merge+0x3d4) [0x7fb120356204]
[0x41221333]

Debug info from gdb:

Got a SIGSEGV while executing native code. This usually indicates
a fatal error in the mono runtime or one of the native libraries

used by your application.

./build.libgit2sharp.sh: line 26: 5350 Aborted xbuild CI-build.msbuild /t:Deploy

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/579#issuecomment-29915021
.

jamill · 2013-12-05T17:14:43Z

LibGit2Sharp.Tests/MergeFixture.cs

@@ -1,6 +1,7 @@
 using System.Linq;
 using LibGit2Sharp.Tests.TestHelpers;
 using Xunit;
+using System;


Is this using necessary? if so, maybe sort it with the other System.* statements?

dahlbyk · 2013-12-05T17:54:22Z

Please feel free to tear me apart

No worries there... Welcome to the party!

MergeFixture: Cleaned usings & removed unused method. NativeMethods: Used suggested signature for git_merge. Added ref+fetchhead git_merge_froms just in case they could be used in future (Should I revert this, I'm not sure on your rules about unused methods in a wrapper?) Proxy: Split git_merge and git_merge_from_oid into separate methods and used suggested signature. Fixed the result checking for boolean C calls in is_uptodate and is_fastforward. GitMergeResult: Renamed to MergeResult, put loading prop values in constructor. Repository: Replaced MergeOnto with Merge.

MergeResult: Fixed up bad reference + added FastForwardOid prop Repository: Wrapped MergeHead and MergeResult handles in usings.

nulltoken · 2013-12-11T09:12:08Z

LibGit2Sharp/Core/GitMergeOpts.cs

+namespace LibGit2Sharp.Core
+{
+    [Flags]
+    internal enum MergeFlags


@ethomson Can one combine those flags?

IOW, would GIT_MERGE_NO_FASTFORWARD | GIT_MERGE_FASTFORWARD_ONLY make sense?

One cannot, it definitely doesn't make sense.

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.

@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?

nulltoken · 2013-12-11T19:13:11Z

@CrumblyCake Do you think you could also add a test putting under the light a failed merged when a conflict occur?

@ethomson Have we got actually all we need to actually resolve the merge and perform the merge commit?

angusbjones · 2013-12-15T09:40:37Z

@nulltoken Will do for next commit, I've hit a few snags that I'll need some light shed on.
@ethomson Merging a binary file seems to actually merge the binary file (as in, <<<< Contents of first |||| Contents of second >>>>>>), should I write a test so this is replicate-able or put in an issue to the libgit2 project or what? I'm not to sure of the procedure for this scenario. Or is this expected and I have forgotten a flag to stop it?
Another issue seems to be that FastForwards don't appear to occur, the OID is generated and the flag is set in the merge result, but no changes are populated or staged, should I be calling a different method to git_merge?

Separately, while its possible (commenting on the CheckoutStrategy.GIT_CHECKOUT_USE_OURS indicates it and THEIRS are unimplemented but they work fine), the process to checkout ours/theirs on a conflict is a bit arduous when I was thinking it could just be something like CheckoutOurs(Conflict conflict) or Conflict.Ours.Checkout(). Would this be the place to put a method like this or should it be kept separate from the topic?

ethomson · 2013-12-16T14:51:41Z

Merging a binary file seems to actually merge the binary file (as in, <<<< Contents of first |||| Contents of second >>>>>>), should I write a test so this is replicate-able or put in an issue to the libgit2 project or what?

Indeed, that looks like a bug. An oversight on my part. You're welcome to file an issue in libgit2 for this, unfortunately I will not be able to take a look at this today.

Another issue seems to be that FastForwards don't appear to occur, the OID is generated and the flag is set in the merge result, but no changes are populated or staged, should I be calling a different method to git_merge?

This is the expected behavior. A fast-forward is not a merge, so we stop and tell you that you are eligible to fast-forward to that commit. At that point you just do a checkout.

Separately, while its possible (commenting on the CheckoutStrategy.GIT_CHECKOUT_USE_OURS indicates it and THEIRS are unimplemented but they work fine), the process to checkout ours/theirs on a conflict is a bit arduous

I'm not certain, to be honest; @nulltoken would have a better sense of how you should identify a path to checkout and specify which version of a file to checkout.

jamill · 2013-12-18T18:27:31Z

LibGit2Sharp.Tests/MergeFixture.cs

@@ -1,5 +1,5 @@
-using System.Linq;
-using LibGit2Sharp.Tests.TestHelpers;
+using LibGit2Sharp.Tests.TestHelpers;


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.

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

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

Put an implicit fast forward into the Merge method in Repository.cs, cleaned up usings/comments, removed GitMergeHead and made other suggested changes from github.

jamill · 2013-12-27T05:12:15Z

LibGit2Sharp/Repository.cs

+        /// </summary>
+        /// <param name="commit">The commit to use as a reference for the changes that should be merged into HEAD.</param>
+        /// <returns>The result of the performed merge <see cref="MergeResult"/>.</returns>
+        public MergeResult Merge(Commit commit)


I was thinking about I would expect this method to return... If this method is going to handle the committing of the merge, then maybe callers of this method would only be interested in what happened as a result of this method call - for instance, maybe returning an enum type with possible values such as of UpToDate, NormalMerge, FastForwardMerge, and Conflict would be sufficient. Maybe there are other use cases as well...

At some point, I can see this method getting a MergeOptions parameter as well, to control behaviors such as the type of acceptable merges (Normal only, Fast Forward only), whether to actually commit the merge or not, etc... Not needed at this time, but might be nice in the future.

jamill · 2013-12-27T05:38:08Z

I think these changes are looking good, and I am very excited for this functionality to be included in LibGit2Sharp as well! Thanks!

jamill · 2014-01-06T22:07:53Z

LibGit2Sharp/Core/GitMergeTreeOpts.cs

+namespace LibGit2Sharp.Core
+{
+    [Flags]
+    internal enum MergeTreeFlags


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

jamill · 2014-01-08T03:48:27Z

@CrumblyCake - I have been thinking about merge a bit more and made a commit to capture some of my thoughts and things I wanted to try out at: e2523bb. How do you feel about the direction of those changes?

That series of commits were also rebased on top of latest vNext, in order to get some "fixes" for tests that have recently started to fail.

/cc @nulltoken

nulltoken · 2014-01-10T21:02:44Z

@jamill Wow! Thanks a lot for giving a hand here!

@CrumblyCake How do you feel about @jamill's proposed changes?

nulltoken · 2014-01-13T21:05:51Z

@ethomson Could you please glance at @jamill's spike, my dearest MergeMaster™?

angusbjones · 2014-01-14T09:51:39Z

@jamill & @nulltoken -
I feel very good about them, everything looks to be a lot more readable as well as fitting better with the library, it also looks as if you cleared all the points you raised @jamill so thanks for that ;)
I'm sorry I didn't get to them sooner, my last 1.5 weeks have been a bit more eventful than my usual. There doesn't seem to be any branch against this commit, is there a way I can merge it so your indicated as the author?
The one thing I'd bring up is I liked your point at #579 (comment) (about checking if there is an operation running) and it doesn't appear to be in this commit, I understand why as there hasn't been a response to the comment yet so I figured I'd mention it again just in case it was missed? @ethomson @nulltoken

nulltoken · 2014-01-16T18:52:07Z

Closed as #608 has been merged

Introduce basic merging

2baf633

nulltoken reviewed Dec 5, 2013
View reviewed changes

jamill reviewed Dec 5, 2013
View reviewed changes

angusbjones added 2 commits December 7, 2013 00:07

MergeFixture: Added in recommended asserts

7f21005

MergeResult: Fixed up bad reference + added FastForwardOid prop Repository: Wrapped MergeHead and MergeResult handles in usings.

nulltoken reviewed Dec 11, 2013
View reviewed changes

jamill reviewed Dec 18, 2013
View reviewed changes

Introduced suggested GitHub changes. +Added more tests.

3c96d22

Put an implicit fast forward into the Merge method in Repository.cs, cleaned up usings/comments, removed GitMergeHead and made other suggested changes from github.

jamill reviewed Dec 27, 2013
View reviewed changes

jamill reviewed Jan 6, 2014
View reviewed changes

jamill mentioned this pull request Jan 15, 2014

Merge #608

Merged

nulltoken closed this Jan 16, 2014


		repo.MergeOnto(repo.Branches["FirstBranch"].Tip);

		repo.Commit("Merge First+Second");

Introduce basic merging #579

Introduce basic merging #579

Uh oh!

Conversation

angusbjones commented Dec 5, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethomson commented Dec 5, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nulltoken commented Dec 5, 2013

Uh oh!

nulltoken commented Dec 5, 2013

Uh oh!

ethomson commented Dec 5, 2013

Debug info from gdb:

used by your application.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dahlbyk commented Dec 5, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nulltoken commented Dec 11, 2013

Uh oh!

angusbjones commented Dec 15, 2013

Uh oh!

ethomson commented Dec 16, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamill commented Dec 27, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamill commented Jan 8, 2014

Uh oh!

nulltoken commented Jan 10, 2014

Uh oh!

nulltoken commented Jan 13, 2014

Uh oh!

angusbjones commented Jan 14, 2014

Uh oh!

nulltoken commented Jan 16, 2014

Uh oh!

Uh oh!