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

fix some LibGit2 errors #20155

Merged
merged 5 commits into from
Jan 21, 2017
Merged
Show file tree
Hide file tree
Changes from 2 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
6 changes: 3 additions & 3 deletions base/libgit2/diff.jl
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@
function Base.cconvert(::Type{Ptr{DiffOptionsStruct}}, pathspecs::AbstractString)
str_ref = Base.cconvert(Ref{Cstring}, [pathspecs])
sa = StrArrayStruct(Base.unsafe_convert(Ref{Cstring}, str_ref), 1)
do_ref = Ref(DiffOptions(pathspec = sa))
do_ref = Ref(DiffOptionsStruct(pathspec = sa))
do_ref, str_ref
end
function Base.unsafe_convert(::Type{Ptr{DiffOptionsStruct}}, rr::Tuple{Ref{DiffOptionsStruct}, Ref{Cstring}})
Base.unsafe_convert(Ptr{DiffOptionStruct}, first(rr))
Base.unsafe_convert(Ptr{DiffOptionsStruct}, first(rr))
end


Expand Down Expand Up @@ -42,6 +42,6 @@ function Base.getindex(diff::GitDiff, i::Integer)
delta_ptr = ccall((:git_diff_get_delta, :libgit2),
Ptr{DiffDelta},
(Ptr{Void}, Csize_t), diff.ptr, i-1)
delta_ptr == C_NULL && return nothing
delta_ptr == C_NULL && throw(BoundsError("Attempt to access $(count(diff))-element GitDiff at index $i"))
Copy link
Contributor

Choose a reason for hiding this comment

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

boundserror doesn't take a string, does it?

Copy link
Contributor

Choose a reason for hiding this comment

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

julia> BoundsError("yolo")
BoundsError("yolo",#undef)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

return unsafe_load(delta_ptr)
end
14 changes: 9 additions & 5 deletions base/libgit2/libgit2.jl
Original file line number Diff line number Diff line change
Expand Up @@ -75,18 +75,22 @@ end
isdirty(repo::GitRepo, paths::AbstractString=""; cached::Bool=false) =
isdiff(repo, Consts.HEAD_FILE, paths, cached=cached)

""" git diff-index <treeish> [-- <path>]"""
"""
LibGit2.isdiff(repo::GitRepo, treeish[, paths]; [cached=false])
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we typically put keyword arguments in brackets in docstrings?


Checks if there are any differences between the tree specified by `treeish` and working tree (if `cached=false`) or the index (if `cached=true`).
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to break onto multiple lines to keep the line length down.


See `git diff-index <treeish> [-- <path>]`
"""
function isdiff(repo::GitRepo, treeish::AbstractString, paths::AbstractString=""; cached::Bool=false)
tree_oid = revparseid(repo, "$treeish^{tree}")
iszero(tree_oid) && return true
iszero(tree_oid) && return error("invalid treeish $treeish") # this can be removed by #20104
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps just error instead of return error? Might actually be better as throw(ArgumentError(...)).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a stopgap, will get removed after we merge #20104

result = false
tree = get(GitTree, repo, tree_oid)
try
diff = diff_tree(repo, tree, paths)
diff = diff_tree(repo, tree, paths, cached=cached)
result = count(diff) > 0
close(diff)
catch err
result = true
finally
close(tree)
end
Expand Down
30 changes: 30 additions & 0 deletions test/libgit2.jl
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,36 @@ mktempdir() do dir
close(repo)
end
end

@testset "diff" begin
repo = LibGit2.GitRepo(cache_repo)
try
@test !LibGit2.isdirty(repo)
@test !LibGit2.isdiff(repo, "HEAD")
@test !LibGit2.isdirty(repo, cached=true)
@test !LibGit2.isdiff(repo, "HEAD", cached=true)
open(joinpath(cache_repo,test_file), "a") do f
println(f, "zzzz")
end
@test LibGit2.isdirty(repo)
@test LibGit2.isdiff(repo, "HEAD")
@test !LibGit2.isdirty(repo, cached=true)
@test !LibGit2.isdiff(repo, "HEAD", cached=true)
LibGit2.add!(repo, test_file)
@test LibGit2.isdirty(repo)
@test LibGit2.isdiff(repo, "HEAD")
@test LibGit2.isdirty(repo, cached=true)
@test LibGit2.isdiff(repo, "HEAD", cached=true)
LibGit2.commit(repo, "zzz")
@test !LibGit2.isdirty(repo)
@test !LibGit2.isdiff(repo, "HEAD")
@test !LibGit2.isdirty(repo, cached=true)
@test !LibGit2.isdiff(repo, "HEAD", cached=true)
finally
close(repo)
end
end

Copy link
Member

Choose a reason for hiding this comment

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

I think typically we don't put line breaks between ends at different indentation levels.

end

@testset "Fetch from cache repository" begin
Expand Down