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

Moar wrapper functions #549

Merged
merged 7 commits into from
Feb 17, 2016
Merged

Moar wrapper functions #549

merged 7 commits into from
Feb 17, 2016

Conversation

slavikus
Copy link
Contributor

@slavikus slavikus commented Feb 2, 2016

Added wrappers for git_diff_index_to_index() and git_merge_commits().

/// Merges the given commit into the receiver in memory and produces the result as
/// an index.
///
/// otherTree - The commit with which the receiver should be merged with. Cannot be
Copy link
Member

Choose a reason for hiding this comment

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

Should be otherCommit

///
/// Returns an index which represents the result of the merge, or nil if an error
/// occurred.
- (nullable GTIndex *)merge:(GTCommit *)otherCommit error:(NSError **)error;
Copy link
Member

Choose a reason for hiding this comment

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

Don't GTCommit and NSError need nullability annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

git_merge_commits() requires second commit to exist, so no.

As for NSError nullability, I've followed the syntax in -[GTTree merge:ancestor:error:] notation.

Copy link
Member

Choose a reason for hiding this comment

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

I mean nonnull GTCommit * else GTCommit! is inferred for swift?

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry, just saw the macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, well the header has NS_ASSUME_NONNULL_BEGIN on top, so everything assumed to have nonnull as long as it's not explicitly stated otherwise.

@joshaber
Copy link
Member

joshaber commented Feb 3, 2016

Looks good to me!

@pietbrauer Are you 👍?

@pietbrauer
Copy link
Member

I would vote for tests but to me it looks good otherwise.

@joshaber
Copy link
Member

joshaber commented Feb 3, 2016

Cool, I'ma assign this to you @pietbrauer. Feel free to merge when you're 👍 on it.

@pietbrauer pietbrauer closed this Feb 11, 2016
@pietbrauer pietbrauer reopened this Feb 11, 2016
pietbrauer added a commit that referenced this pull request Feb 17, 2016
@pietbrauer pietbrauer merged commit 8b46e45 into libgit2:master Feb 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants