Git notes #576

slavikus · 2016-05-19T15:28:14Z

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.

# Conflicts: # External/libgit2

Added `GTNote` class and a few helpers into `GTRepository`.

tiennou · 2016-05-19T21:53:16Z

ObjectiveGit/GTRepository+RemoteOperations.m

+		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);


You're freeing on error, but not freeing on success here.

tiennou · 2016-05-19T22:24:30Z

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

@tiennou

Some stylistic and other fixes based on @tiennou feedback on the pull request.

slavikus · 2016-05-20T11:01:47Z

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.

pietbrauer · 2016-07-11T15:07:26Z

Closed and reopened for Travis to pick it up again.

pietbrauer · 2016-07-13T15:18:57Z

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

slavikus · 2016-07-15T14:04:06Z

There you go, @pietbrauer :)

tiennou · 2016-09-06T20:23:31Z

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 -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 · 2016-09-06T20:26:32Z

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 · 2016-09-27T14:09:59Z

@tiennou nudge nudge? :)

tiennou · 2016-09-27T17:22:35Z

ObjectiveGit/GTNote.m

+	int gitErr = git_note_read(&_note, repository, referenceName, oid);
+
+	if (gitErr != GIT_OK) {
+		if (error != NULL) *error = gitErr;


Well, I do not think it works that way 😉.

Actually, it does.

Can you make it less "unexpected" ? As in, not using int *.

Removed the error altogether. In all other parts of the framework initializers that take libgit2 pointers never return the error, only nil on failure.

tiennou · 2016-09-27T17:23:07Z

ObjectiveGit/GTNote.m

+	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."];


Error message says "create" but method works as "read".

tiennou · 2016-09-27T17:32:04Z

ObjectiveGit/GTRepository.m

+}
+
+- (BOOL)enumerateNotesWithReferenceName:(NSString *)referenceName error:(NSError **)error usingBlock:(void (^)(GTNote *note, GTObject *object, BOOL *stop))block
+{


Style: we don't do the opening brace thing.

tiennou · 2016-09-27T17:33:00Z

ObjectiveGit/GTRepository.m

+	int iterError = GIT_OK;
+
+	while ((iterError = git_note_next(&note_id, &object_id, iter)) == GIT_OK) {
+		NSError* lookupErr = nil;


Style: out of place * (sorry, there's a bunch below too).

tiennou · 2016-09-27T17:35:15Z

ObjectiveGit/GTRepository.m

+			break;
+		}
+
+		GTObject* obj = [self lookUpObjectByGitOid:&object_id error:&lookupErr];


Now that I've hit the shallow repository problem, this kind of "helpful" lookup thing makes me nervous...

See #590.

For the records, I won't hold the PR up for that though 😉.

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.

tiennou · 2016-09-27T17:48:01Z

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

A test exercising note-pushing might be nice. I was wondering what would happen in case I specified a non-existent note-ref name.
We still don't have a clear answer on the pushBranches thing, and I'd like some feedback from someone else. My preference would be for a new GTRepositoryRemoteOptionsPushNotes, with either a @(BOOL) which pushes the default note-ref, of an array of reference names.

tiennou · 2016-09-27T17:50:50Z

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

@pietbrauer Insight needed please !

pietbrauer · 2016-09-27T17:57:52Z

Does not seem necessary to me.

Some properties could return nil where they are not annotated as such.

slavikus · 2016-12-02T15:28:24Z

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

tiennou · 2016-12-02T17:43:05Z

ObjectiveGit/GTNote.m

+
+	int gitErr = git_note_read(&_note, repository, referenceName, oid);
+
+	if (gitErr != GIT_OK) return nil;


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.

Good point.

tiennou · 2016-12-02T17:43:38Z

ObjectiveGit/GTNote.m

+	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"];
+


Nitpick: whitespace.

I'm blind. What's wrong with the whitespace above? Triple checked and don't see anything wrong. :\

Sorry, I meant the newline is unwarranted.

tiennou · 2016-12-02T17:49:50Z

ObjectiveGit/GTNote.m

+	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"];


s/GIT_ERROR/gitErr. GIT_ERROR is one of libgit2 error classes, not the error code returned by git_note_default_ref.

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.

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.

tiennou · 2016-12-02T17:58:24Z

ObjectiveGit/GTRepository.m

+	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"];
+


Nitpick: whitespace.

tiennou · 2016-12-02T18:02:44Z

ObjectiveGit/GTRepository.m

+		return nil;
+	}
+
+	return [[GTNote alloc] initWithTargetOID:theTarget.OID repository:self referenceName:referenceName error:error];


I'm confused, wouldn't it be cleaner if you used oid above instead of performing a lookup again ?

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.

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.

tiennou · 2016-12-02T18:03:52Z

ObjectiveGit/GTRepository.m

+	int gitError = git_note_iterator_new(&iter, self.git_repository, referenceName.UTF8String);
+
+	if (gitError != GIT_OK)
+	{


Nitpick: no newline before opening braces.

tiennou · 2016-12-02T18:07:57Z

ObjectiveGit/GTRepository.m

+		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];


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.

tiennou · 2016-12-02T18:10:13Z

ObjectiveGit/GTRepository.m

+		}
+	}
+
+	if (iterError != GIT_OK && iterError != GIT_ITEROVER && error != NULL) *error = [NSError git_errorFor:iterError description:@"Iterator error"];


