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.

Sign up for GitHub

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

Jump to bottom

Improvements to libgit2 #16861

Closed
wants to merge 4 commits into from
Closed

Improvements to libgit2 #16861

wants to merge 4 commits into from

Conversation

kshyatt
Copy link
Contributor

@kshyatt kshyatt commented Jun 10, 2016

I know there's a lot going on here.

  • I documented some functions inline that were confusing to me, and perhaps to others.
  • I added a few utility functions that were "missing" - constructors, a few API functions, etc.
  • I fixed some things that were not BoundsError-ing that really need to be, cc @tkelman
  • I added a bunch of tests

@kshyatt kshyatt added docs This change adds or pertains to documentation test This change adds or pertains to unit tests libgit2 The libgit2 library or the LibGit2 stdlib module labels Jun 10, 2016
tkelman
tkelman reviewed Jun 10, 2016
View reviewed changes
base/libgit2/blob.jl
@@ -1,4 +1,16 @@
# This file is a part of Julia. License is MIT: http://julialang.org/license
function GitBlob(path::AbstractString)
Copy link
Contributor

Choose a reason for hiding this comment

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

put at least one blank line after the license header

tkelman
tkelman reviewed Jun 11, 2016
View reviewed changes
base/libgit2/rebase.jl
@@ -21,9 +21,12 @@ function current(rb::GitRebase)
end

function Base.getindex(rb::GitRebase, i::Csize_t)
if i < 1 || i >= count(rb)
Copy link
Contributor

Choose a reason for hiding this comment

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

what about here, is asking for the last element valid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Knew I missed one!

@kshyatt
Copy link
Contributor Author

kshyatt commented Jun 13, 2016

AppVeyor seems to have timed out. @tkelman, is this ok to go?

@tkelman
Copy link
Contributor

tkelman commented Jun 13, 2016

Not if repo = LibGit2.init(cache_repo) obeys global gitconfigs and could ever create a CRNL-terminated config file, for which this would fail.

wildart
wildart reviewed Jun 13, 2016
View reviewed changes
base/libgit2/blob.jl

function owner(blob::GitBlob)
repo_ptr = @check ccall((:git_blob_owner, :libgit2), Ptr{Void}, (Ptr{Void},), blob.ptr)
return GitRepo(repo_ptr)
Copy link
Member

Choose a reason for hiding this comment

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

Wrong, do not use @check. Test result for C_NULL.

@kshyatt
Copy link
Contributor Author

kshyatt commented Jun 14, 2016

@tkelman @wildart I've updated the PR - thoughts?

tkelman
tkelman reviewed Jun 14, 2016
View reviewed changes
base/libgit2/remote.jl

function name(rmt::GitRemote)
str_ptr = ccall((:git_remote_name, :libgit2), Cstring, (Ptr{Void}, ), rmt.ptr)
str_ptr == C_NULL && return nothing
Copy link
Contributor

@tkelman tkelman Jun 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

empty string or error?

edit: just saw the above

No need for exception, return nothing, because C_NULL is returned for in-memory remotes.

why would we want nothing for an in-memory remote? should we have a better way of dealing with that distinction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, rmt may be anonymous, in which case it has no name, right? This seems like a prime candidate for judicious application of Nullable...

Copy link
Member

Choose a reason for hiding this comment

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

Or empty string

@kshyatt
Copy link
Contributor Author

kshyatt commented Jan 9, 2017

Superseded by #19839 and friends I think.

@kshyatt kshyatt closed this Jan 9, 2017
@kshyatt kshyatt deleted the ksh/libgit2rampage branch January 9, 2017 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
docs This change adds or pertains to documentation libgit2 The libgit2 library or the LibGit2 stdlib module test This change adds or pertains to unit tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kshyatt @tkelman @wildart