Added git_repository_open_ext wrapper #593

glegrain · 2016-10-08T13:18:32Z

Added a GTRepository convenience initializer to use git_repository_open_ext to permit "searching" from the given url.

tiennou · 2016-10-08T15:21:48Z

ObjectiveGit/GTRepository.h

+/// +initWithURL:flags:ceilingDirs:error:.
+///
+/// See respository.h for documentation of each individual flag.
+typedef NS_OPTIONS(UInt32, GTRepositoryOpenFlags) {


NSInteger instead of UInt32. I think UInt32 comes from Carbon ?

Correct, UInt32 will not always be an unsigned int. I guess I will just use an NSInteger and down-cast it to an unsigned int.

tiennou · 2016-10-08T15:23:04Z

ObjectiveGit/GTRepository.h

+/// error        - The error if one occurs.
+///
+/// Returns the initialized repository, or nil if an error occurred.
+ (nullable instancetype)initWithURL:(NSURL *)localFileURL flags:(UInt32)flags ceilingDirs:(nullable const char *)ceilingDirs error:(NSError **)error;


initWith... class-method is confusing. It should be named -repositoryWith... instead.

Use the typedef name instead of UInt32

const char * isn't very Cocoa-y. What about passing an NSArray <NSURL *> * and doing the conversion internally ?

Others class-methods for GTRepository are using initWithURL:.... It make sense for me to stay consistent. (Plus, I don't know how repositoryWithURL... will translate into Swift. I will experiment and see)

NSArray <NSURL *> * it is 😃

tiennou · 2016-10-08T15:26:09Z

ObjectiveGit/GTRepository.m

@@ -177,6 +181,22 @@ - (instancetype)initWithURL:(NSURL *)localFileURL error:(NSError **)error {
 	return [self initWithGitRepository:r];
 }

+- (instancetype)initWithURL:(NSURL *)localFileURL flags:(UInt32)flags ceilingDirs:(const char *)ceilingDirs error:(NSError **)error {
+	if (!localFileURL.isFileURL || localFileURL.path == nil) {
+		if (error != NULL) *error = [NSError errorWithDomain:NSCocoaErrorDomain code:NSFileWriteUnsupportedSchemeError userInfo:@{ NSLocalizedDescriptionKey: NSLocalizedString(@"Invalid file path URL to initialize repository.", @"") }];


NSFileWriteUnsupportedScheme ?

Copy paste error. Will fix.

tiennou · 2016-10-08T15:31:43Z

ObjectiveGit/GTRepository.h

+/// error        - The error if one occurs.
+///
+/// Returns the initialized repository, or nil if an error occurred.
+- (nullable instancetype)initWithURL:(NSURL *)localFileURL flags:(UInt32)flags ceilingDirs:(nullable const char *)ceilingDirs error:(NSError **)error;


Also, having both a class and an instance method is unwarranted IMHO. I'd drop this one.

I totally agree on this one. I will remove the instance method. I only created this one for consistency with the rest

Actually, is there any reason you would rather drop the instance initializer rather than the class initializer? Keeping the convenience instance initializer allows my Swift code to look cleaner: GTRepository(url: url, flags: 0, ceilingDirs: nil)

Usually, my Cocoa-fu says that convenience initializers get declared as class methods, designated initializers go under -init.... Arguably, this one could be kept as a designated initializer (since it ends up calling one of the handful libgit2 functions that creates objects), but our designated initializer is actually initWithGitRepository:.

I'm not sure how that would like after being Swift-preprocessed though.

glegrain

Thank you for your input. Greatly appreciated.

glegrain · 2016-10-08T16:32:26Z

ObjectiveGit/GTRepository.h

+/// +initWithURL:flags:ceilingDirs:error:.
+///
+/// See respository.h for documentation of each individual flag.
+typedef NS_OPTIONS(UInt32, GTRepositoryOpenFlags) {


Correct, UInt32 will not always be an unsigned int. I guess I will just use an NSInteger and down-cast it to an unsigned int.

glegrain · 2016-10-08T16:41:42Z

ObjectiveGit/GTRepository.h

+/// error        - The error if one occurs.
+///
+/// Returns the initialized repository, or nil if an error occurred.
+- (nullable instancetype)initWithURL:(NSURL *)localFileURL flags:(UInt32)flags ceilingDirs:(nullable const char *)ceilingDirs error:(NSError **)error;


I totally agree on this one. I will remove the instance method. I only created this one for consistency with the rest

glegrain · 2016-10-08T16:42:23Z

ObjectiveGit/GTRepository.m

@@ -177,6 +181,22 @@ - (instancetype)initWithURL:(NSURL *)localFileURL error:(NSError **)error {
 	return [self initWithGitRepository:r];
 }

+- (instancetype)initWithURL:(NSURL *)localFileURL flags:(UInt32)flags ceilingDirs:(const char *)ceilingDirs error:(NSError **)error {
+	if (!localFileURL.isFileURL || localFileURL.path == nil) {
+		if (error != NULL) *error = [NSError errorWithDomain:NSCocoaErrorDomain code:NSFileWriteUnsupportedSchemeError userInfo:@{ NSLocalizedDescriptionKey: NSLocalizedString(@"Invalid file path URL to initialize repository.", @"") }];


Copy paste error. Will fix.

glegrain · 2016-10-08T17:16:14Z

ObjectiveGit/GTRepository.h

+/// error        - The error if one occurs.
+///
+/// Returns the initialized repository, or nil if an error occurred.
+ (nullable instancetype)initWithURL:(NSURL *)localFileURL flags:(UInt32)flags ceilingDirs:(nullable const char *)ceilingDirs error:(NSError **)error;


Others class-methods for GTRepository are using initWithURL:.... It make sense for me to stay consistent. (Plus, I don't know how repositoryWithURL... will translate into Swift. I will experiment and see)

NSArray <NSURL *> * it is 😃

glegrain · 2016-10-11T02:35:17Z

Any ideas how to fix Travis?

tiennou · 2016-10-11T07:59:18Z

The iOS tests are flaky, so (unless you happen to find a solution 😉 ), you're better off just making sure the macOS run is green.

pietbrauer · 2016-10-27T12:23:41Z

@glegrain Could you rebase onto master and see if the tests run through? #591 might have fixed the iOS tests, which now also run on Xcode 8

glegrain · 2016-10-31T19:43:49Z

Travis is still failing after rebase

pietbrauer · 2016-10-31T19:47:52Z

iOS is green, Mac is failing because of SSH check which is because of something else. So this should be good from a CI standpoint.

pietbrauer · 2016-10-31T19:50:09Z

Thanks for the PR 👾

tiennou requested changes Oct 8, 2016

View reviewed changes

tiennou reviewed Oct 8, 2016

View reviewed changes

glegrain commented Oct 8, 2016

View reviewed changes

pietbrauer assigned tiennou Oct 27, 2016

glegrain added 3 commits October 31, 2016 13:36

Added git_repository_open_ext wrapper

edf3eff

Removed convenient class initializer

d0bcc78

Changed parameters types

367ad42

glegrain force-pushed the git_repository_open_ext branch from d15fb27 to 367ad42 Compare October 31, 2016 13:22

pietbrauer merged commit 614bce9 into libgit2:master Oct 31, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added git_repository_open_ext wrapper #593

Added git_repository_open_ext wrapper #593

glegrain commented Oct 8, 2016

tiennou Oct 8, 2016

glegrain Oct 8, 2016

tiennou Oct 8, 2016

tiennou Oct 8, 2016

glegrain Oct 8, 2016

tiennou Oct 8, 2016

glegrain Oct 8, 2016

tiennou Oct 8, 2016

glegrain Oct 8, 2016

glegrain Oct 11, 2016

tiennou Oct 11, 2016

glegrain left a comment

glegrain Oct 8, 2016

glegrain Oct 8, 2016

glegrain Oct 8, 2016

glegrain Oct 8, 2016

glegrain commented Oct 11, 2016

tiennou commented Oct 11, 2016

pietbrauer commented Oct 27, 2016

glegrain commented Oct 31, 2016

pietbrauer commented Oct 31, 2016 •

edited

Loading

pietbrauer commented Oct 31, 2016

Added git_repository_open_ext wrapper #593

Added git_repository_open_ext wrapper #593

Conversation

glegrain commented Oct 8, 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

glegrain left a 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

glegrain commented Oct 11, 2016

tiennou commented Oct 11, 2016

pietbrauer commented Oct 27, 2016

glegrain commented Oct 31, 2016

pietbrauer commented Oct 31, 2016 • edited Loading

pietbrauer commented Oct 31, 2016

pietbrauer commented Oct 31, 2016 •

edited

Loading