feat: add sha256 feature to gix-commitgraph

[cruessler]

This PR adds the sha256 feature to gix-commitgraph. I opened it as a draft, so we have a space where we can discuss how to best approach testing this new feature.

This PR was triggered by a comment in #2359.

[Byron]

Thanks a lot, I am looking forward to sorting this out!

There is also #2378, hopefully done in the next couple of days (it might conflict).

[Byron]

Thanks a lot for starting this! This crate is special as it's the first one that parses a file-format that changes depending on the hash kind.

It seems like one commit currently in the gix-object PR would be useful here as well, in case you want to bring it up in its own PR.

[cruessler]

Assuming you’re referring to the one adding Kind::all (6d950f0), I’ll extract that addition into its own PR!

Apart from that, my plan is to go through the tests and, for every tests that currently uses sha1, add a corresponding one that uses sha256. Is that the way to go? Are there other things that would need to be done in this PR?

[Byron]

Assuming you’re referring to the one adding Kind::all (6d950f0), I’ll extract that addition into its own PR!

Yes, that's the one, thanks 🙏.

Apart from that, my plan is to go through the tests and, for every tests that currently uses sha1, add a corresponding one that uses sha256. Is that the way to go? Are there other things that would need to be done in this PR?

Sorry for the confusion, that's the way. The only thing tests have to do is to actively set all hashes using feature toggles, and they should just set sha1/sha256 while they are at it. Once a crate has been converted, the tests should automatically test both hashes, and make sure they are compiled in.

[cruessler]

I added fixtures for SHA256 hashes to gix-testtools and changed gix-commitgraph tests to run for all available hash kinds (those depend on the feature flags passed to gix-commitgraph). This seems to have worked on my machine, but I’m not sure I got the logic in gix-testtools right (any pointers would be much appreciated!). I also added gix-commitgraph to unit-test in justfile, and now I want to see what feedback CI has to give. :-)

[cruessler]

I think I’ve now gotten to a point where I need some feedback in order to be able to continue. (CI is still running, so I also might need to address any issue that comes up there.)

Specifically, I’ve got the following questions:

• The PR runs tests for both hashes through cargo nextest run -p gix-commitgraph --features sha256 --no-fail-fast. Is that correct?
• Last time CI ran it did not complain about any of the generated archives in gix-commitgraph, but when I delete both generated-archives as well as generated-do-not-edit in order to regenerate the archives, git status shows that most files have changed their size by 512 bytes and that one file has been both deleted and modified. (I’m running cargo test --features sha256 on Ubuntu 25.10.) What’s the best way to debug this?
• Is there anything else I would need to change or include before I can mark this PR as ready?

# output of `gix status` inside `gix-commitgraph/tests/fixtures`
modified:   generated-archives/generation_number_overflow.tar
modified:   generated-archives/generation_number_overflow_sha256.tar
modified:   generated-archives/octopus_merges.tar
modified:   generated-archives/single_commit.tar
modified:   generated-archives/single_commit_huge_dates.tar
modified:   generated-archives/single_parent.tar
deleted:    generated-archives/single_parent_huge_dates.tar
modified:   generated-archives/two_parents.tar

[Byron]

I think this is great!

The PR runs tests for both hashes through cargo nextest run -p gix-commitgraph --features sha256 --no-fail-fast. Is that correct?

I made a note in Cargo.toml which should lead to the extra run in the justfile to be removed. Please take a look to see if it makes sense.

Last time CI ran it did not complain about any of the generated archives in gix-commitgraph, but when I delete both generated-archives as well as generated-do-not-edit in order to regenerate the archives, git status shows that most files have changed their size by 512 bytes and that one file has been both deleted and modified. (I’m running cargo test --features sha256 on Ubuntu 25.10.) What’s the best way to debug this?

generated-do-no-edit would be the folder to delete for it to restore from archive or regenerate the local cache in generated-do-not-edit. That's all there is to it. With the sha256 versions showing up, I'd expect it to want to create the sha256 versions which seems to have happened as well. It seems to work from my PoV, but I will take a closer look when reviewing properly.
PS: removing generated-archives will regenerate them, and they always appear changed. That's normal, and one would avoid deleting the tracked files.

Is there anything else I would need to change or include before I can mark this PR as ready?

It looks like it's ready for a non-armchair and final review after this one 🎉.

[Byron]

While it would be more cluttered, I seem to prefer the idea of not nesting hash_kind, but add it to the format string here.

