[commitgraph] Implement commit-graph-verify plumbing command.

[avoidscorn]

Commit graphs are optional, so commit-graph plumbing is guarded by a
commitgraph feature that is enabled by default.

[Byron]

Thanks a lot! This is indeed a nice increment, and I will take a closer look soon.
Even though commit-graph is optional within a repository, I don't think it's worth feature gating it as it is not driving up compile times with its ~1000 lines of Rust code. Rather I prefer to provide the capability similar to how git does, hoping that one day we can even generate it, maybe fast enough to do that by default and transparently.

edit: just saw it's a draft. Will take a look when ready. Happy coding!

[avoidscorn]

Completed the non-parallel, no-progress implementation of commit-graph-verify.

I think the most iffy part of this is File::traverse's two error types: Error and EitherError. I wanted graph::verify::Error to be able to contain file::verify::Error values, but for convenience I also wanted Graph::verify_integrity to use the same error type inside and outside of its processor closure.

I'm also unsure how closely you want to match git in terms of verification error messages and test cases.

[Byron]

Completed the non-parallel, no-progress implementation of commit-graph-verify.

That sounds like great progress! I wonder how fast verification already is. After all, I wouldn't expect the graph files to be too large, to the point where single-threaded operation might already be fast enough for most. After all, using git-features::parallel will drive the code complexity up a notch.
Of course it would be interesting to see how it fares against git commit-graph verify, I could imagine it's competitive already.

The same applies to adding progress, to some extend, even though it's usually much easier to accomplish. It might be a waste though if the average verify operation takes less than 1s, the time the 'line renderer' will wait until it shows up at all.

Edit: I just noticed that git also looks up the actual commit object for additional validation. This is something the processor will be used for, and with that it certainly drives up the processing time. The current plumbing implementation could implement it, as git-odb::compound::Db supports full git object databases. This would also be its first use, so that could be very interesting.

I think the most iffy part of this is File::traverse's two error types: Error and EitherError. I wanted graph::verify::Error to be able to contain file::verify::Error values, but for convenience I also wanted Graph::verify_integrity to use the same error type inside and outside of its processor closure.

I took a look and understand why that is necessary. Something leaving a bitter taste is the asymmetry this causes in the API. One could (probably) argue that Graph::verify_integrity could also use EitherError. But would that be practical or will it ever be useful?

A somewhat less complex way of restoring symmetry could be to put the Processor Error into file::verify::Error similarly to how it's done in graph::verify::Error, and that would require a separate ProcessorError for all error cases that can happen within the Processor itself to prevent graph::verify::Error from being recursive. This would get rid of EitherError and add a new specific error for everything that can happen within the processor.

To me that doesn't sound too bad, at least if a symmetric API is considered valuable, and is my preferred solution. Please let me know if I am missing something though, as what's there is definitely doing the job well enough already.

I'm also unsure how closely you want to match git in terms of verification error messages and test cases.

This is entirely up to you. gitoxide is not trying to mirror git in every way, but wants to improve it where it matters. This could be performance, but probably is ease of use in the most cases. That said, verification messages could be whatever is most descriptive for the error at hand - and if that's what git is using, it should be fair to use that too.

When it comes to test-cases, I tend to be very pragmatic and go with a happy-path test for every feature (or command line flag), along with at least one sad-path test to see that verification actually works while producing acceptable error messages, without the need to test for every variant in the error enum at all. Thus git's tests are certainly more exhaustive, but there is value to keep it simple and lean at the beginning to help moving fast, at the cost of being reactive in case bugs inevitably come in with tests for their fixes only.

Great work thus far, thanks so much! I am super interested in seeing this in a stress test, and of course to compare its performance with git, which I think is single-threaded too.

[avoidscorn]

