-
Notifications
You must be signed in to change notification settings - Fork 56
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
Update libgit2 to 0.19.0 #56
Conversation
Opening this early so I can get some feedback. I've done most of the work to get gitteh compiling again, but I haven't actually been able to run the tests yet, so none of the api changes have made it into the coffeescript source. |
👍 Great work! Hey, what about the And since |
@@ -56,7 +56,7 @@ async.series([ | |||
envpassthru("mkdir", "-p", buildDir, cb); | |||
}, | |||
function(cb) { | |||
envpassthru("cmake", "-DTHREADSAFE=1", "-DBUILD_CLAR=0", "..", { | |||
envpassthru("cmake", "-DCMAKE_C_FLAGS='-fPIC'", "-DTHREADSAFE=1", "-DBUILD_CLAR=0", "..", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmendeth, this forces cmake to include the -fPIC
flag when building. We might have to revise that when we do Windows support, but this should fix it for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that will do it for now.
Travis is 💚! I'd like some more eyes on the changes to refspecs and progress notifications, but otherwise I think this is good to go. |
@ben Just reviewed and it all looks rather sexcellent actually. Awesome work! I'm a little confused though - I tried running the tests locally and I'm getting a few fails. Also the merge commit that Travis built (dd6d82e) I can't even find in my local repo, though I can see the HEAD of this PR (a3e10bb)... O.o |
Oh jesus I forgot to run |
@ben as you said there's some issues with the JS api now (particularly around the refspec changes), and these unfortunately aren't being picked up by the tests (possibly because there is none yet for remotes ... derp). I'm going to merge this in now and fix up the JS API. |
Good point – I was just making sure the tests passed, I didn't try to fix up anything that wasn't covered by them. 😊 |
Haha yeah it's totoally my bad not having better coverage. I'm going to rectify that as I update the broken parts of the API ;) |
quick question: how different is 0.19's API from 0.17? I'm still in the middle of upgrading to 0.17, and if it's significantly different, I might as well just move all the way to 0.19 |
Pretty different. Here's the quick bullet list from 0.18.0:
And here's the list for v0.19.0:
And a quick function-level changelist. We're trying to get all of the breaking changes done before marking a 1.0 release, so lots of stuff has happened. |
Thanks for the info. I should have been a little clearer with my question, since the version numbers for gitteh are close to the version numbers for libgit2. Will the API for gitteh change significantly from 0.17 to 0.19? |
@iamwilhelm it will change, as is expected when the major version component is still 0. That said however, the API for gitteh is pretty far removed from libgit2 itself, so things like naming changes won't affect Gitteh API so much, and added features will generally not cause existing API changes either. That said the move to gitteh version |
🚧👷 WIP DO NOT MERGE 👷🚧
Libgit2 recently released v0.19.0, and there are a number of improvements that node-gitteh could take advantage of (see #8).
However, there are also a number of API changes that need to be adjusted for. In some cases this will probably mean changing the gitteh API as well.
A quick summary:
GIT_DIR_*
macros were changed into an enum, and their names changed.git_indexer_stats
is now calledgit_transfer_progress
, and its fields have changed names and semantics._oid
now end with_id
git_submodule_foreach
now take agit_submodule*
as a parameter. We may be able to do better insideRepository::SubmoduleListCallback
with this, but for now I've just added the parameter to the signature.