In future, there will be even more of these as another parameter, the one for the refs backend, is added.

Alternatively… what do you think about looking at both parameters and using them for nesting?

path.join(hash_kind).join(refs_kind)

The refs_kind is nothing that exists now, but it would be added at some point.

Yes, that seems like the way to go.

So let's make a mild breaking change and always add the hash_kind to the path via join.

[Byron]

Did you see this comment?

[cruessler]

My bad, sorry! I had seen the comment, but wasn’t 100 % sure what you had in mind at first, and then I forgot to address it. 😄 I think making the hash kind a part of the path is a good idea, though.

[Byron]

I think the remove here isn't needed if we are always overriding. If there is something special, then let's keep this and document it though.

[Byron]

Could all() rather return an iterator that hands out owned versions of hash_kind? I think that should work with impl Iterator<Item = Kind>.

[Byron]

I think we'd want to use dev-dependencies to always add sha256 to the features when testing. That way, there is only one way to run tests: all of them.

[cruessler]

That means removing the feature here, is that correct?

[Byron]

Yes, let's start with that.

This means we say that plumbing will never set hash features, and it expects to be able to see the hash to be used at runtime via gix-hash::Kind. Most implementations do that already, and I think this should work for most cases.

[cruessler]

Makes sense, removed.

[Byron]

As I am looking at Git for how they tackle this I wonder this: Should we, rather than changing every test and looping over it, prefer to build with all hashes, but run tests for each hash selectively, maybe by setting an environment variable that controls which hash is under test?

Performance-wise it should be very similar, even though development is a bit less convenient as one will have to run twice with different flags or environment variables set. There, having the tests run everything automatically, always, and what we have now, seems better.

Also, how do we handle expectations on hashes, particularly those with insta? It basically requires to chose one hash and find/replace the actual output to match that. I.e. insta::assert_debug_snapshot(hash.norm(actual), @r"expected with sha1"). And that remapping table, where would that come from? Is it passed in per function, or is it global? It can be global, and maybe that's how Git does it? I never checked, but it would be interesting to know.

So I think it's fine to keep this PR as is as it's happy with this simple and convenient solution, and we can probably layer moer on top of that when a need arises.

[cruessler]

I’m glad you mention this other option because I’m still very hesitant to force every test to be rewritten to make it loop over hash kinds as well. What I could try instead is adding an environment variable, e. g. GIX_TEST_HASH, that defaults to sha1 if not set, but can be changed to sha256. Any place that wants code to work regardless of hash kind wouldn’t have to bother, but could rely on CI as well as fixture script creation to take care of running with all hash kinds. I’ll try that approach in this PR!

[cruessler]

I’ve added a dev-depencency on gix-hash that activates the feature sha256, but I suspect that’s the reason why a couple of tests related to the size of objects now fail. At first sight, it looks like the feature is now enabled for every cargo nextest run. Apart from that, I think using an environment variable to set the hash per cargo nextest run works quite well.

[Byron]

I’m glad you mention this other option because I’m still very hesitant to force every test to be rewritten to make it loop over hash kinds as well. What I could try instead is adding an environment variable, e. g. GIX_TEST_HASH, that defaults to sha1 if not set, but can be changed to sha256. Any place that wants code to work regardless of hash kind wouldn’t have to bother, but could rely on CI as well as fixture script creation to take care of running with all hash kinds. I’ll try that approach in this PR!

That's fair, and probably that's best whenever there is no actual assertions against a hash.
When there is such assertions, one will have to touch these tests to have some sort of mapping, i.e. like with translations the test mentions only one kind of hash, probably SHA1, but it auto-translates to SHA256 when that's run.
Doing this ergonomically probably requires globals so one doesn't have to pass the hash kind in everywhere.

The manual looping is always something one can implement if that's a good choice in specific cases.

With such a system, we'd need just one more job to run the SHA256 version of the test suite, which just sets an environment variable.

I’ve added a dev-depencency on gix-hash that activates the feature sha256, but I suspect that’s the reason why a couple of tests related to the size of objects now fail. At first sight, it looks like the feature is now enabled for every cargo nextest run. Apart from that, I think using an environment variable to set the hash per cargo nextest run works quite well.

Oh, right, the nextest builds everything at once so has only one dependency tree for resolving feature flags.

