Skip to content
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

Merged
merged 23 commits into from
Dec 5, 2016
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
abc1d74
Updating libgit2 pointer
slavikus Mar 23, 2016
41279e4
Updating for new libgit2 git_remote_connect() call
slavikus Mar 23, 2016
0871b0e
Merge remote-tracking branch 'upstream/master'
slavikus May 4, 2016
47dc0fc
Git notes support.
slavikus May 19, 2016
fe13986
Added support for [GTRepository pushNotes:...]
slavikus May 19, 2016
6df9faf
Fixes per @tiennou feedback
slavikus May 20, 2016
947058d
Push branches and notes in one operation
slavikus May 23, 2016
e99a62e
Check if notes reference exists before pushing it
slavikus May 26, 2016
b64f024
Merge remote-tracking branch 'upstream/master'
slavikus Jul 15, 2016
0661bab
Merge branch 'master' into git-notes
slavikus Jul 15, 2016
c27e838
Merge remote-tracking branch 'upstream/master' into git-notes
slavikus Sep 6, 2016
73037a4
Style fixes per @tiennou feedback
slavikus Sep 6, 2016
75e5356
Merge remote-tracking branch 'upstream/master' into git-notes
slavikus Nov 8, 2016
16ef587
Merge remote-tracking branch 'upstream/master'
slavikus Nov 23, 2016
2352543
Respect nullable returns in various methods
slavikus Nov 23, 2016
6e96e11
Merge branch 'master' into git-notes
slavikus Nov 23, 2016
6228da7
Merge remote-tracking branch 'upstream/master' into git-notes
slavikus Nov 30, 2016
2fea0a5
Fix nullability warning for GTNote
slavikus Dec 2, 2016
5bf9d5f
Corrections per @tiennou feedback
slavikus Dec 2, 2016
f4a1103
Rollback libgit2 back to globally used release
slavikus Dec 2, 2016
55898a7
More fixes for pull request #576
slavikus Dec 2, 2016
16f980f
Extra newlines removal
slavikus Dec 2, 2016
927cc49
Fix tests for GTNote
slavikus Dec 4, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion External/libgit2
Submodule libgit2 updated 54 files
+0 −3 .travis.yml
+24 −1 CHANGELOG.md
+1 −1 CMakeLists.txt
+16 −1 CONTRIBUTING.md
+1 −6 appveyor.yml
+2 −3 examples/general.c
+43 −0 include/git2/blob.h
+67 −0 include/git2/commit.h
+0 −8 include/git2/common.h
+10 −1 include/git2/merge.h
+47 −1 include/git2/odb.h
+177 −0 include/git2/sys/merge.h
+1 −3 script/appveyor-mingw.sh
+0 −13 script/toolchain-mingw32.cmake
+71 −39 src/blob.c
+192 −47 src/commit.c
+39 −16 src/config_file.c
+6 −1 src/filebuf.c
+1 −0 src/filebuf.h
+2 −0 src/global.c
+3 −11 src/ignore.c
+1 −0 src/indexer.c
+160 −63 src/merge.c
+61 −2 src/merge.h
+396 −0 src/merge_driver.c
+60 −0 src/merge_driver.h
+11 −49 src/merge_file.c
+187 −54 src/odb.c
+3 −0 src/pack.c
+0 −3 src/refs.c
+17 −61 src/transports/smart_protocol.c
+0 −2 src/tree.c
+1 −2 src/xdiff/xprepare.c
+0 −13 tests/attr/ignore.c
+139 −0 tests/commit/write.c
+1 −1 tests/config/multivar.c
+0 −27 tests/config/write.c
+0 −2 tests/core/array.c
+2 −6 tests/core/stream.c
+51 −9 tests/index/nsec.c
+388 −0 tests/merge/driver.c
+2 −2 tests/merge/workdir/dirty.c
+36 −0 tests/merge/workdir/simple.c
+0 −156 tests/object/blob/fromchunks.c
+103 −0 tests/object/blob/fromstream.c
+155 −0 tests/odb/mixed.c
+26 −5 tests/rebase/abort.c
+57 −0 tests/rebase/merge.c
+205 −0 tests/rebase/setup.c
+5 −5 tests/refs/create.c
+0 −8 tests/refs/lookup.c
+0 −5 tests/repo/iterator.c
+3 −3 tests/reset/hard.c
+1 −4 tests/status/worktree.c
63 changes: 63 additions & 0 deletions ObjectiveGit/GTNote.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
//
// GTNote.h
// ObjectiveGitFramework
//
// Created by Slava Karpenko on 5/16/2016.
//
// The MIT License
//
// Copyright (c) 2016 Wildbit LLC
//
// Permission is hereby granted, free of charge, to any person obtaining a copy
// of this software and associated documentation files (the "Software"), to deal
// in the Software without restriction, including without limitation the rights
// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
// copies of the Software, and to permit persons to whom the Software is
// furnished to do so, subject to the following conditions:
//
// The above copyright notice and this permission notice shall be included in
// all copies or substantial portions of the Software.
//
// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.
//