Here are some quick measurements on the most pathological repo I could find (https://github.com/cirosantilli/test-many-commits-1m):

• git commit-graph verify takes 14.5s and reports progress. Single threaded.
• git -c core.commitGraph=false merge-base --is-ancestor <oldest commit> <newest commit> takes 8.3 seconds. Single threaded.
• git -c core.commitGraph=true merge-base --is-ancestor <oldest commit> <newest commit> takes 1.3 seconds. Single threaded.

I'll revisit the EitherError stuff. I'm actually thinking of moving all commit-level stuff from File::traverse to Graph::verify_integrity, as that would eliminate having multiple layers of processors, which I think side-steps this whole issue. If not, I'll just create a private graph::verify::ProcessorError and map its variants to graph::verify::Error.

Edit: If we go with the graph::verify::ProcessorError approach, do you want those errors mapped to graph::verify::Errors before Graph::verify_integrity returns, or do you want them exposed as graph::verify::Error::File(file::verify::Error::Processor(graph::verify::ProcessorError))? The later seems super-gross, as it implies graph::verify::Error::File(file::verify::Error::Processor(graph::verify::ProcessorError::Processor(<caller-supplied error>))) is also a thing.

Can git-commitgraph depend on git-odb? If not, I'm not sure where you'd want to put full commit-graph + object database verification and graph-algorithm code.

[Byron]

git commit-graph verify takes 14.5s and reports progress. Single threaded.

This looks very much like progress should be implemented then. It's quite swift given that it probably makes 1 million object lookups… if that's true, it would do 71k lookups per second on a single thread, which seems like a lot judging from my experience with git's performance with pack lookups. Alternatively your CPU is ~2.5x of what I am using (also not too unlikely :D).

The other timings will also be interesting especially once we have a similar command.

[…] The later seems super-gross, as it implies graph::verify::Error::File(file::verify::Error::Processor(graph::verify::ProcessorError::Processor()))

That's true, it looks like the nesting level gets a little deeper than I thought. I hoped it would look like this when matched:

match err {
  graph::verify::Error::File(file::verify::Error::Processor(err)) => {case 1: I thought it would be this…},
  graph::verify::Error::File(file::verify::Error::Processor(graph::verify::ProcessorError::Processor(err))) => {case 2: …and agree this seems a bit unwieldy, despite being lossless and correct},
  _ => {}
}

Case 1 seems quite alright still, even though case 2 might be a bit too much, especially if you see a way to sidestep the issue.

Can git-commitgraph depend on git-odb? If not, I'm not sure where you'd want to put full commit-graph + object database verification and graph-algorithm code.

That's an interesting one! Thus far gitoxide-core was able to play a glue-role, bringing everything together, without having to know too much. By the looks of the canonical implementation, the processor looking up the commit would have to know an aweful lot about the commit-graph, which I'd certainly want to avoid putting into gitoxide-core.

What about putting the 'detailed verification' behind a feature flag in git-commitgraph? That way it could implement a processor bound to a compound::Db, which gitoxide-core could just use after instantiating one from an object directory, something like this:

let db = git_odb::compound::Db::at(object_dir)?;
graph.verify_integrity(…, |commit| verify_detailed(commit, &db), …);

Thinking about it, there might be another way. Looking up an object could feasibly be put behind an impl of a function that does the lookup, completely avoiding having to know git-odb.

let db = git_odb::compound::Db::at(object_dir)?;
let mut buf = Vec::new();
graph.verify_integrity(…, |commit| verify_detailed(commit, |id| db.locate(id, &mut buf).transpose().ok().flatten().and_then(borrowed::Object::as_commit).map(borrowed::Commit::to_owned), …);

The above would not work in a multi-threaded environment, but could be made to work by making a closure returning a closure for lookups per thread (this is common practice in git-odb already, but complicates things as you can imagine). It's certainly alright to aim for single-threadedness first to get first performance results.

I would certainly prefer the second one as it is general, but can live with the first one as it's simpler to get started with.
Maybe I am missing something, what do you think about these options?

[avoidscorn]

graph.verify_integrity(…, |commit| verify_detailed(commit, |id| db.locate(id, &mut buf).transpose().ok().flatten().and_then(borrowed::Object::as_commit).map(borrowed::Commit::to_owned), …);

Not gonna lie, that line keeps me up at night. Almost as much as EitherError.

I'd like to follow this up with implementing cargo bench support using the Criterion library. I'd reuse tests/fixtures/repos and add tests/fixtures/commit-graphs. For ease of use, I'd like to make the benchmarks populate those fixtures as needed. Maybe via lazy_static.

[Byron]

Not gonna lie, that line keeps me up at night. Almost as much as EitherError.

That sounds like it's still (a tad) better then :). Actually there is already a precedence for doing this, as a similar problem exists when resolving objects foreign to a thin pack. Technically the code could prescribe the use of compound::Db, but I found it unnecessarily restrictive despite a Repository probably doing exactly that.

I'd like to follow this up with implementing cargo bench support using the Criterion library.

That sounds good, thanks for all the work you put into this! I will hopefully find some more time tomorrow to do a thorough review.

[Byron]

Fantastic work, thank you!

I just applied tiny refactorings, with two ones worth mentioning (or at least in recent memory :D):

• The special Error type that wanted to be ! was replaced with std::convert::Infallible, which also wants to be ! when ready.
• git_odb::hash::bytes_of_flie is now git_features::hash::bytes_of_file, but is not yet used for file::verify_checksum() as it's slightly more involved and probably not worth it. However, you might implement it anyway when progress support is added.

Please share your thoughts in case you would want things to look differently, or make the changes in the next PR.

In any case, I am very much looking forward to your next PR :)!

[Byron]

Oh, and before I forget: Now would be a good time to add, with minimal effort, a stress test for commit-graph-verify. I think it could be two lines around here to run the verification on a big commit-graph like the one of the linux kernel repo.

[avoidscorn]

Both of your changes look good!