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.

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

Enable git_push_transfer_progress - Help wanted #1500

Merged
merged 9 commits into from Sep 25, 2018
Merged

Enable git_push_transfer_progress - Help wanted #1500

merged 9 commits into from Sep 25, 2018

Conversation

mitchheddles
Copy link
Contributor

@mitchheddles mitchheddles commented May 18, 2018

Have enabled the pushTransferProgress callback for RemoteCallbacks, which is used in Remote.push.

The docs for PushOptions says it accepts callbacks as RemoteCallbacks which is correct, but transferProgress is only for downloads, libgit has separate API for uploads.

I've been able to get the callback (pushTransferProgress) working, similar to transferProgress (with throttling). I just need help finishing it off.

Current Issues

Generate - The generate callback function uses a regex pattern (/\s*_(cb|fn)/) against the function name which isn't satisfied by libgit function name git_push_transfer_progress. For now, I've just put in a strict check to bypass.

Testing - I'm unable to get the test case working... I keep getting credentials errors. I was able to get it working by setting up a local bare repo and have the test push to that.

Docs - May need to update the docs to reflect the changes.

It would be great to get some help finishing this off.

Cheers!

@rcjsuen
Copy link
Member

rcjsuen commented May 18, 2018

Perhaps this is something the libgit2-supplement.json file can help with overriding but I've never added any content to that file myself.

@mitchheddles
Copy link
Contributor Author

mitchheddles commented May 19, 2018

@rcjsuen The overrides, from the supplement file, are applied after the Helpers.isCallbackFunction is called on the field. I tried the approach you put forward, but it failed to build with the following error:

            wrapper->raw->push_transfer_progress = (git_push_transfer_progress_cb)push_transfer_progress_cppCallback;
                                                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                    git_push_transfer_progress
../vendor/libgit2/include/git2/remote.h:351:15: note: 'git_push_transfer_progress' declared here
typedef int (*git_push_transfer_progress)(```

I tried a couple of variations based on your code, but haven't managed to get past the build.
Perhaps there's somebody who better understands the generators that could help out?

@mitchheddles
Copy link
Contributor Author

@rcjsuen I've got it working using the supplement file and changing type to include _cb. I'm still not sure if this is the correct approach, but it is working.

I will get onto the test case shortly.

@mitchheddles
Copy link
Contributor Author

mitchheddles commented May 23, 2018

@rcjsuen I've setup the test using a local bare repository... Once again, I'm not sure if the approach is correct, but it works.

Update 2: Tried building from a clean repo and the build/test failed.

Update: Forgot to credit your advice, sorry... I'm now looking into the build errors.

It seems that the culprit is:

"push_transfer_progress": {
    "dupFunction": "git_transfer_progress_dup"
}

@mitchheddles
Copy link
Contributor Author

Have rewritten the isCallbackFunction to accept a second argument to override the regex test.

This means we can now declare a callback from the libgit2-supplement.json file using isCallback.

I hope this approach is alright, I can't see another way around it. I'm also not sure why libgit have named the function without _cb.

@rcjsuen Any suggestions for moving this forward?

@mitchheddles
Copy link
Contributor Author

@rcjsuen All checks have passed now, is there any chance of pushing this one through?

Copy link
Member

@implausible implausible left a comment

Choose a reason for hiding this comment

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

Looking really good, just found this one issue.

@@ -2918,6 +2915,9 @@
"transfer_progress": {
"dupFunction": "git_transfer_progress_dup"
},
"push_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.

I don't believe you need this block. There is no git_push_transfer_progress struct, and it is also theoretically would not compile if that struct was passed to the dupFunction you declared here.

@mitchheddles
Copy link
Contributor Author

@implausible No worries, I've made the changes.

@implausible
Copy link
Member

Thanks!

@implausible implausible merged commit d9e63b9 into nodegit:master Sep 25, 2018
@codercatdev
Copy link

codercatdev commented Dec 20, 2018

Any word on this merge going live?

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.

None yet

4 participants