#import <Foundation/Foundation.h>
#import "git2/oid.h"

@class GTSignature;
@class GTRepository;
@class GTOID;
@class GTObject;

NS_ASSUME_NONNULL_BEGIN

@interface GTNote : NSObject {}

/// The author of the note.
@property (nonatomic, readonly, strong, nullable) GTSignature *author;

/// The committer of the note.
@property (nonatomic, readonly, strong, nullable) GTSignature *committer;

/// Content of the note.
@property (nonatomic, readonly, strong) NSString *note;

@property (nonatomic, readonly, strong) GTObject *target;

/// The underlying `git_note` object.
- (git_note * _Nullable)git_note __attribute__((objc_returns_inner_pointer));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_Nullable ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear, my nullability-fu is really not up-to-speed 😉. I'm just perplexed since it's in the designated initializer...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a remains of an experimental work-in-progress back in the iterations when it could be NULL. This is indeed no longer the case. Fixed.


/// These initializers may fail if there's no note attached to the provided oid.
- (nullable instancetype)initWithTargetOID:(GTOID*)oid repository:(GTRepository*)repository ref:(nullable NSString*)ref;
- (nullable instancetype)initWithTargetGitOID:(git_oid*)oid repository:(git_repository *)repository ref:(const char* _Nullable)ref;

@end

NS_ASSUME_NONNULL_END

75 changes: 75 additions & 0 deletions ObjectiveGit/GTNote.m
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
//
// GTNote.m
// ObjectiveGitFramework
//
// Created by Slava Karpenko on 16.05.16.
// Copyright © 2016 Wildbit LLC. All rights reserved.
//

#import "GTNote.h"
#import "NSError+Git.h"
#import "GTSignature.h"
#import "GTReference.h"
#import "GTRepository.h"
#import "NSString+Git.h"
#import "GTOID.h"

#import "git2/errors.h"
#import "git2/notes.h"

@interface GTNote ()
{
git_note *_note;
}

@end
@implementation GTNote

- (NSString *)description {
return [NSString stringWithFormat:@"<%@: %p>", NSStringFromClass([self class]), self];
}

#pragma mark API

- (void)dealloc {
if (_note != NULL) {
git_note_free(_note);
}
}

- (git_note *)git_note {
return _note;
}

- (NSString*)note {
return @(git_note_message(self.git_note));
}

- (GTSignature *)author {
return [[GTSignature alloc] initWithGitSignature:git_note_author(self.git_note)];
}

- (GTSignature *)committer {
return [[GTSignature alloc] initWithGitSignature:git_note_committer(self.git_note)];
}

- (GTOID*)targetOID {
return [GTOID oidWithGitOid:git_note_id(self.git_note)];
}

- (instancetype)initWithTargetOID:(GTOID*)oid repository:(GTRepository*)repository ref:(NSString*)ref {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: referenceName: maybe ? Also spaces between type and *.

return [self initWithTargetGitOID:(git_oid *)oid.git_oid repository:repository.git_repository ref:ref.UTF8String];
}

- (instancetype)initWithTargetGitOID:(git_oid*)oid repository:(git_repository *)repository ref:(const char*)ref {
if (self = [super init]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style : Explicit nil comparison + return early.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: whitespace.

Copy link
Contributor Author

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. :\

Copy link
Contributor

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 (gitErr != GIT_OK)
return nil; // Cannot read the note, means it either doesn't exists for this object, this object is not found, or whatever else.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will silently swallow errors (e.g. -createNote:... below).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added error parameter to init calls to propagate errors, if needed.

}

return self;
}

