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

Convenience API and bug fixes for git tree walking #25432

Merged
merged 5 commits into from
Jan 27, 2018
Merged

Convenience API and bug fixes for git tree walking #25432

merged 5 commits into from
Jan 27, 2018

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Jan 6, 2018 •
edited
Loading

This PR fixes a bug with LibGit2's treewalk wrapper, and adds a convenience getindex-based API for accessing objects in a tree by path. First time working with libgit2(.jl), so feel free to bikeshed 🙂

EDIT: I only ran the libgit2 tests locally because of the device I'm working on, so this might cause test failures.

@maleadt maleadt added the libgit2 The libgit2 library or the LibGit2 stdlib module label Jan 6, 2018
maleadt
maleadt commented Jan 6, 2018
View reviewed changes
base/libgit2/oid.jl
@@ -139,6 +139,8 @@ function GitHash(obj::GitObject)
GitHash(ccall((:git_object_id, :libgit2), Ptr{UInt8}, (Ptr{Cvoid},), obj.ptr))
end

==(obj1::GitObject, obj2::GitObject) = GitHash(obj1) == GitHash(obj2)
Copy link
Member Author

Choose a reason for hiding this comment

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

This seemed like a reasonable thing to do, but is not really required for this PR (it simplifies a test).

Copy link
Member

Choose a reason for hiding this comment

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

It seems reasonable to me as well, so long as there's a test for it. 🙂

ararslan
ararslan reviewed Jan 6, 2018
View reviewed changes
base/libgit2/tree.jl Outdated
@@ -122,3 +131,47 @@ function Base.show(io::IO, tree::GitTree)
println(io, "Owner: ", repository(tree))
println(io, "Number of entries: ", count(tree))
end

"""
getindex(tree::GitTree, target::AbstractString)::GitHash
Copy link
Member

Choose a reason for hiding this comment

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

Base still uses -> to show return types in docstrings

Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Member

Choose a reason for hiding this comment

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

Partly historical artifact, partly because it's often used in such a way that the :: T return type syntax would be wrong or inappropriate.

@ararslan ararslan requested review from kshyatt and simonbyrne January 6, 2018 20:00
@maleadt maleadt force-pushed the tb/treewalk branch 2 times, most recently from 0a16f68 to a0e9e40 Compare January 7, 2018 16:57
@maleadt
Copy link
Member Author

maleadt commented Jan 11, 2018

Bump? Comments have been addressed. Or should this wait until after 1.0?

@StefanKarpinski
Copy link
Member

No, this can be merged now if someone familiar with the LibGit2 code takes a look at it. Maybe @kshyatt?

kshyatt
kshyatt reviewed Jan 22, 2018
View reviewed changes
base/libgit2/tree.jl Outdated
"""
getindex(tree::GitTree, target::AbstractString) -> GitObject

Look up `target` path in the `tree`, returning a `GitObject` (a `GitBlob` in the case of a
Copy link
Contributor

Choose a reason for hiding this comment

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

X-refs here?

kshyatt
kshyatt approved these changes Jan 22, 2018
View reviewed changes
Copy link
Contributor

@kshyatt kshyatt left a comment

Choose a reason for hiding this comment

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

other than the minor request to xref docs this lgtm sorry for the wait on the review

@maleadt
Copy link
Member Author

maleadt commented Jan 23, 2018

OK, addressed all comments. Will merge if CI is green.

@StefanKarpinski
Copy link
Member

AppVeyor failures where libgit2 remote exceptions. I've restarted them to make sure they're not actually related since this change does touch libgit2 code. Suspiciously, the Travis Linux 64 failure was also a libgit2 remote exception. I've restarted that one as well, so we'll see...

@maleadt
Copy link
Member Author

maleadt commented Jan 23, 2018

I'll look into it if any more suspicious failures turn up, after all I've also rebased the branch which might have introduced something.

@maleadt
Copy link
Member Author

maleadt commented Jan 25, 2018

So this keeps throwing the RemoteException on AppVeyor during libgit2 tests, while other CI services are doing fine (only CircleCI fails, but due to a different ProcessExitedException). So this is probably related, I guess? Annoying how I can't see the original exception though (I assume this is a known issue?):

libgit2: Error During Test at nothing:1
  Test threw an exception of type RemoteException
  Expression: libgit2
  Error deserializing a remote exception from worker 11
  Remote(original) exception of type TestSetException
  Remote stacktrace : 
  finish at C:\projects\julia\usr\share\julia\site\v0.7\Test\src\Test.jl:784
  macro expansion at .\util.jl:310 [inlined]
  top-level scope at .\<missing>:19
  eval at .\boot.jl:295
  #runtests#1 at C:\projects\julia\julia-31f138a0c1\share\julia\test\testdefs.jl:25
  #runtests at .\<missing>:0 [inlined] (repeats 2 times)
  #125 at C:\projects\julia\usr\share\julia\site\v0.7\Distributed\src\process_messages.jl:269
  run_work_thunk at C:\projects\julia\usr\share\julia\site\v0.7\Distributed\src\process_messages.jl:56
  macro expansion at C:\projects\julia\usr\share\julia\site\v0.7\Distributed\src\process_messages.jl:269 [inlined]

I'll set-up a windows system tomorrow and debug this.

@simonbyrne
Copy link
Contributor

You can also RDP to an appveyor job: https://www.appveyor.com/docs/how-to/rdp-to-build-worker/

@maleadt
Copy link
Member Author

maleadt commented Jan 27, 2018

OK, this was a bug on Windows indeed: I was using joinpath, while libgit2 expects Unix path separators. Didn't know debugging Appveyor was such a pain though; the stacktrace was clear (ie. no RemoteException) after RDP'ing into the builder.

Only remaining CI failure is a seemingly unrelated ProcessExitedException on CircleCI; @KristofferC is it OK if I merge this before #25706?

@KristofferC
Copy link
Member

@KristofferC is it OK if I merge this before #25706?

I've rebased #25706 N times, so rebasing it N+1 times is no problem.

@maleadt maleadt merged commit e4d4529 into master Jan 27, 2018
@maleadt maleadt deleted the tb/treewalk branch January 27, 2018 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@StefanKarpinski StefanKarpinski StefanKarpinski left review comments

@ararslan ararslan ararslan left review comments

@bramtayl bramtayl bramtayl left review comments

@kshyatt kshyatt kshyatt approved these changes

@simonbyrne simonbyrne Awaiting requested review from simonbyrne

Assignees
No one assigned
Labels
libgit2 The libgit2 library or the LibGit2 stdlib module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@maleadt @StefanKarpinski @simonbyrne @KristofferC @kshyatt @ararslan @bramtayl