From: Kristofer Karlsson Date: Wed, 07 Oct 2026 07:33:26 GMT Subject: Re: [PATCH v2 2/2] fetch: write commit-graph using updated refs only Message-ID: In-Reply-To: On Wed, 7 Oct 2026 at 08:39, Patrick Steinhardt wrote: > > On Tue, Oct 06, 2026 at 09:46:32AM +0000, Kristofer Karlsson via GitGitGadget wrote: > > From: Kristofer Karlsson > > > > 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. Will fix, since I am rerolling anyway. (And will keep in mind for the future.) > > 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. Will simplify and shorten it significantly. Something like this: This relies on the commit-graph write being additive, keeping the commits that are already in the graph. fetch already operates in this mode (COMMIT_GRAPH_WRITE_SPLIT) and now that becomes required for correctness. Without that mode, the write would replace the commit-graph and lose other commits. > > 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. That's fair -- I added it because I wanted to convince myself that auto-followed tags don't need special handling, but as you say, their commits are reachable from the fetched tips anyway. Will remove. > > 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. Yes, removing this. > > > diff --git a/builtin/fetch.c b/builtin/fetch.c > > index 533fdfe7d8..574c361530 100644 > > --- a/builtin/fetch.c > > +++ b/builtin/fetch.c > > @@ -1903,10 +1903,34 @@ out: > > return retcode; > > } > > > > +static void collect_updated_tips(struct oidset *tips, struct ref *ref_map) > > +{ > > + struct ref *rm; > > + for (rm = ref_map; rm; rm = rm->next) { > > + struct commit *commit; > > + /* > > + * Like store_updated_refs(), skip shallow-rejected refs: > > + * they are not stored, and their history is incomplete. > > + */ > > 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. Right, I will simplify the text here a bit to: Shallow-rejected refs are not stored and their history is incomplete, so skip them. The reference was meant to point out that store_updated_refs() skips these refs as well (which is also why the full reachable scan never runs into them), but that is indirect, so best to just remove it. > > + git clone --no-local --depth=2 .git shallow-graph && > > + ( > > + cd shallow-graph && > > + git checkout --orphan no-shallow && > > + commit no-shallow > > + ) && > > Can't we instead: > > git -C shallow-graph checkout --orphan no-shallow && > test_commit -C shallow-graph no-shallow Sometimes the blocks help for clarity, but this is short enough anyway so you're right it's not needed. I will update to that, using --no-tag, since the fetch would otherwise auto-follow the new tags and they would show up in the for-each-ref check. > > > + git init notshallow-graph && > > + git -C notshallow-graph -c fetch.writeCommitGraph=true \ > > + fetch ../shallow-graph/.git "refs/heads/*:refs/remotes/shallow/*" && > > + ( > > + cd shallow-graph && > > + commit no-shallow-2 > > + ) && > > And likewise, `test_commit -C shallow-graph no-shallow-2`? Yes, will fix that too. Thanks, Kristofer