-
Notifications
You must be signed in to change notification settings - Fork 280
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
Git notes #576
Git notes #576
Conversation
# Conflicts: # External/libgit2
Added `GTNote` class and a few helpers into `GTRepository`.
git_buf default_ref_name = { 0 }; | ||
int gitErr = git_note_default_ref(&default_ref_name, self.git_repository); | ||
if (gitErr != GIT_OK) { | ||
git_buf_free(&default_ref_name); |
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.
You're freeing on error, but not freeing on success here.
Review complete. There are a few "open questions" here but apart from that and a few style points, it looks good, thanks ! |
Submitted the fixes per @tiennou feedback. |
Added -[GTRepository pushBranches:toRemote:withOptions:withNotesReferenceName:error:progress:] to push both a set of branches and stored Git notes at the same operation to save on network ops.
Closed and reopened for Travis to pick it up again. |
@slavikus Could you rebase this onto master, somehow the tests failed but master seems to be stable so far. |
There you go, @pietbrauer :) |
Sorry, I've put my styleguide-master hat on 😉. There are a few *-spacing issues I didn't explicitly comment on though. And the new @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 |
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. |
@tiennou nudge nudge? :) |
int gitErr = git_note_read(&_note, repository, referenceName, oid); | ||
|
||
if (gitErr != GIT_OK) { | ||
if (error != NULL) *error = gitErr; |
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.
Well, I do not think it works that way 😉.
Actually, it does.
Can you make it less "unexpected" ? As in, not using int *
.
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.
Removed the error altogether. In all other parts of the framework initializers that take libgit2 pointers never return the error, only nil
on failure.
id object = [self initWithTargetGitOID:(git_oid *)oid.git_oid repository:repository.git_repository referenceName:referenceName.UTF8String error:&err]; | ||
|
||
if (err != GIT_OK && error != NULL) { | ||
*error = [NSError git_errorFor:err description:@"Failed to create a note."]; |
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.
Error message says "create" but method works as "read".
} | ||
|
||
- (BOOL)enumerateNotesWithReferenceName:(NSString *)referenceName error:(NSError **)error usingBlock:(void (^)(GTNote *note, GTObject *object, BOOL *stop))block | ||
{ |
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.
Style: we don't do the opening brace thing.
int iterError = GIT_OK; | ||
|
||
while ((iterError = git_note_next(¬e_id, &object_id, iter)) == GIT_OK) { | ||
NSError* lookupErr = nil; |
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.
Style: out of place *
(sorry, there's a bunch below too).
break; | ||
} | ||
|
||
GTObject* obj = [self lookUpObjectByGitOid:&object_id error:&lookupErr]; |
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.
Now that I've hit the shallow repository problem, this kind of "helpful" lookup thing makes me nervous...
See #590.
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.
For the records, I won't hold the PR up for that though 😉.
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.
I'll see how this can be reworked to be in sync with #590. But I'll submit a different pull request for that one.
There's a few things left, and a few things I missed the first time around.
|
Also, does the PR really depend on the @pietbrauer Insight needed please ! |
Does not seem necessary to me. |
Some properties could return nil where they are not annotated as such.
@tiennou I appreciate the time and effort you put into this, can you please check the changes? :) |
|
||
int gitErr = git_note_read(&_note, repository, referenceName, oid); | ||
|
||
if (gitErr != GIT_OK) return nil; |
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.
The "standard Cocoa way" is to return failure (e.g nil
or NO
), and additionally provide a helpful NSError
object that gives more context as to why the failure happened, through a NSError **
pointer that will be carefully provided by the developer (hint hint). Just want to point that out, because one of your recent comments seemed to imply we'd be doing otherwise (which I would IMHO consider a bug).
So the general idiom actually is :
- (id)randomInstanceMethod:(NSError **)error {
int gitErr = git_random_library_function(...);
if (gitErr != GIT_OK) {
if (error != NULL) *error = [NSError git_errorFor:gitErr description:@"Short message used by the final modal dialog that will pop up"];
return nil;
}
}
Also, it's better if your designated inits just call through to a designated one. More specifically, -initWithTargetGitOID:(git_oid *)oid...
(this method) is considered the "designated initializer" (eg. calls [super init...];
, and -initWithTargetOID:(GTOID *)OID...
is a convenience initializer that merely unwraps our objects.
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.
Good point.
int gitErr = git_note_default_ref(&default_ref_name, repository.git_repository); | ||
if (gitErr != GIT_OK) { | ||
if (error != NULL) *error = [NSError git_errorFor:gitErr description:@"Unable to get default git notes reference name"]; | ||
|
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.
Nitpick: whitespace.
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.
I'm blind. What's wrong with the whitespace above? Triple checked and don't see anything wrong. :\
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.
Sorry, I meant the newline is unwarranted.
if (default_ref_name.ptr != NULL) { | ||
noteRef = @(default_ref_name.ptr); | ||
} else { | ||
if (error != NULL) *error = [NSError git_errorFor:GIT_ERROR description:@"Unable to get default git notes reference name"]; |
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.
s/GIT_ERROR/gitErr
. GIT_ERROR
is one of libgit2 error classes, not the error code returned by git_note_default_ref
.
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.
But if gitErr is not GIT_OK, then it would return that error earlier. If we got to that point when git_note_default_ref
returned GIT_OK
, but default_ref_name.ptr
is NULL
for some reason, then I am generating a generic GIT_ERROR
. I know in reality it shouldn't happen, but I am being just extra cautious here.
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.
Good point, I got confused. Normally you would have to go through the hassle of setting up an NSError
manually, but since those cases are scarce in the codebase, I'm not against reusing GIT_ERROR
then.
int gitError = git_note_create(&oid, self.git_repository, referenceName.UTF8String, author.git_signature, committer.git_signature, theTarget.OID.git_oid, [note UTF8String], overwrite ? 1 : 0); | ||
if (gitError != GIT_OK) { | ||
if (error != NULL) *error = [NSError git_errorFor:gitError description:@"Failed to create a note in repository"]; | ||
|
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.
Nitpick: whitespace.
return nil; | ||
} | ||
|
||
return [[GTNote alloc] initWithTargetOID:theTarget.OID repository:self referenceName:referenceName error:error]; |
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.
I'm confused, wouldn't it be cleaner if you used oid
above instead of performing a lookup again ?
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.
The note is initialised using target's OID, not its own, so the lookup is justified there. Of course I could've used the designated initializer initWithTargetGitOID:...
instead, but it's a little difference anyway.
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.
I wasn't clear (also, do realise I have no prior knowledge of how notes actually work, apart from a quick glance at the docs that told me that notes are blobs 😉).
From what I can tell, the oid
above is the one that is created by git_note_create
, a.k.a the oid
of the blob that got added with the note contents as its data. git_note_read
does a re-lookup "from the start" — ie. grabs the ref, does "something", and populates the git_note
object we want to wrap.
int gitError = git_note_iterator_new(&iter, self.git_repository, referenceName.UTF8String); | ||
|
||
if (gitError != GIT_OK) | ||
{ |
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.
Nitpick: no newline before opening braces.
GTNote *note = [[GTNote alloc] initWithTargetOID:[GTOID oidWithGitOid:&object_id] repository:self referenceName:referenceName error:error]; | ||
if (note == nil) return NO; | ||
|
||
GTObject *obj = [self lookUpObjectByGitOid:&object_id error:&lookupErr]; |
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.
One way of being #590-friendly would be to add an NSError *
parameter to the block, and when the lookup fails, just send nil
down the block along with it, so we don't completely fail the enumeration.
} | ||
} | ||
|
||
if (iterError != GIT_OK && iterError != GIT_ITEROVER && error != NULL) *error = [NSError git_errorFor:iterError description:@"Iterator error"]; |
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.
Nitpick: For consistency, I'd really prefer that to be :
if (/* errorCondition */) {
if (error) *error = [NSError ...];
return NO;
}
F964D5F51CE9D9B200F1D8DD /* GTNote.m in Sources */ = {isa = PBXBuildFile; fileRef = F964D5F01CE9D9B200F1D8DD /* GTNote.m */; }; | ||
F9D1D4231CEB79D1009E5855 /* GTNoteSpec.m in Resources */ = {isa = PBXBuildFile; fileRef = F9D1D4221CEB79D1009E5855 /* GTNoteSpec.m */; }; | ||
F9D1D4241CEB79D1009E5855 /* GTNoteSpec.m in Resources */ = {isa = PBXBuildFile; fileRef = F9D1D4221CEB79D1009E5855 /* GTNoteSpec.m */; }; | ||
F9D1D4251CEB7BA6009E5855 /* GTNoteSpec.m in Sources */ = {isa = PBXBuildFile; fileRef = F9D1D4221CEB79D1009E5855 /* GTNoteSpec.m */; }; |
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.
Generic Xcode confusion 😆.
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.
Ack. :P
GTSignature* sig = [repository userSignatureForNow]; | ||
expect(sig).notTo(beNil()); | ||
|
||
NSError* err = nil; |
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.
Nitpick: misplaced *
(also on GTSignature
above), and NSMutableArray
below.
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. |
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. :) |
Travis build for macOS seems to be failing for some reason beyond the code of this pull request. |
A local run of the tests (both iOS & Mac) are green, so merging in. Thanks for the PR @slavikus ! |
Thank you, appreciate your patience and feedback on this, guys! |
Adding support for Git Notes already supported in libgit2. A new class,
GTNote
has been added, along with a few methods forGTRepository
to create and remove notes, as well as push them to remotes.