[commitgraph] implement basic, low-level read API

[avoidscorn]

This is just the first pass at ensuring we can read commit-graph
files correctly. It does not support anything interesting like
reachability, merge-base, or ahead-behind queries.

[avoidscorn]

TODO before merging:

1. Add tests for non-happy-path cases.
2. Fill in docs.
3. Integrate into CI.

I wanted to settle on the test fixtures before making a PR, as I wanted to avoid polluting git history with too many fixture changes. But I can still split this into multiple PRs if desired.

[Byron]

(Related to #17 )

Thanks a lot! This is a ton of work and a massive, much appreciated contribution!

I wanted to settle on the test fixtures before making a PR, as I wanted to avoid polluting git history with too many fixture changes. But I can still split this into multiple PRs if desired.

Let's keep things as easy for you as possible, and keeping everything in one PR is absolutely alright with me.
Regarding the TODOs:

• Is it truly necessary to test for non-happy cases? If there are no unwrap() calls, I believe we are sure the program will not panic unexpectedly. Maybe an opportunity to save time.
• Docs are nice, and I usually write them this early only if I have to keep intricate, difficult knowledge (relative to myself at the time) somewhere. Maybe the tests are already doing this, but I do recommend waiting with excessive docs until there is a tool using and validating the current API (something that could be added in future PRs).
• For CI integration, have a look at the makefile. I would expect commit-graph tests to be picked up already, but there is where you would setup anything special.

In any case, I would hope we can focus on making this PR mergable soon, and not extend its scope too much, rather the opposite.
I would also take a more thorough look in the next days if you are interested in suggestions and questions as they come up.

[Byron]

I had a very rough look at a few pieces, and really like that the implementation tries to follow the overall style of what's already there in the other crates. If you have any feedback about that of things you would rather want to do differently, please let me know.

When I saw the V1 mentioned in the fixture directories, I was a little confused as there seems to be only one version of the commit-graph format. Maybe this was done akin to the fixtures in git-protocol, which does have to known formats right now (unfortunately).

I would be very interested to learn if you see interesting applications even in the context of a 'plumbing' tool, something like statistics maybe.

[avoidscorn]

I guess I should follow up with some terminology/style questions:

1. Are you okay with fully-spelled out type names like GraphPosition/LexPosition? Would you prefer GraphPos/LexPos?
2. I've been ordering struct fields/impl methods/enum variants in alphabetical order and putting private methods in separate impls. Any preference on this?
3. Do you have guidance on which symbols the top-level module should re-export?

[Byron]

Great! Let's do this.

Are you okay with fully-spelled out type names like GraphPosition/LexPosition? Would you prefer GraphPos/LexPos?

I prefer fully spelled out names, but am fine if the shortened name is canonical. Notable example might be the term ref, which is also used in this codebase. That probably doesn't apply to GraphPos though.

I've been ordering struct fields/impl methods/enum variants in alphabetical order and putting private methods in separate impls. Any preference on this?

No. I find myself using multiple impls as well, not for private/public separation, but to (arbitrarily) put related methods into a 'category' impl block which can even carry its own documentation.

Do you have guidance on which symbols the top-level module should re-export?

I am usually keeping everything in their own modules until code that uses the crate really 'asks' for some top-level exports for convenience. An example for this was git_packetline::PacketLine, which is a rename of git_packetline::Borrowed. Maybe that's another hint at how the gitoxide crates name things - they try not to repeat their parent crate name or their module name.

I hope that helps!

[Byron]

I saw some changes were made, and besides the windows test failures, is there anything I should look at in particular?

[avoidscorn]

I believe this PR is ready for real review now, although it looks like the odb tests are bamboozling me.

I'd prefer these commits to get squashed when or before merging, but I don't know if you are okay doing that via github or whether I should do it after review is complete. I don't plan on signing commits anytime soon, so any merge strategy is okay with me.

[Byron]

Alright, I will be working on it, pushing some changes to make the PR mergeable. I will let you know when done for a chance to hear what you think about the changes.

[Byron]

I finally managed to have a thorough look at each fo the ~1000 lines of code you are generously contributing, and am absolutely amazed about the level of quality I believe to see even with my limited understanding of the commit-graph data as a whole.

I dared to make minor refactorings mostly around Position and LexPosition.
The only question remaining is if there is anything you can imagine doing about the commented-out code in file::init. My suggestion is to remove it, while possibly keeping the note.

If there is nothing you want to change, I am happy to merge this PR and release a v0.1 to crates.io.

Hopefully you will find the time to continue your work on this crate or gitoxide as a whole! Something I would love to see is a stress-test which runs some plumbing command on big commit-graphs on real-world repositories, like the linux or rust repositories. Infrastructure for that exists already.

Below is my notes

• adjust tests to disable gpgsignatures
• do graph results need a reference to their owning file?
  • Yes, as it allows to obtain additional information related to the item in the file itself, like File::commit_at(…)
• feature-toggled support for serde
• make tests depend on checked-in fixtures, instead of generating them (and depend on git on CI), making it easy to recreate them
  • the tests currently rely on calling git, see inspect_refs(…)
• Questions
  • How can Commit return Graph positions? It doesn't seem to learn about an offset.
    • Parent IDs are indeed specified as graph positions, not file positions, as they may be in previous commit graph files.
  • What to do with the 'extra-garbage',
    some code is commented out.
• Future Work
  • A plumbing command to extract some value from the current implementation, maybe statistics, or verification
  • Application of the command above in a stress test

[avoidscorn]

All your changes look good!

Plumbing for verification sounds like a good next step. I'll try to be more incremental next time, e.g. stubs in gitoxide-core, then a naive verifier, then a parallelized verifier, then a verifier with progress.