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

libgit2: Bump to v1.8.0 #1032

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

bnjmnt4n
Copy link

@bnjmnt4n bnjmnt4n commented Mar 2, 2024

This PR bumps libgit2 to v1.8.0, in preparation for #1031. I've bumped libgit2-sys to 0.17.0+1.8.0, but haven't touched the git2 version since I thought that's better left for the maintainers.

I haven't included any new Rust bindings for the new functions (e.g. git_commit_create_from_stage). Basically I've just done the bare minimum required to get the libgit2 v1.8.0 working.

API changes:

  • Remote:update_tips now accepts a RemoteUpdateFlags which is an integer consisting of bitfields instead of a boolean
  • FetchOptions: add report_unchanged method which updates the update_flags bitflag (similar to above)
  • PushOptions: add remote_push_options method to set remote push options
  • WorktreeAddOptions: add checkout_existing method to set the boolean checkout_existing option

References

bnjmnt4n added a commit to bnjmnt4n/git2-rs that referenced this pull request Mar 2, 2024
This commit changes the original `ssh` feature into two new ones:
`ssh-libssh2` and `ssh-openssh`. By default, the `ssh-libssh2` feature
is enabled for backwards compatibility.

To use OpenSSH instead, the following listing in `Cargo.toml` can be
used:

    git2-rs = { version = "...", default-features = false, features = ["https", "ssh-openssh"] }

This commit is stacked on top of rust-lang#1032, and should only be merged after
libgit2 v1.8.0 has been released.

Closes rust-lang#1028.
bnjmnt4n added a commit to bnjmnt4n/git2-rs that referenced this pull request Mar 2, 2024
This commit changes the original `ssh` feature into two new ones:
`ssh-libssh2` and `ssh-openssh`. By default, the `ssh-libssh2` feature
is enabled for backwards compatibility.

To use OpenSSH instead, the following listing in `Cargo.toml` can be
used:

    git2-rs = { version = "...", default-features = false, features = ["https", "ssh-openssh"] }

This commit is stacked on top of rust-lang#1032, and should only be merged after
libgit2 v1.8.0 has been released.

Closes rust-lang#1028.
@bnjmnt4n
Copy link
Author

bnjmnt4n commented Mar 7, 2024

Looks like the current test failures are due to changes made in libgit2/libgit2#6743 👀

bnjmnt4n added a commit to bnjmnt4n/git2-rs that referenced this pull request Mar 17, 2024
This commit changes the original `ssh` feature into two new ones:
`ssh-libssh2` and `ssh-openssh`. By default, the `ssh-libssh2` feature
is enabled for backwards compatibility.

To use OpenSSH instead, the following listing in `Cargo.toml` can be
used:

    git2-rs = { version = "...", default-features = false, features = ["https", "ssh-openssh"] }

This commit is stacked on top of rust-lang#1032, and should only be merged after
libgit2 v1.8.0 has been released.

Closes rust-lang#1028.
@bnjmnt4n
Copy link
Author

Looks like v1.8.0 is out! I will update the PR to bump the version.

@bnjmnt4n bnjmnt4n changed the title libgit2: Bump to v1.8.0-alpha libgit2: Bump to v1.8.0 Mar 22, 2024
@bnjmnt4n bnjmnt4n marked this pull request as draft March 22, 2024 12:41
@bnjmnt4n bnjmnt4n marked this pull request as ready for review March 22, 2024 13:24
@bnjmnt4n
Copy link
Author

This should be ready for review. (I'm pretty new to Rust and the libgit2 / this codebase, please do let me know if there are better ways to do anything.)

The test failures on Windows are related to libgit2/libgit2#6743 as mentioned above—I'm not too sure how to handle these conventionally using Rust.

@@ -391,7 +402,7 @@ pub struct git_fetch_options {
pub version: c_int,
pub callbacks: git_remote_callbacks,
pub prune: git_fetch_prune_t,
pub update_fetchhead: c_int,
pub update_flags: c_uint,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I'm not really confident this is the correct way to handle packed bitflags. I had the understanding that Rust doesn't really support packed bitflags, at least in a compatible way with C, since it depends on the compiler. Do you have some more information on why this might be safe?

Copy link
Author

Choose a reason for hiding this comment

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

You're right, I wasn't familiar with C bitfields, I originally thought they were just a convenient way of specifying bit-flags which are handled in a similar way to bitmasks for unsigned ints. It does seem hard to support since their memory layout is compiler dependent. I was thinking since we know the number of bits set, assuming that bits are packed only either left-to-right or right-to-left, we could do something like update_flag.bits() | update_flag.bits().reverse_bits() when getting the unsigned int as a workaround? (This admittedly doesn't work if the memory layout is different.)

Copy link
Contributor

Choose a reason for hiding this comment

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

@ehuss whats the way to interact with c bitfields via FFI?

Copy link
Author

Choose a reason for hiding this comment

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

There is no official support for C bitfields in Rust: rust-lang/rfcs#314.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no good way to use bitfields from Rust's FFI, and that won't be fixed in a timeframe reasonable for updating this library, so I suggest hacking around it.

This field will have a correct size — the C header explicitly specifies it as unsigned int, so the risk of problems seems relatively low. In the worst case these options won't be set, but that's just an application-level nuisance, not memory corruption.

AFAIK C treats unused bits as padding, so update_flag.bits() | update_flag.bits().reverse_bits() should be safe.

If that's still too risky, then the correct way to deal with this would be to compile and link a C file that exports Rust-FFI-compatible getter and setter functions for these fields. Perhaps do that only when building the library from vendored source to avoid pains of with finding correct headers and ensuring they actually match .so lib's ABI?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I haven't had much time lately. I've been intending to push a change that will use a C wrapper to get/set the fields, I just haven't gotten to it, yet. I don't feel comfortable trying to guess the layout from within Rust. @bnjmnt4n If you have time to give that a shot, that would be helpful. Otherwise, I'll try to look into it soonish.

Copy link
Contributor

@kornelski kornelski Apr 27, 2024

Choose a reason for hiding this comment

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

@ehuss @bnjmnt4n The maintainer of libgit2 said he can guarantee the ABI will use least significant bits for the flags: libgit2/libgit2#6800

Copy link
Contributor

Choose a reason for hiding this comment

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

Status update: ethomson mentioned in libgit2/libgit2#6800 that they will look to see if there is some workaround for us.

@ehuss
Copy link
Contributor

ehuss commented Mar 23, 2024

The test failures on Windows are related to libgit2/libgit2#6743 as mentioned above—I'm not too sure how to handle these conventionally using Rust.

Hm, that's awkward. I don't really understand why TempDir is returning short path. One option is to canonicalize both paths before comparing they are equal. Another option is to use same-file to check that the paths are the same. Canonicalize might be easier if it works.

@jirutka
Copy link

jirutka commented Apr 3, 2024

FYI, this blocks the upgrade of libgit2 to 1.8.0 in Alpine Linux (!63296).

@SS1823
Copy link

SS1823 commented Apr 23, 2024

Is this PR still blocked?
this may fix actions/checkout#1689

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

6 participants