Re: [PATCH v2 2/2] fetch: write commit-graph using updated refs only
- From
Kristofer Karlsson <krka@spotify.com>
- Date
- Oct 7, 2026, 07:33 UTC
- Message-ID
- <CAL71e4OmArbY00EpGZfSME1XoHJZSjusm-AJPQajd6Qg1SDDPg@mail.gmail.com>
- In-Reply-To
- <asXpD2YB_MpunVFs@pks.im>
On Wed, 7 Oct 2026 at 08:39, Patrick Steinhardt <ps@pks.im> wrote:
Show 12 quoted lines
> > On Tue, Oct 06, 2026 at 09:46:32AM +0000, Kristofer Karlsson via GitGitGadget wrote: > > From: Kristofer Karlsson <krka@spotify.com> > > > > 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.)
Show 25 quoted lines
> > 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.
Show 10 quoted lines
> > 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.
Show 8 quoted lines
> > 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.
Show 22 quoted lines
>
> > 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.
Show 11 quoted lines
> > + 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.
Show 10 quoted lines
> > > + 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