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

Improvements to libgit2 #16861

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions base/libgit2/blob.jl
Original file line number Diff line number Diff line change
@@ -1,5 +1,19 @@
# 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

blob_ptr_ptr = Ref{Ptr{Void}}(C_NULL)
apath = abspath(path)
ccall((:git_blob_create_fromdisk, :libgit2), Ptr{Void},
(Ptr{Ptr{Void}}, Ptr{UInt8}), blob_ptr_ptr, apath)
blob_ptr_ptr == C_NULL && throw(Error.GitError(Error.ERROR))
return GitBlob(blob_ptr_ptr[])
Copy link
Contributor

@tkelman tkelman Jun 10, 2016

Choose a reason for hiding this comment

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

should this check for null before dereferencing or trying to create a GitBlob?

edit: I guess there's an assertion in the auto-generated constructor

Copy link
Member

Choose a reason for hiding this comment

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

If error is returned, the exception will be raised.

end

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.

end

function content(blob::GitBlob)
return ccall((:git_blob_rawcontent, :libgit2), Ptr{Void}, (Ptr{Void},), blob.ptr)
end
Expand Down
5 changes: 4 additions & 1 deletion base/libgit2/diff.jl
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,12 @@ function Base.count(diff::GitDiff)
end

function Base.getindex(diff::GitDiff, i::Csize_t)
if i < 1 || i > count(diff)
throw(BoundsError(diff, i))
end
delta_ptr = ccall((:git_diff_get_delta, :libgit2), Ptr{Void},
(Ptr{Void}, Csize_t), diff.ptr, i-1)
delta_ptr == C_NULL && return nothing
delta_ptr == C_NULL && throw(BoundsError(diff, i))
return unsafe_load(convert(Ptr{DiffDelta}, delta_ptr), 1)
end
Base.getindex(diff::GitDiff, i::Int) = getindex(diff, Csize_t(i))
5 changes: 4 additions & 1 deletion base/libgit2/index.jl
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,12 @@ function Base.count(idx::GitIndex)
end

