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

WIP: ( feat: create function to allow setting commit_create_cb while rebasing ) #1047

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Pranav2612000
Copy link

@Pranav2612000 Pranav2612000 commented Apr 28, 2024

Fixes #850

@Pranav2612000
Copy link
Author

I'll make the pipelines pass but I just wanted to get an early review on the approach.
Do you think this could work? The commit_create_cb_op expects a callback fn which has arguments slightly different from the one expected by raw.commit_create_cb ( e.g Oid instead of *mut git_oid, Signature instead of *const git_signature ) but the rust typechecker did not throw an error.

@CouleeApps
Copy link

Just throwing my opinion on here since I was tagged on the issue: I don't think that transmute is safe (the rust parameter types probably don't map cleanly to c ffi). I would use something like this: (example from my job) https://github.com/Vector35/binaryninja-api/blob/dev/rust/src/debuginfo.rs#L181-L246 where Rust puts all of its information into a Box type provided to the userdata (void*) parameter of the callback, and a static callback function which takes the boxed data and interprets it properly.

@ehuss
Copy link
Contributor

ehuss commented May 1, 2024

remote_callbacks.rs has some examples for handling callbacks (using an extern "C" callback with a void * payload for the function pointer). That might provide some inspiration.

@Pranav2612000
Copy link
Author

Thank you for the help @CouleeApps @ehuss
I'll take a look at the code you shared and take another stab at it soon

@Pranav2612000 Pranav2612000 force-pushed the feat/add-support-for-commit-create-cb branch from 066ba42 to 494c2b1 Compare May 11, 2024 11:17
@Pranav2612000 Pranav2612000 force-pushed the feat/add-support-for-commit-create-cb branch from 494c2b1 to 5b2d0cb Compare May 11, 2024 13:04
@Pranav2612000
Copy link
Author

@ehuss I've followed an approach similar to the one used in remote_callback.rs. Can you take a look again?

@Pranav2612000
Copy link
Author

Also, I'm testing the changes through an application I have and passing GITPASSTHROUGH works as expected, but passing other values, which should result in the stopping and returning a failure crashes the app. The error is inside rebase.commit func. Here's a sample code I'm using

                if let Ok(commit_id) = rebase.commit(None, &committer.clone().into(), None) {
                    last_rebase_head = commit_id.into();
                } else {
                    rebase_success = false;
                    break;
                }

I've added print statements and ensured that it doesn't reach the else block before crashing.

Do you know what may be happening?

@Pranav2612000
Copy link
Author

Bumping this up

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.

Support git_commit_create_cb in RebaseOptions
3 participants