@end
14 changes: 14 additions & 0 deletions ObjectiveGit/GTRepository+RemoteOperations.h
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,20 @@ typedef NS_ENUM(NSInteger, GTFetchPruneOption) {
/// will point to an error describing what happened).
- (BOOL)pushBranches:(NSArray<GTBranch *> *)branches toRemote:(GTRemote *)remote withOptions:(nullable NSDictionary *)options error:(NSError **)error progress:(nullable void (^)(unsigned int current, unsigned int total, size_t bytes, BOOL *stop))progressBlock;

/// Push a given Git notes reference name to a remote.
///
/// noteRef - Name of the notes reference. If NULL, will default to whatever the default is (e.g. "refs/notes/commits")
/// remote - The remote to push to. Must not be nil.
/// options - Options applied to the push operation. Can be NULL.
/// Recognized options are:
/// `GTRepositoryRemoteOptionsCredentialProvider`
/// error - The error if one occurred. Can be NULL.
/// progressBlock - An optional callback for monitoring progress. May be NULL.
///
/// Returns YES if the push was successful, NO otherwise (and `error`, if provided,
/// will point to an error describing what happened).
- (BOOL)pushNotes:(nullable NSString *)noteRef toRemote:(GTRemote *)remote withOptions:(nullable NSDictionary *)options error:(NSError **)error progress:(nullable void (^)(unsigned int current, unsigned int total, size_t bytes, BOOL *stop))progressBlock;

/// Delete a remote branch
///
/// branch - The branch to push. Must not be nil.
Expand Down
24 changes: 24 additions & 0 deletions ObjectiveGit/GTRepository+RemoteOperations.m
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@

#import "git2/errors.h"
#import "git2/remote.h"
#import "git2/notes.h"
#import "git2/buffer.h"

NSString *const GTRepositoryRemoteOptionsCredentialProvider = @"GTRepositoryRemoteOptionsCredentialProvider";
NSString *const GTRepositoryRemoteOptionsFetchPrune = @"GTRepositoryRemoteOptionsFetchPrune";
Expand Down Expand Up @@ -198,6 +200,28 @@ - (BOOL)pushBranches:(NSArray *)branches toRemote:(GTRemote *)remote withOptions
return [self pushRefspecs:refspecs toRemote:remote withOptions:options error:error progress:progressBlock];
}

- (BOOL)pushNotes:(NSString *)noteRef toRemote:(GTRemote *)remote withOptions:(NSDictionary *)options error:(NSError **)error progress:(GTRemotePushTransferProgressBlock)progressBlock {
NSParameterAssert(remote != nil);

if (noteRef == nil) {
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);
Copy link
Contributor

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.


if (error != NULL) {
*error = [NSError git_errorFor:gitErr description:@"Unable to get default git notes reference name"];
}

return NO;
}

noteRef = [NSString stringWithUTF8String:default_ref_name.ptr];
}

return [self pushRefspecs:@[[NSString stringWithFormat:@"%@:%@", noteRef, noteRef]] toRemote:remote withOptions:options error:error progress:progressBlock];
}