If [dev-dependencies] would instead pull in gix-hash to enable one or more hashes (probably always all of them), then there would be no need for duplicating the hash related cargo features in the crate that uses gix-hash, and instead one would have to tell plumbing users to depend on gix-hash just to select hash features. This is what gix-features is there for, and maybe the hash selection could be added there as well.

In any case, it seems we'd have to find a way to get CI green. Something that I'd not want to lose is to see what the best possible size is, i.e. if just one hash is supported and that is the smallest hash. But then one would have to test the individual crate just with one hash kind which may also be difficult in future. So… maybe just change the size expectations to the new value, while also using [dev-dependencies] to enforce building with dual-hash support.

[Byron]

Oh, and what happens if the crate is now tested individually?
My feeling is that it needs its own feature toggles so that it knows what to test for… . If that's the case, I think we should add the respective statements to the justfile, every crate must be testable in isolation and work just the same.
Or… their tests can be adjusted to enforce all hashes as well, then there should be no difference. Probably my preference for now to keep it as simple as possible.

[cruessler]

We could add the following line to gix-pack/Cargo.toml which would make the failing test in gix-pack pass:

gix-hash = { version = "^0.22.0", path = "../gix-hash", features = ["sha256"] }

This would duplicate the dependency, though.

[Byron]

We could add the following line to gix-pack/Cargo.toml which would make the failing test in gix-pack pass:

Yes, such a line would be needed via [dev-dependencies], for all crates that had to be adjusted to compile with nextest.
On top of that, to play it safe, I think the justfile entry to test them individually would still be needed.

Lastly, it looks like there will never be a need to add sha1/sha256 feature toggle forwarding to crates that depend on gix-hash, so plumbing stays clean and expects its direct consumers to know they have to configure gix-hash on application level.

gix (the crate) would offer such a forwarding though with reasonable defaults. That seems quite minimal then.

[cruessler]

CI is now green! Quick summary of this PR’s changes:

• It adds GIX_TEST_HASH which affects the default hash used in fixture scripts via GIT_DEFAULT_HASH. (We might want to consider renaming to GIX_TEST_FIXTURE_HASH or something that makes it clearer this is related to fixtures.)
• It adapts size-of-various-objects-related expectations in tests so that the sizes always take both hash kinds into account.
• For that to work consistently, it adds gix-hash = { …, features = ["sha256"] } to dev-dependencies in several crates.

[Byron]

Exciting!

It adds GIX_TEST_HASH which affects the default hash used in fixture scripts via GIT_DEFAULT_HASH. (We might want to consider renaming to GIX_TEST_FIXTURE_HASH or something that makes it clearer this is related to fixtures.)

Yes, I'd also like that.

I left a comment, but ideally it's applied to all the numbers so we keep track of the SHA1 versions.

[Byron]

Yes, let's start with that.

This means we say that plumbing will never set hash features, and it expects to be able to see the hash to be used at runtime via gix-hash::Kind. Most implementations do that already, and I think this should work for most cases.

[Byron]

Could we record the old number? Maybe by doing:

let sha1 = 48;
let sha256_extra = 24
actual <= sha1 + sha256_extra

This will just help document how much memory can be saved by not compiling for SHA256.

[cruessler]

I like the idea and have changed all occurrences to explicitly contain that piece of information.

[Byron]

Great catch, thanks!

[Byron]

Did you see this comment?

[Byron]

Never mind, I think this is related to the comment at the top. What follows is what I wrote before.

---

Now I wonder: does gix-commitgraph still need its own feature? Or could it be told by gix_hash::Kind what it should expect?
My hope is that this can all be done with runtime values, and not forwarding sha1/sha256 features should make that clearer as well.

[Byron]

What surprises me is that there is no way to check what hash format the graph is using. Could we have an implementation of object_hash (which exists on File) for the Graph, and assert on the value.
But then we'd need a way to get the currently set hash, which had to come from gix-testtools.
I think this would be very useful to have and some assertion should definitely be made.

[cruessler]

Are you thinking about something like the following:

/// The kind of hash used in this `Graph`.
///
/// Note that it is always conforming to the hash used in the owning repository.
///
/// # Panics
///
/// If the graph does not contain any `File`.
pub fn object_hash(&self) -> gix_hash::Kind {
    self.files
        .first()
        .map(super::File::object_hash)
        .expect("graph to have at least one file")
}

[cruessler]

@Byron I’ve merged main and CI is green, so you can have another look if you want! Feel free to convert back to draft if you think it’s not ready yet!