-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
fix some LibGit2 errors #20155
Conversation
Ah, found the problem. Tests added. |
d5e3800
to
724c020
Compare
@@ -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")) |
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.
boundserror doesn't take a string, does it?
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.
julia> BoundsError("yolo")
BoundsError("yolo",#undef)
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.
Fixed.
@@ -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]) |
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.
I don't think we typically put keyword arguments in brackets in docstrings?
""" | ||
LibGit2.isdiff(repo::GitRepo, treeish[, paths]; [cached=false]) | ||
|
||
Checks if there are any differences between the tree specified by `treeish` and working tree (if `cached=false`) or the index (if `cached=true`). |
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.
Would be nice to break onto multiple lines to keep the line length down.
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 |
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.
Perhaps just error
instead of return error
? Might actually be better as throw(ArgumentError(...))
.
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.
This is a stopgap, will get removed after we merge #20104
close(repo) | ||
end | ||
end | ||
|
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.
I think typically we don't put line breaks between end
s at different indentation levels.
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) |
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.
not something that this pr should address, but "isdiff" is an odd name - sounds like it's asking whether something is a diff
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.
I agree. I've added it to #19839
Some of these were due to typos in #19962. I do have some tests, but
they're currently failing.