Nitpick: For consistency, I'd really prefer that to be :

if (/* errorCondition */) { if (error) *error = [NSError ...]; return NO; }

tiennou · 2016-12-02T18:11:09Z

ObjectiveGitFramework.xcodeproj/project.pbxproj

+		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 */; };


Generic Xcode confusion 😆.

tiennou · 2016-12-02T18:12:29Z

ObjectiveGitTests/GTNoteSpec.m

+	GTSignature* sig = [repository userSignatureForNow];
+	expect(sig).notTo(beNil());
+
+	NSError* err = nil;


Nitpick: misplaced *(also on GTSignature above), and NSMutableArray below.

tiennou · 2016-12-02T18:18:34Z

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 · 2016-12-02T23:09:10Z

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 · 2016-12-04T17:13:22Z

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

tiennou · 2016-12-05T10:11:15Z

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

slavikus · 2016-12-05T11:48:46Z

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

slavikus added 5 commits March 23, 2016 14:20

Updating libgit2 pointer

abc1d74

Updating for new libgit2 git_remote_connect() call

41279e4

Merge remote-tracking branch 'upstream/master'

0871b0e

# Conflicts: # External/libgit2

Git notes support.

47dc0fc

Added `GTNote` class and a few helpers into `GTRepository`.

Added support for [GTRepository pushNotes:...]

fe13986

tiennou reviewed May 19, 2016
View reviewed changes

Fixes per @tiennou feedback

6df9faf

Some stylistic and other fixes based on @tiennou feedback on the pull request.

slavikus added 2 commits May 23, 2016 18:18

Push branches and notes in one operation

947058d

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.

Check if notes reference exists before pushing it

e99a62e

pietbrauer closed this Jul 11, 2016

pietbrauer reopened this Jul 11, 2016

slavikus added 2 commits July 15, 2016 16:24

Merge remote-tracking branch 'upstream/master'

b64f024

Merge branch 'master' into git-notes

0661bab

slavikus added 2 commits September 6, 2016 23:28

Merge remote-tracking branch 'upstream/master' into git-notes

c27e838

Style fixes per @tiennou feedback

73037a4

tiennou requested changes Sep 27, 2016

View reviewed changes

pietbrauer assigned tiennou and pietbrauer Oct 27, 2016

slavikus added 8 commits November 8, 2016 17:37

Merge remote-tracking branch 'upstream/master' into git-notes

75e5356

Merge remote-tracking branch 'upstream/master'

16ef587

Respect nullable returns in various methods

2352543

Some properties could return nil where they are not annotated as such.

Merge branch 'master' into git-notes

6e96e11

Merge remote-tracking branch 'upstream/master' into git-notes

6228da7

Fix nullability warning for GTNote

2fea0a5

Corrections per @tiennou feedback

5bf9d5f

Rollback libgit2 back to globally used release

f4a1103

tiennou requested changes Dec 2, 2016

View reviewed changes

More fixes for pull request libgit2#576

55898a7

slavikus added 2 commits December 3, 2016 02:13

Extra newlines removal

16f980f

Fix tests for GTNote

927cc49

tiennou merged commit 6dc2722 into libgit2:master Dec 5, 2016


		int gitErr = git_note_read(&_note, repository, referenceName, oid);

		if (gitErr != GIT_OK) return nil;

Git notes #576

Git notes #576

Conversation

slavikus commented May 19, 2016

Choose a reason for hiding this comment

tiennou commented May 19, 2016

slavikus commented May 20, 2016

pietbrauer commented Jul 11, 2016

pietbrauer commented Jul 13, 2016

slavikus commented Jul 15, 2016

tiennou commented Sep 6, 2016

slavikus commented Sep 6, 2016

slavikus commented Sep 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiennou commented Sep 27, 2016

tiennou commented Sep 27, 2016

pietbrauer commented Sep 27, 2016

slavikus commented Dec 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiennou commented Dec 2, 2016

slavikus commented Dec 2, 2016

slavikus commented Dec 4, 2016

tiennou commented Dec 5, 2016

slavikus commented Dec 5, 2016