Add some nullables to methods & properties that can return nil #589

Uncommon · 2016-09-15T22:38:51Z

Xcode 8 now complains about methods and properties that return nil but are not declared nullable. This fixes all of those analyzer warnings.

Uncommon · 2016-09-15T22:39:43Z

ObjectiveGit/GTCredential.h

@@ -46,7 +46,7 @@ NS_ASSUME_NONNULL_BEGIN
 /// type     - the credential types allowed by the operation.
 /// URL      - the URL the operation is authenticating against.
 /// userName - the user name provided by the operation. Can be nil, and might be ignored.
- (GTCredential * _Nullable)credentialForType:(GTCredentialType)type URL:(NSString *)URL userName:(nullable NSString *)userName;
+- (GTCredential * _Nullable)credentialForType:(GTCredentialType)type URL:(nullable NSString *)URL userName:(nullable NSString *)userName;


GTCredentialAcquireCallback potentially calls this with a nil url.

The libgit2 documentation implies that it's never NULL. At least git_remote_connect will return an error on a NULL URL. It seems the only other user is proxy support, and in that case it's not clear (there's a check skipping NULLs in git_proxy_options_dup).

I can't even find where it's used, searching for git_cred_acquire_cb etc.

I used the regex /->cred(entials|_acquire_cb)/ against libgit2 and got most of them.

It looks like it's probably supposed to be non-null, but to guarantee it I'd rather depend on someone who knows the code better.

tiennou · 2016-09-15T23:20:02Z

Question : does NSAssert-ing that the return value is not nil makes the warning go away ? Some of those changes look strange, eg. GTOID having a nullable -SHA, or GTReference and a nullable -OID...

Uncommon · 2016-09-16T14:40:44Z

Yes it does. Would you like me to replace the null checks with NSAsserts?

tiennou · 2016-09-16T14:48:41Z

Good question. From an API standpoint, I think asserting makes it cleaner. On the other hand, if it ever happens, we'll throw an exception at runtime, which is Not Cool.

Uncommon · 2016-09-16T15:11:29Z

Let's think about the actual failure cases, then.

For GTOID.SHA:

Failure to malloc 40 bytes. If that happens, you're in trouble anyway.
NSString.initWithBytesNoCopy is declared as nullable, but doesn't specify when it might return null. Presumably it's if the given string doesn't fit the specified encoding. If git_oid_fmt yields something that's not UTF-8 compliant, then something is seriously wrong.

I think it's okay to assert in those cases. I'll proceed similarly with the others and see what you think.

Uncommon · 2016-09-16T15:32:12Z

ObjectiveGit/GTReference.h

@@ -52,7 +52,7 @@ NS_ASSUME_NONNULL_BEGIN
 @property (nonatomic, readonly, strong) GTRepository *repository;
 @property (nonatomic, readonly) GTReferenceType referenceType;
 @property (nonatomic, readonly) const git_oid *git_oid;
-@property (nonatomic, strong, readonly) GTOID *OID;
+@property (nonatomic, strong, readonly, nullable) GTOID *OID;


git_reference_target returns null if it's not a direct reference, so this should be nullable.

Uncommon · 2016-09-16T15:38:07Z

ObjectiveGit/GTSignature.h

@@ -42,10 +42,10 @@ NS_ASSUME_NONNULL_BEGIN
 @property (nonatomic, readonly, copy, nullable) NSString *email;

 /// The time when the action happened.
-@property (nonatomic, readonly, strong) NSDate *time;
+@property (nonatomic, readonly, strong, nullable) NSDate *time;


The other properties are nullable, so this is consistent.

On the other hand, they all return nil if git_signature is nil, and it's not nullable, and it doesn't look like it should ever be null.

But since I'm making this consistent with the other properties, I'll leave that as a separate issue.

IIRC it's done that way because libgit2 might fail to parse an object's headers (in some degenerate-odb cases), hence allowing some of them to be null.

tiennou · 2016-09-16T16:44:56Z

ObjectiveGit/GTSignature.h

@@ -42,10 +42,10 @@ NS_ASSUME_NONNULL_BEGIN
 @property (nonatomic, readonly, copy, nullable) NSString *email;

 /// The time when the action happened.
-@property (nonatomic, readonly, strong) NSDate *time;
+@property (nonatomic, readonly, strong, nullable) NSDate *time;


IIRC it's done that way because libgit2 might fail to parse an object's headers (in some degenerate-odb cases), hence allowing some of them to be null.

tiennou · 2016-09-16T16:57:50Z

ObjectiveGit/GTOID.m


 	git_oid_fmt(SHA, self.git_oid);

 	NSString *str = [[NSString alloc] initWithBytesNoCopy:SHA length:GIT_OID_HEXSZ encoding:NSUTF8StringEncoding freeWhenDone:YES];
-	if (str == nil) free(SHA);
+	NSAssert(str != nil, @"Failed to allocate SHA string");


Maybe

char *SHA = git_oid_tostr_s(self.git_oid); // Stored in TLS NSString *str = [[NSString alloc] initWithBytes:SHA length:GIT_OID_HEXSZ encoding:NSUTF8StringEncoding]; // So we copy the TLS data NSAssert(str != nil, @"Failed to allocate SHA string");

?

Even better!

tiennou · 2016-09-16T16:59:38Z

ObjectiveGit/GTCredential.h

@@ -46,7 +46,7 @@ NS_ASSUME_NONNULL_BEGIN
 /// type     - the credential types allowed by the operation.
 /// URL      - the URL the operation is authenticating against.
 /// userName - the user name provided by the operation. Can be nil, and might be ignored.
- (GTCredential * _Nullable)credentialForType:(GTCredentialType)type URL:(NSString *)URL userName:(nullable NSString *)userName;
+- (GTCredential * _Nullable)credentialForType:(GTCredentialType)type URL:(nullable NSString *)URL userName:(nullable NSString *)userName;


I used the regex /->cred(entials|_acquire_cb)/ against libgit2 and got most of them.

Uncommon · 2016-09-16T22:44:09Z

I see the Travis failure, but it doesn't seem to be related to my changes.

Uncommon · 2016-10-17T15:48:04Z

(bump) Any further changes needed on this?

tiennou · 2016-10-18T08:43:56Z

ObjectiveGit/GTOID.m

+	char *SHA = git_oid_tostr_s(self.git_oid);
+	NSString *str = [[NSString alloc] initWithBytes:SHA
+																					 length:GIT_OID_HEXSZ
+																				 encoding:NSUTF8StringEncoding];


Crazy indentation here 😉 .

Looks like Xcode was doing tabs instead of spaces, at 2 spaces per tab. Fixed.

Uncommon · 2016-11-23T17:00:18Z

Another bump. Any more issues?

tiennou · 2016-11-23T21:02:18Z

Not that I can see. A local run of script/cibuild worked here so I'm merging this. Thanks for the PR !

Add some nullables to methods & properties that can return nil

fc9e959

Uncommon commented Sep 15, 2016

View reviewed changes

Uncommon commented Sep 16, 2016

View reviewed changes

Use asserts because GTOID.SHA should never be nil.

bbea49d

tiennou reviewed Sep 16, 2016

View reviewed changes

Use git_oid_tostr_s for GTOID.SHA

b4ea30c

tiennou reviewed Oct 18, 2016

View reviewed changes

Fix indentation

10420c9

pietbrauer assigned tiennou Oct 27, 2016

tiennou merged commit 3480278 into libgit2:master Nov 23, 2016

Uncommon deleted the nullables branch December 5, 2016 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add some nullables to methods & properties that can return nil #589

Add some nullables to methods & properties that can return nil #589

Uncommon commented Sep 15, 2016

Uncommon Sep 15, 2016

tiennou Sep 15, 2016

Uncommon Sep 16, 2016

tiennou Sep 16, 2016

Uncommon Sep 16, 2016

tiennou commented Sep 15, 2016 •

edited

Loading

Uncommon commented Sep 16, 2016

tiennou commented Sep 16, 2016

Uncommon commented Sep 16, 2016

Uncommon Sep 16, 2016

Uncommon Sep 16, 2016

tiennou Sep 16, 2016

tiennou Sep 16, 2016

tiennou Sep 16, 2016

Uncommon Sep 16, 2016

tiennou Sep 16, 2016

Uncommon commented Sep 16, 2016

Uncommon commented Oct 17, 2016

tiennou Oct 18, 2016

Uncommon Oct 18, 2016

Uncommon commented Nov 23, 2016

tiennou commented Nov 23, 2016

Add some nullables to methods & properties that can return nil #589

Add some nullables to methods & properties that can return nil #589

Conversation

Uncommon commented Sep 15, 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

tiennou commented Sep 15, 2016 • edited Loading

Uncommon commented Sep 16, 2016

tiennou commented Sep 16, 2016

Uncommon commented Sep 16, 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

Uncommon commented Sep 16, 2016

Uncommon commented Oct 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uncommon commented Nov 23, 2016

tiennou commented Nov 23, 2016

tiennou commented Sep 15, 2016 •

edited

Loading