Convenience API and bug fixes for git tree walking by maleadt · Pull Request #25432 · JuliaLang/julia

maleadt · 2018-01-06T15:49:44Z

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 · 2018-01-06T15:50:26Z

base/libgit2/oid.jl

    GitHash(ccall((:git_object_id, :libgit2), Ptr{UInt8}, (Ptr{Cvoid},), obj.ptr))
 end

+==(obj1::GitObject, obj2::GitObject) = GitHash(obj1) == GitHash(obj2)


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

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

ararslan · 2018-01-06T19:59:16Z

base/libgit2/tree.jl

 end
+
+"""
+    getindex(tree::GitTree, target::AbstractString)::GitHash


Base still uses -> to show return types in docstrings

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

maleadt · 2018-01-11T12:35:56Z

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

StefanKarpinski · 2018-01-11T17:04:02Z

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

kshyatt · 2018-01-22T23:39:19Z

base/libgit2/tree.jl

+"""
+    getindex(tree::GitTree, target::AbstractString) -> GitObject
+
+Look up `target` path in the `tree`, returning a `GitObject` (a `GitBlob` in the case of a


X-refs here?

kshyatt

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

maleadt · 2018-01-23T13:57:23Z

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

StefanKarpinski · 2018-01-23T15:19:28Z

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 · 2018-01-23T15:24:31Z

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 · 2018-01-25T16:57:40Z

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 · 2018-01-25T17:36:56Z

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

maleadt · 2018-01-27T15:57:33Z

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 · 2018-01-27T16:28:41Z

@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 added the libgit2 The libgit2 library or the LibGit2 stdlib module label Jan 6, 2018

maleadt commented Jan 6, 2018

View reviewed changes

ararslan reviewed Jan 6, 2018

View reviewed changes

ararslan requested review from kshyatt and simonbyrne January 6, 2018 20:00

maleadt force-pushed the tb/treewalk branch 2 times, most recently from 0a16f68 to a0e9e40 Compare January 7, 2018 16:57

kshyatt reviewed Jan 22, 2018

View reviewed changes

kshyatt approved these changes Jan 22, 2018

View reviewed changes

maleadt added 4 commits January 23, 2018 07:49

Compare GitObject objects using their GitHash.

803fd5b

Allow nonzero return codes from a treewalk callback.

f947d2a

Put the LibGit2 test file in a test directory.

03a69cd

Add convenience getindex method for accessing tree entries.

d2eec50

maleadt force-pushed the tb/treewalk branch from a0e9e40 to d2eec50 Compare January 23, 2018 06:49

maleadt force-pushed the tb/treewalk branch from 7a0c38d to e330ced Compare January 26, 2018 10:47

Don't use Windows path separators with libgit2.

d6f65ba

maleadt force-pushed the tb/treewalk branch from e330ced to d6f65ba Compare January 26, 2018 11:39

maleadt merged commit e4d4529 into master Jan 27, 2018

maleadt deleted the tb/treewalk branch January 27, 2018 17:52

Uh oh!

Conversation

maleadt commented Jan 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maleadt Jan 6, 2018

Choose a reason for hiding this comment

Uh oh!

ararslan Jan 6, 2018

Choose a reason for hiding this comment

Uh oh!

ararslan Jan 6, 2018

Choose a reason for hiding this comment

Uh oh!

bramtayl Jan 7, 2018

Choose a reason for hiding this comment

Uh oh!

StefanKarpinski Jan 7, 2018

Choose a reason for hiding this comment

Uh oh!

maleadt commented Jan 11, 2018

Uh oh!

StefanKarpinski commented Jan 11, 2018

Uh oh!

kshyatt Jan 22, 2018

Choose a reason for hiding this comment

Uh oh!

kshyatt left a comment

Choose a reason for hiding this comment

Uh oh!

maleadt commented Jan 23, 2018

Uh oh!

StefanKarpinski commented Jan 23, 2018

Uh oh!

maleadt commented Jan 23, 2018

Uh oh!

maleadt commented Jan 25, 2018

Uh oh!

simonbyrne commented Jan 25, 2018

Uh oh!

maleadt commented Jan 27, 2018

Uh oh!

KristofferC commented Jan 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

maleadt commented Jan 6, 2018 •

edited

Loading