Add Rugged::Repository#merge_base. #128

Merged
merged 6 commits into from Dec 14, 2012

Conversation

Projects
None yet
3 participants
Contributor

spraints commented Dec 11, 2012

No description provided.

@vmg vmg commented on an outdated diff Dec 11, 2012

ext/rugged/rugged_repo.c
@@ -830,6 +858,8 @@ void Init_rugged_repo()
rb_define_method(rb_cRuggedRepo, "head_detached?", rb_git_repo_head_detached, 0);
rb_define_method(rb_cRuggedRepo, "head_orphan?", rb_git_repo_head_orphan, 0);
+ rb_define_method(rb_cRuggedRepo, "merge_base", rb_git_repo_merge_base, 2);
@vmg

vmg Dec 11, 2012

Owner

Wrong indentation!

@vmg vmg and 1 other commented on an outdated diff Dec 11, 2012

ext/rugged/rugged_repo.c
@@ -323,6 +323,34 @@ static VALUE rb_git_repo_get_config(VALUE self)
/*
* call-seq:
+ * repo.merge_base(oid1, oid2)
+ *
+ * Find a merge base between two commits.
+ */
+static VALUE rb_git_repo_merge_base(VALUE self, VALUE hex_oid1, VALUE hex_oid2)
+{
+ int error;
+ git_oid oid1, oid2, base;
+ git_repository *repo;
+
+ Data_Get_Struct(self, git_repository, repo);
+
+ Check_Type(hex_oid1, T_STRING);
+ error = git_oid_fromstr(&oid1, StringValueCStr(hex_oid1));
@vmg

vmg Dec 11, 2012

Owner

This is Ruby! We need to be more lenient here. Ideally, you should be able to pass a Rugged::Commit object and the function will work properly too (i.e. by getting the OID of the object and using that in git_merge_base).

@spraints

spraints Dec 12, 2012

Contributor

Definitely. Would you be interested in a PR that made more of rugged flexible about its inputs?

@vmg

vmg Dec 12, 2012

Owner

I think we are quite flexible right now (i.e. any place that takes an OID also takes an object, and so on). But besides fixing this specific case here, if you find any other cases where we're being to stringent with types, I'd love to see that PR!

@spraints spraints and 1 other commented on an outdated diff Dec 12, 2012

lib/rugged/repository.rb
@@ -162,5 +162,17 @@ def blob_at(revision, path)
blob = Rugged::Blob.lookup(self, blob_data[:oid])
(blob.type == :blob) ? blob : nil
end
+
+ # Get the oid of a merge base for two commits.
+ #
+ # commit1 - A commit, ref, or oid.
+ # commit2 - Another commit, ref, or oid.
+ #
+ # Returns the oid of the base commit.
+ def merge_base(commit1, commit2)
+ commit1 = commit1.is_a?(Commit) ? commit1.oid : rev_parse_oid(commit1)
+ commit2 = commit2.is_a?(Commit) ? commit2.oid : rev_parse_oid(commit2)
+ merge_base_oid(commit1, commit2)
+ end
@spraints

spraints Dec 12, 2012

Contributor

Is Rugged::Repository the best place for #merge_base? I didn't see a better place, but it seems like the git_THING_FUNC functions generally get mapped to Rugged::THING#func, so I could see putting it in a new module Rugged::Merge. I'm happy with it here, but I assume you've got a better long-term perspective on this.

@vmg

vmg Dec 12, 2012

Owner

Yeah, merge_base is kind of the odd-one out. I think it fits here just fine.

@vmg

vmg Dec 12, 2012

Owner

One last nitpick: could you rename merge_base_oid to _merge_base or the likes, just so to let people know you're supposed to use the external wrapper?

Contributor

spraints commented Dec 13, 2012

Does this look good? Hopefully I used tabs in the right places. 😉

@arthurschreiber arthurschreiber and 2 others commented on an outdated diff Dec 13, 2012

ext/rugged/rugged_repo.c
@@ -830,6 +858,8 @@ void Init_rugged_repo()
rb_define_method(rb_cRuggedRepo, "head_detached?", rb_git_repo_head_detached, 0);
rb_define_method(rb_cRuggedRepo, "head_orphan?", rb_git_repo_head_orphan, 0);
+ rb_define_private_method(rb_cRuggedRepo, "_merge_base", rb_git_repo_merge_base_oid, 2);
@arthurschreiber

arthurschreiber Dec 13, 2012

Member

I'm not sure, but I don't like this magic underscore method. The functionality inside the actual merge_base method is simple enough to be pulled into the c code.

@vmg

vmg Dec 13, 2012

Owner

In the interest of full disclosure, I was going to rewrite the functionality in merge_base in C. I just didn't want to be exceedingly nitpicky to @spraints on his first Rugged PR. :)

@spraints

spraints Dec 14, 2012

Contributor

No worries. Ruby is more familiar to me now than C, but I rewrote in C. Hopefully it's not too offensive to your eyes.

Contributor

spraints commented Dec 14, 2012

If you want to refactor / rewrite / whatever, I'd like to see what you change, so I can make less work for you next time.

@vmg vmg added a commit that referenced this pull request Dec 14, 2012

@vmg vmg Merge pull request #128 from spraints/merge-base
Add Rugged::Repository#merge_base.
8138988

@vmg vmg merged commit 8138988 into libgit2:development Dec 14, 2012

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment