Pull #464

phatblat · 2015-06-17T19:25:01Z

Initial Pull implementation preferring fast-forward merge if possible or a "normal" merge if branches have diverged, depending on the merge analysis. Pull will fail if a conflict is detected.

TODO

Fast-forward merge
Normal merge
Merge conflict communication to caller (via error)
Tests

Out of scope

Pull with rebase
Explicit merge commit (aka --no-ff)
Prevent merge commit (aka --ff-only)
Pull an unborn branch

pietbrauer · 2015-06-17T20:08:06Z

Slightly more elegant than what I had: https://gist.github.com/pietbrauer/b1b3d2c6bd927ac85094

Things with open question marks for me:

How to fast forward (shell out to libgit2?) to avoid merge commit
~~How to support rebase (I guess libgit2 needs rebase support first?)~~
How to deal with merge conflicts (or how to communicate them back to the caller)

pietbrauer · 2015-06-17T20:40:09Z

I also end up with a dirty working copy when trying to pull in 2 commits from the remote:

Phone is at pietbrauer/git2go-test-repo@02ed2a3
Remote is at pietbrauer/git2go-test-repo@7c17c58

diff --git a/Test.txt b/Test.txt
index 5948f52..34f55f5 100644
--- a/Test.txt
+++ b/Test.txt
@@ -26,5 +26,3 @@ Test 25
 Test 26
 Test 27
 Test 28
-Test 29
-Test 30

Can you confirm?

palmin · 2015-06-17T22:03:37Z

My insights are for raw libgit2 but I will outline how I merge and perhaps this can be applied in objective-git.

For me git_merge_analysis is key to getting good merges in the second step of a push.

I start out getting merge heads for HEAD and corresponding head on remote tracking branch. These merge heads are git_annotated_commit in libgit2. Then I use git_merge_analysis to determine if fast-forward is possible, if HEAD is unborn (without any previous commits) or if we need to perform a regular merge.

fast-forward and unborn HEAD is fixed by pointing to the head commit of the remote tracking branch with git_reference_set_target but the real work lies in regular merge of course.

The regular merge changes the tree and I use git_index_has_conflicts to check for conflicts. When there are none I can simply make the merge commit having the old HEAD and remote tracking head commits as parents.

If there are conflicts these need to be resolved by the user somehow and git_index_remove_bypath followed by git_index_add_bypath is used to mark a file as resolved and when everything is resolved a commit can proceed.

Hope this helps in some small way.

Working Copy does not currently do rebase but it should be a matter of applying git_cherrypick on the commits that the remote tracking branch is ahead.

pietbrauer · 2015-06-18T14:23:45Z

@palmin Thanks so much for sharing your insights!

@phatblat I added GitMergeAnalysis support and will try to add support for Fast-Forward.

tiennou · 2015-06-18T14:51:45Z

ObjectiveGit/GTRepository+Pull.m

