libgit2 / objective-git

Objective-C bindings to libgit2
MIT License
1.16k stars 280 forks source link

Git notes #576

Closed slavikus closed 7 years ago

slavikus commented 8 years ago

Adding support for Git Notes already supported in libgit2. A new class, GTNote has been added, along with a few methods for GTRepository to create and remove notes, as well as push them to remotes.

tiennou commented 8 years ago

Review complete. There are a few "open questions" here but apart from that and a few style points, it looks good, thanks !

slavikus commented 8 years ago

Submitted the fixes per @tiennou feedback.

pietbrauer commented 8 years ago

Closed and reopened for Travis to pick it up again.

pietbrauer commented 8 years ago

@slavikus Could you rebase this onto master, somehow the tests failed but master seems to be stable so far.

slavikus commented 8 years ago

There you go, @pietbrauer :)

pietbrauer commented 8 years ago

There is an open question by @tiennou https://github.com/libgit2/objective-git/pull/576#discussion_r63961775.

@tiennou can decide when to merge, tests are green now.

slavikus commented 8 years ago

@tiennou any insight whether this pull request should be accepted or not? :)

tiennou commented 8 years ago

Sorry, I've put my styleguide-master hat on :wink:. There are a few *-spacing issues I didn't explicitly comment on though. And the new -pushBranches:...withNotesReferenceName: I'm not fond of.

@pietbrauer I feel the responsibility 😨. I'm concerned about the libgit2 "bump" though, but I'm confused because the merge is still green which I did not expect. Current master tracks 0.24.1, but it wasn't at the time, hence the 1st commit.

slavikus commented 8 years ago

Wow, @tiennou , thanks for taking your time to review this. I'll fix the style stuff tomorrow, and will wait for the pushBranches decision. After some thought, maybe options is the way to go, yep.

I'll also try and merge with the most recent stuff in master.

slavikus commented 8 years ago

@tiennou nudge nudge? :)

tiennou commented 8 years ago

There's a few things left, and a few things I missed the first time around.

tiennou commented 8 years ago

Also, does the PR really depend on the libgit2 bump ? I'm still concerned about that...

@pietbrauer Insight needed please !

pietbrauer commented 8 years ago

Does not seem necessary to me.

slavikus commented 7 years ago

@tiennou I appreciate the time and effort you put into this, can you please check the changes? :)

tiennou commented 7 years ago

A few minor comments still.

I'm slightly concerned about that lookup/use-oid issue there seem to be, but I don't feel my git-fu is sufficient to tell whether it's right or wrong to not use (arguably, libgit2 has no API for grabbing a note by its OID directly)... For the record, it seems notes are just blobs, so a GTObject-lookup for that would do, but that's just a hunch.

slavikus commented 7 years ago

Sadly, there's no API in libgit2 to grab by note's OID directly, so that's why we have to go the other way 'round.

Fixed most other notes and nitpicks, aside for the whitespace issue I don't see. :)

slavikus commented 7 years ago

Travis build for macOS seems to be failing for some reason beyond the code of this pull request.

tiennou commented 7 years ago

A local run of the tests (both iOS & Mac) are green, so merging in. Thanks for the PR @slavikus !

slavikus commented 7 years ago

Thank you, appreciate your patience and feedback on this, guys!