Skip to content

Conversation

KristofferC
Copy link
Member

src/GitTools.jl Outdated

function transfer_progress(progress::Ptr{GitTransferProgress}, p::Any)
progress = unsafe_load(progress)
bar = unsafe_pointer_to_objref(p.transfer_progress)
Copy link
Member Author

@KristofferC KristofferC Mar 13, 2018

Choose a reason for hiding this comment

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

@omus I would have assumed that the payload sent to this function was supposed to be the p.transfer_progress instead of the whole struct (since those are given together to RemoteCallbacks).

Copy link
Member

@omus omus Mar 13, 2018

Choose a reason for hiding this comment

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

I forgot to update this code with my Pkg3.jl PR. This will need to be changed to:

transfer_progress_ptr = LibGit2.payload_unpack(p, :transfer_progress)
@assert transfer_progress_ptr != C_NULL
bar = unsafe_pointer_to_objref(transfer_progress_ptr)

At the moment if you use the tuple syntax for RemoteCallbacks it breaks apart the tuple and puts each of the payloads inside of a RemotePayloads container. I decided that having each callback unpack this object was the better solution instead of unwrapping it before calling the user's callback.

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided that having each callback unpack this object was the better solution instead of unwrapping it before calling the user's callback.

Alright.

Copy link
Member Author

@KristofferC KristofferC Mar 13, 2018

Choose a reason for hiding this comment

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

@omus

payload_unpack(p, :transfer_progress)

doesn't work because p is an instance of RemotePayloads and not a pointer to it (like payload_unpack expects). I just added a p.transfer_progress != C_NULL check.

@omus
Copy link
Member

omus commented Mar 13, 2018

I would have assumed that the payload sent to this function was supposed to be the p.transfer_progress instead of the whole struct (since those are given together to RemoteCallbacks).

I could change JuliaLang/julia#26437 to handle this more transparently. The reason I didn't go in this direction is because it would involve an extra cfunction which would intercept the callback from libgit2, unpack the payload, then execute the original C call.

@KristofferC
Copy link
Member Author

In some sense that would be "cleaner" in that your callback get exactly the corresponding payload that you specified. On the other hand, maybe you want to look at the payloads for all the other callbacks? Doesn't matter too much to me :)

src/GitTools.jl Outdated

progress = unsafe_load(progress)
@assert p.transfer_progress != C_NULL
bar = unsafe_pointer_to_objref(p.transfer_progress)
Copy link
Member

Choose a reason for hiding this comment

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

If you define this function as transfer_progress(progress::Ptr{GitTransferProgress}, bar::MiniProgressBar) you can perform the unsafe operations in another method like:

function transfer_progress(progress::Ptr{GitTransferProgress}, p::LibGit2.RemotePayloads)
    transfer_progress(progress, unsafe_pointer_to_objref(p.transfer_progress))
end

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the advantage?

Copy link
Member

Choose a reason for hiding this comment

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

May make the code slightly cleaner

@KristofferC
Copy link
Member Author

I'll rebase this when nightlies are updated.

@StefanKarpinski
Copy link
Member

This is a pretty slick feature 😁

@KristofferC
Copy link
Member Author

KristofferC commented Mar 30, 2018

Mac and Linux nightlies are up, tests are passing, things are awesome 🎉.

Thanks @omus, this is one nice milestone for improving the user-friendliness of the Julia package manager!

@KristofferC KristofferC merged commit 1f8b80a into master Mar 30, 2018
@KristofferC KristofferC deleted the kc/repo_progress branch March 30, 2018 22:28
martinholters pushed a commit that referenced this pull request May 15, 2018
* show a progress bar when cloning git repos and add credential handling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants