-
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
Split the OID lookup from the object lookup in GTEnumerator #590
Conversation
I would like a second pair of eyes on that one. IMHO it's a small change, and it allows us to not-unexpectedly-fail when working against shallow repositories. |
@@ -290,7 +290,10 @@ - (id)lookUpObjectByGitOid:(const git_oid *)oid objectType:(GTObjectType)type er | |||
if (error != NULL) { | |||
char oid_str[GIT_OID_HEXSZ+1]; | |||
git_oid_tostr(oid_str, sizeof(oid_str), oid); | |||
*error = [NSError git_errorFor:gitError description:@"Failed to lookup object %s in repository.", oid_str]; | |||
*error = [NSError git_errorFor:gitError | |||
description:@"Failed to lookup object" |
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.
Indentation seems a bit off
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.
It actually looks fine in Xcode, I've checked. I'd say GitHub gets confused because Holy Wars™ 😉 .
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 sure, Xcode is fine with mixing tabs and spaces, Git is not.
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.
Here's what I see in Xcode (TextMate show the same thing) : https://www.dropbox.com/s/wb3zu4xl3dss0v8/Capture%20d%27%C3%A9cran%202016-10-26%2015.31.21.PNG?dl=0
GH's Obj-C styleguide doesn't really help in that regard. A quick fix would be to disallow newline-in-selectors, which would make the align-colons-in-selectors point moot. But that line would get gigantic...
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.
Arguably, I'm breaking the styleguide by using spaces to align colons. So there's that too...
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 are right. Please remove the multi line error and make it one line. I looked up the other usages of this error method and it is a one liner everywhere. #StickingToTheStyleGuide
#FollowTheStyleGuide
Fixed styling. I'll keep my fingers crossed that Travis doesn't hate me... |
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.
Great 👾
This is a first step toward handling shallow repositories, by separating the lookup done while enumerating.
Also provides the missing OID through the error.
Related to libgit2/libgit2#3058