Skip to content

Update libgit2 submodule. #503

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

Merged
merged 1 commit into from
Dec 12, 2019
Merged

Conversation

nossralf
Copy link
Contributor

@nossralf nossralf commented Dec 6, 2019

This updates the libgit2 submodule to latest master.

As several public struct members as well as some functions' parameters/return values have changed fundamental types (e.g from git_off_t which is an int64_t to git_object_size_t which is an uint64_t) I bumped the minor versions of both libgit2-sys and git2-rs (meaning it's equivalent to a major version bump for 0.x versions). As I'm not familiar with the version increment policy, let me know if that's wrong.

@nossralf nossralf force-pushed the update-libgit2 branch 3 times, most recently from 8d9c278 to 9e32ec1 Compare December 6, 2019 22:48
@nossralf
Copy link
Contributor Author

nossralf commented Dec 6, 2019

I guess I'll have to get a VM with Windows up and running tomorrow to figure out why the Windows build fails.

@nossralf nossralf force-pushed the update-libgit2 branch 2 times, most recently from 8bd0a30 to 1989b93 Compare December 7, 2019 19:18
@nossralf
Copy link
Contributor Author

nossralf commented Dec 7, 2019

The change from enum to uint32_t for flags in some structs is what tripped me up. On Windows with msvc, where git_enum! uses i32, the raw enum values must be cast to u32 for rustc to be satisfied.

The libgit2 pull request which introduced this change mentions the ergonomic downsides for C++ that results from using an uint32_t.

As the usage of raw flag values is hidden behind methods in the Rust API, the numeric casts don't affect users of git2-rs unless they are using raw values directly.

@alexcrichton
Copy link
Member

Everything looks great to me here thanks!

For the enum changes, could the type definitions for each type be switched to u32 all platforms too? That should remove the need ideally for the as cast when using the named constant values.

@nossralf
Copy link
Contributor Author

nossralf commented Dec 9, 2019

I tried to do that (by using pub enum git_merge_flag_t: u32 in the git_enum! definition for all the affected enums) but it led to errors on Windows for the systest suite related to the enums.

@alexcrichton
Copy link
Member

Sorry for the delayed response, but could you gist what the errors look like in that case?

@nossralf
Copy link
Contributor Author

This is the output I get when I set the git_enum! type to u32 explicitly for these three enums.

RUNNING ALL TESTS
bad git_merge_flag_t signed: rust: 0 (0x0) != c 1 (0x1)
bad git_merge_file_flag_t signed: rust: 0 (0x0) != c 1 (0x1)
bad git_stash_apply_flags signed: rust: 0 (0x0) != c 1 (0x1)

The tests that fail all seem to be generated as such, with just the enum name changed:

#[inline(never)]
#[allow(non_snake_case)]
fn sign_git_stash_apply_flags() {
  extern {
    #[allow(non_snake_case)]
    fn __test_signed_git_stash_apply_flags() -> u32;
  }
  unsafe {
    same(((!(0 as git_stash_apply_flags)) < (0 as git_stash_apply_flags)) as u32,
      __test_signed_git_stash_apply_flags(), "git_stash_apply_flags signed");
  }
}

I don't understand what information the systests are generated from, but that a test that covers signedness fails when the type is set to unsigned at least makes some sense based on the name alone. (If it makes sense to someone who understands the tests more deeply is unclear to me.)

@alexcrichton
Copy link
Member

Ah ok no worries! I think that the definitions may be a bit odd at the C layer, but that's ok. Let's go ahead with this and we can improve it later if necessary!

@alexcrichton alexcrichton merged commit a80e6ea into rust-lang:master Dec 12, 2019
@nossralf nossralf deleted the update-libgit2 branch December 12, 2019 07:39
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.

2 participants