+
+	// Check if merge is necessary
+	GTBranch *localBranch = [repo currentBranchWithError:error];
+	if (*error) {


I think you should be checking for the return value, not the error — I fear a nice SEGV if I was to pass NULL here. There are other cases below.

Good catch. Using the user-supplied error pointer for control flow is messy, and dangerous 💥 .

phatblat · 2015-06-18T15:06:55Z

Wow, great info @palmin! Thanks!

pietbrauer · 2015-06-18T15:08:54Z

@phatblat I just added a draft for unborn and fast forward merges. Seems to work. ~~Although I end up with staged changes~~, the history seems to be the same as on the remote. success_kid.gif

palmin · 2015-06-18T15:43:51Z

Regarding changes after using git_reference_set_target to fast-forward:
I need to use git_checkout_head to get the working copy in order.

pietbrauer · 2015-06-18T15:51:20Z

@palmin Thanks once again. Done so in 1a2fd36

This stays consistent with the project. See https://github.com/libgit2/objective-git/blob/master/ObjectiveGit/GTRepository.m#L173 for example.

phatblat · 2015-06-18T23:41:41Z

@pietbrauer did you create a new referenceByUpdatingTarget:committer:message:error: method on GTReference? The logic after analyseMerge gets called is looking for that method and not finding it.

phatblat · 2015-06-18T23:43:03Z

If libgit2 doesn't support rebase then I think it's clear that's out of scope for this PR.

pietbrauer · 2015-06-19T05:50:40Z

@phatblat Ups, looks like I used objective-git ~> 0.6.2 in my project. I updated the implementation in 0bd6b02

What else is missing now?

targetCommitAndReturnError: is deprecated

pietbrauer · 2015-07-21T16:24:35Z

@phatblat The core problem is that travis aborts builds if they take longer than 10 minutes and produce no output. They increased it recently (that is why we added Travis again).

One possible workaround would be to trigger ./script/update_libssh2 manually before starting xctool. The reason is because the scripts produce output while running but xctool is omitting the output, until the script finished (which is after about 11 minutes).

phatblat · 2015-07-21T16:28:32Z

Ah. Those library build scripts could also be changed to inline the output instead of dumping it into architecture-specific log files (perhaps with an optional verbose flag).

Those log files have been troubling me since they make it hard to watch the build as it runs. Also, libgit2 build outputs hundreds of warnings for the 32-bit arm architectures (not sure how big an issue that is), but that's not easily visible since it's all tucked away out of sight. I realize there's a lot of output from those builds, but Xcode hides it all as soon as that build step completes successfully.

phatblat · 2015-08-08T02:00:43Z

Merged a handful of changes from master mostly just to kick off another build

modocache · 2015-08-09T21:52:28Z

ObjectiveGitTests/GTRepository+PullSpec.m

+		});
+
+		/// Conflict During Merge
+		it(@"fails to merge whene there is a conflict", ^{


s/whene/when/g

phatblat · 2015-08-09T22:42:28Z

Oops. Thx @modocache!

I guess it wasn't the 🔥 that was causing the Failed to query the list of test cases in the test bundle: Error while parsing JSON: The data is not in the correct format. error on Travis. It's got to be something in the tests in the /pull branch because our other PRs aren't having this problem.

phatblat · 2015-08-24T15:51:16Z

The build for this PR is finally green! Turns out disabling a test using pending causes Travis to choke.

Ready for review.

joshaber · 2015-09-08T14:48:58Z

ObjectiveGitTests/GTRepository+PullSpec.m

+			});
+		});
+
+/* pending tests break build on travis


joshaber · 2015-09-08T14:49:50Z

Sorry I'm a scumbag and waited too long to review this 😞. It looks good other than needing to be updated.

# Conflicts: # ObjectiveGitFramework.xcodeproj/project.pbxproj

phatblat · 2015-09-08T23:28:15Z

No worries! I've updated from master. I'd like to request a pull of the Pull pull request. 😆

joshaber · 2015-09-09T14:07:54Z

I'd like to request a pull of the Pull pull request.

joshaber · 2015-09-09T14:07:58Z

🤘

Pull

dleehr · 2015-09-09T14:09:49Z

Awesome! 🎉

Initial pull implementation

4730e27

pietbrauer added 2 commits June 18, 2015 16:20

Add GitMergeAnalysis handling

4f058f4

Convert GTRepository+Pull to tabs

5aae55a

tiennou reviewed Jun 18, 2015
View reviewed changes

Add fast-forward and unborn merge

f713326

pietbrauer added 2 commits June 18, 2015 17:50

Use GTMergeAnalysis as parameter instead of return type

c7b439a

Checkout reference after updating HEAD

1a2fd36

pietbrauer and others added 4 commits June 18, 2015 17:56

Remove new lines and use one line only for long method calls

b510505

This stays consistent with the project. See https://github.com/libgit2/objective-git/blob/master/ObjectiveGit/GTRepository.m#L173 for example.

Make header public

ea66aa0

Also checkout after merge commit

d88ba4d

Use return values to determine when to bail out

2de17e1

Use referenceByUpdatingTarget:message:error

0bd6b02

Abandon pull if tracking branch cannot be found

18b86b2

pietbrauer mentioned this pull request Jun 23, 2015

Please provide an up to date "git pull" example libgit2/libgit2#3244

Open

phatblat added the enhancement label Jun 27, 2015

phatblat added 3 commits June 28, 2015 09:33

Merge branch 'master' into pull

25c8d87

Ignore Carthage/Build

b9c8b8e

Change calls to targetCommitWithError:

f8bd53e

targetCommitAndReturnError: is deprecated

joshaber mentioned this pull request Jul 21, 2015

Bump libgit2 #493

Merged

phatblat mentioned this pull request Jul 21, 2015

Enable verbose build output from iOS build scripts #494

Closed

phatblat added 7 commits July 22, 2015 19:26

Analyse -> Analyze

2333aaa

Dial down the indentation

f167004

Doc revision

d8bf6c2

Fix curly braces

1dd20ba

Explicit comparison to nil

6b4d2e0

Merge branch 'master' into pull

631df10

Merge branch 'master' into pull

faa5a92

🔥 the emoji from example name

04f1c4c

modocache reviewed Aug 9, 2015
View reviewed changes

ObjectiveGitTests/GTRepository+PullSpec.m

});

/// Conflict During Merge

it(@"fails to merge whene there is a conflict", ^{

Copy link

modocache Aug 9, 2015

Choose a reason for hiding this comment

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

s/whene/when/g

whene -> when

6dbc38f

Comment out pending test

4feb8c6

joshaber reviewed Sep 8, 2015
View reviewed changes

ObjectiveGitTests/GTRepository+PullSpec.m

});

});

/* pending tests break build on travis

Copy link

Member

joshaber Sep 8, 2015

Choose a reason for hiding this comment

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

lol 😭

Merge branch 'master' into pull

dae0b10

# Conflicts: # ObjectiveGitFramework.xcodeproj/project.pbxproj

joshaber added a commit that referenced this pull request Sep 9, 2015

Merge pull request #464 from libgit2/pull

90dacb6

Pull

joshaber merged commit 90dacb6 into master Sep 9, 2015

joshaber deleted the pull branch September 9, 2015 14:08

phatblat mentioned this pull request Sep 9, 2015

Remove "in progress" from readme #518

Merged

joshaber mentioned this pull request Apr 6, 2016

Fetch/push/pull atom/github#121

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pull #464

Pull #464

phatblat commented Jun 17, 2015

pietbrauer commented Jun 17, 2015

pietbrauer commented Jun 17, 2015

palmin commented Jun 17, 2015

pietbrauer commented Jun 18, 2015

tiennou Jun 18, 2015

phatblat Jun 18, 2015

phatblat commented Jun 18, 2015

pietbrauer commented Jun 18, 2015

palmin commented Jun 18, 2015

pietbrauer commented Jun 18, 2015

phatblat commented Jun 18, 2015

phatblat commented Jun 18, 2015

pietbrauer commented Jun 19, 2015

pietbrauer commented Jul 21, 2015

phatblat commented Jul 21, 2015

phatblat commented Aug 8, 2015

modocache Aug 9, 2015

phatblat commented Aug 9, 2015

phatblat commented Aug 24, 2015

joshaber Sep 8, 2015

joshaber commented Sep 8, 2015

phatblat commented Sep 8, 2015

joshaber commented Sep 9, 2015

joshaber commented Sep 9, 2015

dleehr commented Sep 9, 2015

Pull #464

Pull #464

Conversation

phatblat commented Jun 17, 2015

TODO

Out of scope

pietbrauer commented Jun 17, 2015

pietbrauer commented Jun 17, 2015

palmin commented Jun 17, 2015

pietbrauer commented Jun 18, 2015

tiennou Jun 18, 2015

Choose a reason for hiding this comment

phatblat Jun 18, 2015

Choose a reason for hiding this comment

phatblat commented Jun 18, 2015

pietbrauer commented Jun 18, 2015

palmin commented Jun 18, 2015

pietbrauer commented Jun 18, 2015

phatblat commented Jun 18, 2015

phatblat commented Jun 18, 2015

pietbrauer commented Jun 19, 2015

pietbrauer commented Jul 21, 2015

phatblat commented Jul 21, 2015

phatblat commented Aug 8, 2015

modocache Aug 9, 2015

Choose a reason for hiding this comment

phatblat commented Aug 9, 2015

phatblat commented Aug 24, 2015

joshaber Sep 8, 2015

Choose a reason for hiding this comment

joshaber commented Sep 8, 2015

phatblat commented Sep 8, 2015

joshaber commented Sep 9, 2015

joshaber commented Sep 9, 2015

dleehr commented Sep 9, 2015