Re: [PATCH v2 2/2] fetch: write commit-graph using updated refs only

On Tue, Oct 06, 2026 at 09:46:32AM +0000, Kristofer Karlsson via GitGitGadget wrote:

From: Kristofer Karlsson <redacted>

When fetch.writeCommitGraph was introduced in

    50f26bd035 (fetch: add fetch.writeCommitGraph config
                setting, 2019-09-02),

Tiny nit, not worth a reroll and something I missed in the first round:
it's rather uncustomary to have this commit stand out like this, we
typically have it embedded in the free-flowing text.

the stated goal was to stay updated with the latest commits after
fetching new objects.  The implementation used
write_commit_graph_reachable() because it was the only API available,
but two things have changed since then:

 1. write_commit_graph() was added, and it accepts an explicit set of
    commits as seeds, enabling more targeted commit-graph updates.

 2. The ref-scanning callback add_ref_to_set() became more expensive
    in
        630cd5194e (commit-graph.c: peel refs in 'add_ref_to_set',
                    2020-07-22)

Likewise.

    when it started to validate the refs against the odb
    for correctness.  On a repository with many refs, this makes the
    full reachable scan unnecessarily costly for a targeted fetch.

Optimize the commit-graph write by using only the newly updated refs
as seeds instead of scanning all refs after every fetch.  To keep
this change small, skip the optimization for multi-remote fetches
(since that would require propagating the set of refs across process
boundaries).

Since do_fetch() already knows which refs were updated, collect them
into an oidset and then pass them directly to write_commit_graph().
fetch always writes the commit-graph in split mode, so this adds a
new layer on top of the existing chain rather than replacing it:
close_reachable() walks from the updated tips and stops at commits
already present in the graph, so the new layer only contains the
newly fetched history, and commits covered by the existing layers
remain covered.  This relies on split mode; a non-split write would
replace the graph with just the closure of the seeds.

The part about split commit graphs is important to point out here, as
this is what we rely on to make this whole infra even work. The other
parts about how we collect the object IDs feels overly verbose though,
as you're basically just explaining the diff without providing much
context.

The reachability closure also covers auto-followed tags, since their
targets are reachable from the fetched tips that caused them to be
auto-followed.

This piece of information feels a bit random to me. Tags aren't even
part of the commit graph, are they? And for auto-followed tags we'd
of course naturally cover the commits they point to, but that's just
business as usual and nothing that we specifically had to make sure
keeps on working, right?. So I wonder why this is explicitly being
pointed out now.

Refs that are rejected because they would require changes to
.git/shallow are skipped, just like store_updated_refs() does.  Their
objects are received but their history is incomplete, so walking from
them would make the commit-graph write fail.

And this bordering on the line of getting too verbose, as well. You
already explain this in code with a comment already, so you're basically
just repeating that.

Okay. It's unclear why the reference to `store_updated_refs()` exists
here, as it doesn't seem to give me any useful context. But the other
part about why we skip this is helpful.

Can't we instead:

    git -C shallow-graph checkout --orphan no-shallow &&
    test_commit -C shallow-graph no-shallow

And likewise, `test_commit -C shallow-graph no-shallow-2`?

Okay. So if I understand correctly, this test here verifies that we can
read the non-shallow commit from the graph, but not the shallow one.
Makes sense.

+	)
+'

Thanks!

Patrick