function Base.getindex(idx::GitIndex, i::Csize_t)
if i < 1 || i > count(idx)
throw(BoundsError(idx,i))
end
ie_ptr = ccall((:git_index_get_byindex, :libgit2), Ptr{Void},
(Ptr{Void}, Csize_t), idx.ptr, i-1)
ie_ptr == C_NULL && return nothing
ie_ptr == C_NULL && throw(BoundsError(idx,i))
return unsafe_load(convert(Ptr{IndexEntry}, ie_ptr), 1)
end
Base.getindex(idx::GitIndex, i::Int) = getindex(idx, Csize_t(i))
6 changes: 6 additions & 0 deletions base/libgit2/libgit2.jl
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,13 @@ function diff_files(repo::GitRepo, branch1::AbstractString, branch2::AbstractStr
return files
end

""" Determines if `b` is a child of `a`. `a` and `b` should be OIDs converted to `String`s. """
function is_ancestor_of(a::AbstractString, b::AbstractString, repo::GitRepo)
A = revparseid(repo, a)
merge_base(repo, a, b) == A
end

""" Set a `remote`'s `url` for the `repo`, with default `remote` name `origin`. Uses SSH protocol, not HTTPS."""
function set_remote_url(repo::GitRepo, url::AbstractString; remote::AbstractString="origin")
with(GitConfig, repo) do cfg
set!(cfg, "remote.$remote.url", url)
Expand All @@ -137,6 +139,7 @@ function set_remote_url(repo::GitRepo, url::AbstractString; remote::AbstractStri
end
end

""" Set a `remote`'s `url` for the `repo` located at `path`, with default `remote` name `origin`. Uses SSH protocol, not HTTPS."""
function set_remote_url(path::AbstractString, url::AbstractString; remote::AbstractString="origin")
with(GitRepo, path) do repo
set_remote_url(repo, url, remote=remote)
Expand Down Expand Up @@ -459,6 +462,8 @@ function authors(repo::GitRepo)
end
end

""" Makes a record of the repository's current state so that it can be
restored to that state later, if needed. """
function snapshot(repo::GitRepo)
head = Oid(repo, Consts.HEAD_FILE)
index = with(GitIndex, repo) do idx; write_tree!(idx) end
Expand All @@ -480,6 +485,7 @@ function snapshot(repo::GitRepo)
State(head, index, work)
end

""" Returns a repository to a state previously recorded with `snapshot`."""
function restore(s::State, repo::GitRepo)
reset!(repo, Consts.HEAD_FILE, "*") # unstage everything
with(GitIndex, repo) do idx
Expand Down
5 changes: 4 additions & 1 deletion base/libgit2/rebase.jl
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,12 @@ function current(rb::GitRebase)
end

function Base.getindex(rb::GitRebase, i::Csize_t)
if i < 1 || i > count(rb)
throw(BoundsError(rb,i))
end
rb_op_ptr = ccall((:git_rebase_operation_byindex, :libgit2), Ptr{Void},
(Ptr{Void}, Csize_t), rb.ptr, i-1)
rb_op_ptr == C_NULL && return nothing
rb_op_ptr == C_NULL && throw(BoundsError(rb,i))
return unsafe_load(convert(Ptr{RebaseOperation}, rb_op_ptr), 1)
end
Base.getindex(rb::GitRebase, i::Int) = getindex(rb, Csize_t(i))
Expand Down
6 changes: 6 additions & 0 deletions base/libgit2/remote.jl
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,9 @@ function push{T<:AbstractString}(rmt::GitRemote, refspecs::Vector{T};
!no_refs && finalize(sa)
end
end

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

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

return unsafe_string(str_ptr)
end
4 changes: 2 additions & 2 deletions base/libgit2/status.jl
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,11 @@ end

function Base.getindex(status::GitStatus, i::Csize_t)
if i < 1 || i > length(status)
throw(BoundsError())
throw(BoundsError(status,i))
end
entry_ptr = ccall((:git_status_byindex, :libgit2), Ptr{Void},
(Ptr{Void}, Csize_t), status.ptr, i-1)
entry_ptr == C_NULL && throw(Error.GitError(Error.ERROR))
entry_ptr == C_NULL && throw(BoundsError(status,i))
return unsafe_load(convert(Ptr{StatusEntry}, entry_ptr), 1)
end
Base.getindex(status::GitStatus, i::Int) = getindex(status, Csize_t(i))
4 changes: 2 additions & 2 deletions base/libgit2/tree.jl
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ end

function filename(te::GitTreeEntry)
str = ccall((:git_tree_entry_name, :libgit2), Cstring, (Ptr{Void},), te.ptr)
str != C_NULL && return unsafe_string(str)
return nothing
str == C_NULL && throw(Error.GitError(Error.ERROR))
return unsafe_string(str)
end

function filemode(te::GitTreeEntry)
Expand Down
4 changes: 2 additions & 2 deletions base/libgit2/walker.jl
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ end

function repository(w::GitRevWalker)
ptr = ccall((:git_revwalk_repository, :libgit2), Ptr{Void}, (Ptr{Void},), w.ptr)
ptr != C_NULL && return GitRepo(ptr)
return nothing
ptr == C_NULL && throw(Error.GitError(Error.ERROR))
return GitRepo(ptr)
end

function Base.map(f::Function, walker::GitRevWalker;
Expand Down
57 changes: 56 additions & 1 deletion test/libgit2.jl
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,16 @@ const LIBGIT2_VER = v"0.23.0"
finally
finalize(sa2)
end
vs = vec([p1,p2])
sa4 = LibGit2.StrArrayStruct(vs)
try
arr3 = convert(Vector{AbstractString}, sa4)
@test arr3[1] == p1
@test arr3[2] == p2
finally
finalize(sa4)
end

#end

#@testset "Signature" begin
Expand Down Expand Up @@ -119,8 +129,30 @@ mktempdir() do dir

config = joinpath(cache_repo, ".git", "config")
lines = split(open(readstring, config, "r"), "\n")
@test any(map(x->x == "[remote \"upstream\"]", lines))
@test any(x->x == "[remote \"upstream\"]", lines)

remote = LibGit2.get(LibGit2.GitRemote, repo, branch)
@test LibGit2.url(remote) == repo_url
@test LibGit2.isattached(repo)
finalize(remote)
finally
finalize(repo)
end
#end

#@testset "with remote branch and refspec" begin
repo = LibGit2.init(cache_repo)
try
@test isdir(cache_repo)
@test isdir(joinpath(cache_repo, ".git"))

# set a remote branch
branch = "upstream2"
LibGit2.GitRemote(repo, branch, repo_url,"+refs/heads/$branch:refs/remotes/origin/$branch") |> finalize

config = joinpath(cache_repo, ".git", "config")
lines = split(open(readstring, config, "r"), "\n")
@test any(x->x == "[remote \"upstream\"]", lines)
remote = LibGit2.get(LibGit2.GitRemote, repo, branch)
@test LibGit2.url(remote) == repo_url
@test LibGit2.isattached(repo)
Expand All @@ -130,6 +162,24 @@ mktempdir() do dir
end
#end

#@testset "anonymous" begin
repo = LibGit2.init(cache_repo)
try
@test isdir(cache_repo)
@test isdir(joinpath(cache_repo, ".git"))

# set a remote branch
branch = "upstream3"
LibGit2.GitRemoteAnon(repo, repo_url) |> finalize

config = joinpath(cache_repo, ".git", "config")
lines = split(open(readstring, config, "r"), "\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd want to check that these don't fail if they get windows newlines in them (or check that it's impossible for that to be the case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I should just do a run(unix2dos(some crap)) or something here?

Copy link
Contributor

@tkelman tkelman Jun 21, 2016

Choose a reason for hiding this comment

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

that would modify the file and rely on that program being installed - maybe do something with eachline and chomp ?

Copy link
Member

@wildart wildart Jun 21, 2016

Choose a reason for hiding this comment

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

Does .git/config file on windows really have \r symbols?

Copy link
Contributor

Choose a reason for hiding this comment

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

it might if you edit it with something dumb

@test any(x->x == "[remote \"upstream\"]", lines)
finally
finalize(repo)
end
#end

#@testset "bare" begin
path = joinpath(dir, "Example.Bare")
repo = LibGit2.init(path, true)
Expand Down Expand Up @@ -177,6 +227,7 @@ mktempdir() do dir
@test LibGit2.fetch_refspecs(rmt)[1] == "+refs/*:refs/*"
@test LibGit2.isattached(repo)
@test LibGit2.remotes(repo) == ["origin"]
@test LibGit2.name(rmt) == "origin"
finally
finalize(rmt)
end
Expand Down Expand Up @@ -229,6 +280,10 @@ mktempdir() do dir
@test auth.email == test_sig.email
end

@test LibGit2.iscommit(string(commit_oid1),repo)
@test LibGit2.iscommit(string(commit_oid2),repo)
@test LibGit2.is_ancestor_of(string(commit_oid1),string(commit_oid2),repo)
@test LibGit2.revcount(repo, string(commit_oid1), string(commit_oid2)) == (-1,1)
Copy link
Contributor

Choose a reason for hiding this comment

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

the inconsistency in argument order between these is unfortunate

# lookup commits
cmt = LibGit2.get(LibGit2.GitCommit, repo, commit_oid1)
try
Expand Down