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

merge! #1007

Merged
merged 1 commit into from Nov 5, 2013

Conversation

Projects
None yet
4 participants
@ethomson
Copy link
Member

ethomson commented Oct 22, 2012

This merges a branch and puts the results in the workdir!

src/diff.c Outdated
(diff = a->file_size - b->file_size) == 0 &&
(diff = a->flags - b->flags) == 0 &&
(diff = a->flags_extended - b->flags_extended) == 0) {
diff = git_oid_cmp(&a->oid, &b->oid);

This comment has been minimized.

@vmg

vmg Oct 22, 2012

Member

By the looks of it, I think a single memcmp over the whole struct would be faster -- regardless of whether there are unset fields or not.

@vmg

This comment has been minimized.

Copy link
Member

vmg commented Oct 22, 2012

Gah, this is a lot of code. @arrbee, to the review-mobile.

@ethomson

This comment has been minimized.

Copy link
Member

ethomson commented Oct 23, 2012

@vmg Yeah, this is stupidly large. Let me break some of this stuff out into separate PRs.

@ethomson

This comment has been minimized.

Copy link
Member

ethomson commented Dec 12, 2012

So, here's what's still missing:

  • There's some hacky stuff that needs to get cleaned up (see TODOs)
  • - xxdiff and writing conflicting files does not respect EOL settings
  • - No detection for dirty index/workdir
  • - I want to spike dumping diff_tree and merging the results of two normal diffs. I thought a function like unpack_trees() would be useful, but I'm now less convinced
  • - No rename detection, I've done a few spikes that aren't integrated here yet
  • - The merge_opts / merge_tree_opts stuff is terrible. I'm thinking that there should be no high-level git_merge() command and callers should call git_merge_setup() / do the diff / deal with conflicts themselves. Need some feedback on this.
  • - Fastforward check is wrong (way too naive.)
  • - Can't merge two commits that don't have a common ancestor
GIT_CHECKOUT_SKIP_UNMERGED = (1u << 10),
/** For unmerged files, checkout stage 2 from index (NOT IMPLEMENTED) */
GIT_CHECKOUT_USE_OURS = (1u << 11),
/** For unmerged files, checkout stage 3 from index (NOT IMPLEMENTED) */

This comment has been minimized.

@arrbee

arrbee Jul 3, 2013

Member

I'm guessing you want to remove the (NOT IMPLEMENTED) comment? ⚡️

GIT_CHECKOUT_USE_OURS = (1u << 11),
/** For unmerged files, checkout stage 3 from index (NOT IMPLEMENTED) */
GIT_CHECKOUT_USE_THEIRS = (1u << 12),

This comment has been minimized.

@arrbee

arrbee Jul 3, 2013

Member

Also, are there other options we should have here? I'm always wary of options that were created unimplemented - they should be revisited when implemented.

This comment has been minimized.

@ethomson

ethomson Jul 3, 2013

Member

Oops!

As for other options? I don't think so; at least not at the moment. (Amusingly, I had duplicated these constants before I realized that they already existed. So actually you were really on top of this.)

I think that we'll want to add some more that drive the merge engine (prefer ours / prefer theirs for line-level conflicts) which are (in this PR anyway) defined as flags to git_merge. They probably belong in checkout opts, even though git-checkout wouldn't expose these options. But I'm not entirely certain yet.

@ethomson

This comment has been minimized.

Copy link
Member

ethomson commented Nov 5, 2013

Rebased!

@AnuiCuda

This comment has been minimized.

Copy link

AnuiCuda commented Nov 5, 2013

What's the status of this pull request? When can we expect it to hit the development branch?

@ethomson

This comment has been minimized.

Copy link
Member

ethomson commented Nov 5, 2013

@AnuiCuda Thanks for your interest. This is ready for code review and will need updates based on that review. Getting close!

@AnuiCuda

This comment has been minimized.

Copy link

AnuiCuda commented Nov 5, 2013

Sweet! Can't wait for this! I was about to go dig into the code to figure out what needed to be done when I tripped into this request. Nice work @ethomson!

@vmg

This comment has been minimized.

Copy link
Member

vmg commented Nov 5, 2013

@ethomson: Looks like we're having umask issues again...

@ethomson

This comment has been minimized.

Copy link
Member

ethomson commented Nov 5, 2013

Indeed. #1940 has the fix.

On Nov 5, 2013, at 5:58 AM, Vicent Martí notifications@github.com wrote:

@ethomson: Looks like we're having umask issues again...


Reply to this email directly or view it on GitHub.

@vmg

This comment has been minimized.

Copy link
Member

vmg commented Nov 5, 2013

Sounds good. Let's go for the rebase then. :)

@ethomson

This comment has been minimized.

Copy link
Member

ethomson commented Nov 5, 2013

@vmg Yup, rebased!

@vmg

This comment has been minimized.

Copy link
Member

vmg commented Nov 5, 2013

What could possibly go wrong.

vmg added a commit that referenced this pull request Nov 5, 2013

@vmg vmg merged commit 453e6b3 into libgit2:development Nov 5, 2013

1 check passed

default The Travis CI build passed
Details

phatblat pushed a commit to phatblat/libgit2 that referenced this pull request Sep 13, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment