Support separate payloads for RemoteCallbacks #26437
Conversation
|
cc: @KristofferC |
stdlib/LibGit2/src/types.jl
Outdated
| @@ -206,6 +206,27 @@ abstract type Payload end | |||
| payload::Ptr{Cvoid} | |||
| end | |||
|
|
|||
| @kwdef struct CallbackPayload | |||
There was a problem hiding this comment.
I think I'll rename this to RemotePayloads
stdlib/LibGit2/src/LibGit2.jl
Outdated
| @@ -275,8 +275,9 @@ function fetch(repo::GitRepo; remote::AbstractString="origin", | |||
| GitRemoteAnon(repo, remoteurl) | |||
| end | |||
| result = try | |||
| fo = FetchOptions(callbacks=RemoteCallbacks(credentials=credentials_cb(), payload=p)) | |||
| fetch(rmt, refspecs, msg="from $(url(rmt))", options = fo) | |||
| callbacks = RemoteCallbacks(credentials=(credentials_cb(), pointer_from_objref(p))) | |||
There was a problem hiding this comment.
pointer_from_objref is fishy here since it allows p to be gc'd,
that's why original code uses Ref.
There was a problem hiding this comment.
Too brittle and error-prone. This code really just needs to avoid using Ptr{Void} and stick with Ref.
stdlib/LibGit2/src/types.jl
Outdated
| push_transfer_progress::Ptr{Cvoid} | ||
| push_update_reference::Ptr{Cvoid} | ||
| push_negotiation::Ptr{Cvoid} | ||
| transport::Ptr{Cvoid} |
There was a problem hiding this comment.
These are Julia handles. They should not be typed as opaque C handles. It looks like the input is a Dict, so RemotePayloads can also just be a Dict{Symbol, Any}.
There was a problem hiding this comment.
Should I use Any or Ref{Any}. We could do either.
omus
force-pushed
the
cv/libgit2-remote-callbacks
branch
from
d967cbe to
297d998
Compare
|
I can't seem to reproduce the CI failures locally. I'll push part of my work to try and sort out what's going wrong. |
|
Any update here? |
|
Sorry, I've been on a work trip this last week. I'm planning on making this my highest priority when I get back. |
LibGit2 functions `clone`, `fetch`, and `push` all used a keyword named `payload` which would only take credential information. The keyword was renamed to `credentials` to be more accurate and the original keyword is now deprecated.
Make it easier to support mixing user created callbacks and default callbacks defined by LibGit2.
omus
force-pushed
the
cv/libgit2-remote-callbacks
branch
from
dfc8548 to
7dba146
Compare
|
I spent some more time with these changes and decided that it would be best to have high-level clone/fetch/push function support user callbacks. To me it makes the most sense to have this interface support having each callback be able to have an independent payload. |
|
macOS failure on Travis looks to be a DNS issue. I'll have some tests shortly for this PR. |
|
Wow, I passed the CI gauntlet 😄. The PR should be ready to go now. The tests aren't exhaustive but they should cover the basics. |
|
@KristofferC, do you want to review this or should we just merge? |
Spin off of #26387. Support having separate payloads for each callback in
RemoteCallbacks. Specifically this change was made to support using the credential callback included in LibGit2.jl and the progress bar proposed in JuliaLang/Pkg.jl#177.There is a little work yet to do including:
RemoteCallbackswhere tuples of callbacks/payloads are passed in