I believe to have found something that you missed in this PR. Note that git2::util::zeroed_raw_oid does not take any arguments:

I believe that this means that the kind: c_uchar (where c_uchar is defined as u8) member of the returned libgit2_sys::git_oid will have the value 0u8.

The function git2::util::zeroed_raw_oid is called by git2::Oid::zero, which also takes no arguments. If the function git2::Oid::object_format is called on the result of git2::Oid::zero, the call

will hit the panic! at

This is because 0u8 as u32 (which we got from std::mem::zeroed) is neither GIT_OID_SHA1 = 1, nor GIT_OID_SHA256 = 2.

I discovered this experimenting in code that is downstream of git2, building against this PR. In one of my tests I did essentially git2::Oid::zero().object_format().

Then I started to think about how much unsafe really is necessary here. Maybe zeroed_raw_oid could look like

#[inline]
pub(crate) fn zeroed_raw_oid(format: ObjectFormat) -> raw::git_oid {
  raw::git_oid {
    kind: format.raw(),
    id: [0; raw::GIT_OID_MAX_SIZE],
  }
}

In fact one might even ask why not to have

impl Oid {
  pub const ZERO_SHA1: Self = Self {
    raw: raw::git_oid {
      kind: raw::GIT_OID_SHA1,
      id: [0; raw::GIT_OID_MAX_SIZE],
    }
  };
  
  pub const ZERO_SHA256: Self = Self {
    raw: raw::git_oid {
      kind: raw::GIT_OID_SHA256,
      id: [0; raw::GIT_OID_MAX_SIZE],
    }
  };
  
  // NOTE: Maybe call this `zero_with_object_format` to not break `zero`.
  pub fn zero(format: ObjectFormat) -> Self {
    // TODO: Match on `format` and return `Oid::ZERO_SHA1` or `Oid::ZERO_SHA256`.
  }
}

This way, users that know at compile time which kind of zero they want can just use the constant, while those that decide at run time would call zero. And there is no unsafe involved.

If you do not want git2::Oid::zero to take an argument for backwards compatibility, then you could have it return a SHA-1 object identifier always, and mark it deprecated in favor of these constants and a new zero_with_object_format (if I have not missed a reason why the constants cannot be used).

For oid.rs I also had the gut feeling that some unsafe could be removed, but I do not have any constructive comments here, sorry.

Another comment regarding FromStr: I understand you opted to remove it to "avoid misuse", but I would argue that having FromStr actually is quite convenient. Instead of removing it, you could consider implementing FromStr in a way that it checks the length of the argument and then calls into the more specialized function that takes a string and an object format. The caller can then still inspect the object format of the result. You leave all options open for your users, and keep backwards compatibility.

Another comment regarding FromStr: I understand you opted to remove it to "avoid misuse", but I would argue that having FromStr actually is quite convenient. Instead of removing it, you could consider implementing FromStr in a way that it checks the length of the argument and then calls into the more specialized function that takes a string and an object format. The caller can then still inspect the object format of the result. You leave all options open for your users, and keep backwards compatibility.

This is still ambiguous for those not 41-64 char hex strings. I'd still consider deprecating it and eventually remove.
Alternatively, we just keep it there and have a compatibility note.

@DanielEScherzer most of your review comments are addressed. Thanks!

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.