#pragma mark - Deletion (Public)
- (BOOL)deleteBranch:(GTBranch *)branch fromRemote:(GTRemote *)remote withOptions:(NSDictionary *)options error:(NSError **)error {
NSParameterAssert(branch != nil);
Expand Down
43 changes: 43 additions & 0 deletions ObjectiveGit/GTRepository.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
@class GTTag;
@class GTTree;
@class GTRemote;
@class GTNote;

NS_ASSUME_NONNULL_BEGIN

Expand Down Expand Up @@ -604,6 +605,48 @@ typedef NS_ENUM(NSInteger, GTRepositoryStateType) {
/// Returns YES if operation was successful, NO otherwise
- (BOOL)cleanupStateWithError:(NSError **)error;

/// Creates a new note in this repo (using a default notes reference, e.g. "refs/notes/commits")
///
/// note - Note text.
/// theTarget - Object (usually a commit) to which this note attaches to.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/attaches to/refers

/// This object must belong to this repository.
/// ref - Name for the notes reference in the repo, or nil for default ("refs/notes/commits")
/// author - Signature of the author for this note, and
/// of the note creation time
/// committer - Signature of the committer for this note.
/// overwrite - If set to YES, the note will be overwritten if it already exists.
/// error - Will be filled with a NSError object in case of error.
/// May be NULL.
///
/// Returns the newly created note or nil on error.
- (nullable GTNote *)createNote:(NSString *)note target:(GTObject *)theTarget ref:(nullable NSString *)ref author:(GTSignature *)author committer:(GTSignature *)committer overwriteIfExists:(BOOL)overwrite error:(NSError **)error;

/// Removes a note attached to object in this repo (using a default notes reference, e.g. "refs/notes/commits")
///
/// parentObject - Object (usually a commit) to which the note to be removed is attached to.
/// This object must belong to this repository.
/// ref - Name for the notes reference in the repo, or nil for default ("refs/notes/commits")
/// author - Signature of the author for this note removal, and
/// of the note removal time
/// committer - Signature of the committer for this note removal.
/// error - Will be filled with a NSError object in case of error.
/// May be NULL.
///
/// Returns the YES on success and NO on error.
- (BOOL)removeNoteFromObject:(GTObject *)parentObject ref:(nullable NSString *)ref author:(GTSignature *)author committer:(GTSignature *)committer error:(NSError **)error;

/// Enumerates through all stored notes in this repo (using a default notes reference, e.g. "refs/notes/commits")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the () part can be dropped, as it's not crucial in describing the method, and duplicates the explanation for the parameter below.

///
/// error - Will be filled with a NSError object in case of error.
/// May be null.
/// ref - Name for the notes reference in the repo, or nil for default ("refs/notes/commits")
/// block - A block to be called on each encountered note object. The block accepts
/// a reference to `note`, an `object` that is annotated with the note.
/// If the block sets `stop` to YES, the iterator is finished.
///
/// Returns YES on overall success or NO on error of any kind.
- (BOOL)enumerateNotesWithError:(NSError **)error ref:(nullable NSString *)ref usingBlock:(void (^)(GTNote *note, GTObject *object, BOOL *stop))block;

@end

NS_ASSUME_NONNULL_END
73 changes: 73 additions & 0 deletions ObjectiveGit/GTRepository.m
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
#import "NSError+Git.h"
#import "NSString+Git.h"
#import "GTRepository+References.h"
#import "GTNote.h"

#import "git2.h"

Expand Down Expand Up @@ -919,4 +920,76 @@ - (BOOL)cleanupStateWithError:(NSError * _Nullable __autoreleasing *)error {
return YES;
}

#pragma mark Notes

- (GTNote *)createNote:(NSString *)note target:(GTObject *)theTarget ref:(NSString *)ref author:(GTSignature *)author committer:(GTSignature *)committer overwriteIfExists:(BOOL)overwrite error:(NSError **)error {
git_oid oid;

int gitError = git_note_create(&oid, self.git_repository, ref.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"];

Copy link
Contributor

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 ref:ref];
}

- (BOOL)removeNoteFromObject:(GTObject *)parentObject ref:(NSString *)ref author:(GTSignature *)author committer:(GTSignature *)committer error:(NSError **)error {
int gitError = git_note_remove(self.git_repository, ref.UTF8String, author.git_signature, committer.git_signature, parentObject.OID.git_oid);
if (gitError != GIT_OK) {
if(error != NULL) *error = [NSError git_errorFor:gitError description:@"Failed to delete note from %@", parentObject];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: missing space after if keyword.

return NO;
}

return YES;
}

- (BOOL)enumerateNotesWithError:(NSError **)error ref:(NSString *)ref usingBlock:(void (^)(GTNote *note, GTObject *object, BOOL *stop))block
{
Copy link
Contributor

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.

git_note_iterator* iter = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: git_note_iterator *item


int gitError = git_note_iterator_new(&iter, self.git_repository, ref.UTF8String);

if (gitError != GIT_OK)
{
Copy link
Contributor

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.

if(error != NULL) *error = [NSError git_errorFor:gitError description:@"Failed to enumerate notes"];
return NO;
}

git_oid note_id;
git_oid object_id;
BOOL success = YES;

while (git_note_next(&note_id, &object_id, iter) == GIT_OK) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No error report on iteration failure.

NSError* lookupErr = nil;
Copy link
Contributor

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).


GTNote* note = [[GTNote alloc] initWithTargetGitOID:&object_id repository:self.git_repository ref:ref.UTF8String];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: Extraneous whitespace after =

if (note == nil) {
if (error != NULL)
*error = [NSError git_errorFor:GIT_ENOTFOUND description:@"Cannot create note"];
success = NO;
break;
}

GTObject* obj = [self lookUpObjectByGitOid:&object_id error:&lookupErr];
Copy link
Contributor

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.

Copy link
Contributor

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 😉.

Copy link
Contributor Author

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.

if (obj == nil && lookupErr != nil) {
if (error != NULL)
*error = lookupErr;
success = NO;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is more debatable, but (even though I think there are other places we use that idiom), this prevents us from progressing through failures. Arguably, the "Cocoa way" would be to pass the NSError * to the block, but since it's the first time I notice it, I'll let someone else chime in...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree on that one. I haven't noticed NSError arguments to any iterator blocks, so I attempted to mimic the behavior. OTOH, if the underlying iterator has returned a valid oid, and then we cannot find an object with that oid, something's seriously broken and we probably shouldn't iterate further.

break;
}

BOOL stop = NO;
block(note, obj, &stop);
if (stop) {
break;
}
}

git_note_iterator_free(iter);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can prettify with onExit and a straight return NO above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is still valid 😉.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still still valid ?


return success;
}

@end
1 change: 1 addition & 0 deletions ObjectiveGit/ObjectiveGit.h
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ FOUNDATION_EXPORT const unsigned char ObjectiveGitVersionString[];
#import <ObjectiveGit/GTFilterList.h>
#import <ObjectiveGit/GTFilterSource.h>
#import <ObjectiveGit/GTFetchHeadEntry.h>
#import <ObjectiveGit/GTNote.h>

#import <ObjectiveGit/GTObjectDatabase.h>
#import <ObjectiveGit/GTOdbObject.h>
Expand Down
Loading