Skip to content

bundle: source object hash from repo.object_hash() not Kind::Sha1 #266

Description

@dekobon

src/bundle.rs names gix_hash::Kind::Sha1 as a literal in two places
instead of asking the repository which object hash it uses. Both call
sites already hold an open gix::Repository, and gix exposes
Repository::object_hash() -> gix_hash::Kind (gix 0.83,
src/repository/config/mod.rs). The literal should be replaced by that
accessor.

Background: why it is SHA-1 today

This is not a runtime decision — it is a compile-time feature
exclusion that nothing in the repo documents as deliberate.

Cargo.toml pins:

gix = "0.83"
gix-hash = { version = "0.25", features = ["sha1"] }

In gix-hash 0.25.1 the Kind variants are individually cfg-gated:

#[cfg(feature = "sha1")]   Sha1   = 1,
#[cfg(feature = "sha256")] Sha256 = 2,

and gix 0.83 exposes sha256 = ["gix-hash/sha256"] as an opt-in
feature this crate does not enable. So in our build Kind has exactly
one inhabited variant and gix_hash::Kind::Sha1 is the only value that
can be named.

The comment above the pin in Cargo.toml explains why sha1 is
enabled (without it Kind is uninhabited and every match fails to
compile). Nothing anywhere explains why sha256 is excluded. That
reads as an unrevisited default rather than a decision.

Why change it

  • No behaviour change today. With sha256 off, repo.object_hash()
    can only return Kind::Sha1. The change is free.
  • Provenance. The value stops being an assertion the code makes and
    becomes a fact the code reads. If the sha256 feature is ever
    enabled, bundle.rs is already correct rather than silently writing a
    SHA-1-width trailer into a SHA-256 pack.
  • Upstream readiness. gix has no bundle reader/writer
    (crate-status.md still lists [ ] bundles; upstream issue
    feat: fetch and clone from git bundle files GitoxideLabs/gitoxide#2633 requests it). If any of src/bundle.rs is
    ever offered upstream, sourcing the hash kind from the repository is a
    hard requirement — gitoxide plumbing never hardcodes an object hash.
    Doing it now keeps the delta small.

Scope

In scope:

  • bundle::create — the gix_hash::Kind::Sha1 argument to
    FromEntriesIter::new.
  • bundle::unbundle — the object_hash field of
    gix_pack::bundle::write::Options.
  • The doc-comment on git::Sha, which claims "SHA-1 object OID,
    displayed as 40 lowercase hex characters"
    . The type wraps
    gix_hash::ObjectId and Sha::from_hex delegates straight to
    ObjectId::from_hex, which accepts any known hash width. The comment
    asserts an invariant the type does not enforce. Academic while
    sha256 is off, but it should not claim what it does not check.

Explicit non-goals:

  • Enabling the sha256 feature. Out of scope. It would not work
    anyway: keys::is_valid_bundle_stem requires a 40-character stem and
    packchain::schema::Sha40 is serde-validated at exactly 40 lowercase
    hex on every parse of chain.json / path-index.json. Those are
    on-bucket format constraints, not code constants, and a SHA-256 repo
    would fail at the first manifest write. Real SHA-256 support is a
    separate, much larger issue.
  • The packchain call sites (packchain::pack,
    packchain::read). Several of those parse pack trailers and idx
    entries where the hash width has to come from the chain manifest, so
    they need the hash kind threaded through the schema first. They are
    blocked on the format work above.
  • Unit-test fixtures that construct gix_pack::index::File with an
    explicit Kind::Sha1. Those build SHA-1 repos on purpose; naming the
    kind there is correct.

Resolution Plan

  1. In bundle::create, read the hash kind from the already-open
    repository (repo.object_hash()repo is borrowed, not consumed,
    by the ODB clone and stays live through the whole function) and pass
    it as the final argument to FromEntriesIter::new in place of
    gix_hash::Kind::Sha1.
  2. In bundle::unbundle, do the same for the object_hash field of
    gix_pack::bundle::write::Options, reading from the repo opened
    earlier in the function.
  3. Drop the now-unused gix_hash::Kind import from src/bundle.rs if
    no other reference remains, so the sha1 assumption has no residual
    mention in the module.
  4. Rewrite the git::Sha doc-comment so it describes what the type
    actually guarantees — a gix_hash::ObjectId of whatever width the
    repository's object hash produces, rendered lowercase-hex by
    Display — instead of asserting SHA-1 / 40 characters. Note in the
    comment that the 40-character constraint lives in
    keys::is_valid_bundle_stem and packchain::schema::Sha40, which is
    where it is actually enforced.
  5. Add a unit test in src/bundle.rs asserting that a bundle created
    from a fixture repository round-trips through unbundle and that the
    installed pack's index reports repo.object_hash() as its hash kind
    — so the wiring is exercised rather than merely compiled. Reuse the
    existing empty_repo / add_commit helpers.
  6. Verify the existing git-interop tests still pass
    (native_bundle_create_accepted_by_git, and the git bundle create
    → native unbundle direction), since they are the real guard on
    on-disk format parity.
  7. Run cargo fmt, cargo clippy --all-targets, and cargo test. No
    CHANGELOG.md entry is required — there is no observable behaviour
    change — but add one under Changed if the git::Sha doc rewrite is
    judged user-visible for the published rustdoc.

Assessment

Dimension Rating
Difficulty Low
Complexity Low
Priority Low

Difficulty — Low. Two argument substitutions, one doc-comment
rewrite, one test. Well under 50 lines.

Complexity — Low. One module plus a doc-comment in src/git.rs. No
public API signature changes, no on-bucket format impact, no
cross-module coordination.

Priority — Low. Zero behaviour change while the sha256 feature is
off, which it is and will stay for the foreseeable future. The value is
provenance and upstream-readiness, not correctness of any shipping path.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    code-qualityCode quality, refactoring, or maintainabilitylow-priorityLow priority issuerustPull requests that update rust code

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions