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

From: Kristofer Karlsson <krka@spotify.com>
Date: Oct 2, 2026, 12:40 UTC
Message-ID: <CAL71e4OcAg1PYaZZ2474Q5ayQgTeJFR2-7J+0ddrCe+rwwj=3w@mail.gmail.com>
In-Reply-To: <ar-T2y54X1uDQ4mX@pks.im>

On Fri, 2 Oct 2026 at 13:22, Patrick Steinhardt <ps@pks.im> wrote:

Show 12 quoted lines

>
> >  1. write_commit_graph() was added, and it accepts an explicit set of
> >     commits as seeds, enabling more targeted commit-graph updates.
>
> Hm. The big question here is whether these additional seeds are additive
> or exclusive. That is, if I have an existing commit graph already, would
> it basically just extend the commit graph with the additional object IDs
> or would it replace the commit graph with a new one that only considers
> the passe object IDs as input?
>
> I would hope that it's additive, because otherwise you may now lose
> commit graph coverage for stuff that was covered before the patch.

Yes, it is additive.  fetch always writes with this flag:

    int commit_graph_flags = COMMIT_GRAPH_WRITE_SPLIT;

so write_commit_graph() only adds the commits that are not already in the graph, as a new layer on top of the existing chain. When layers get merged, the commits of the merged layers are carried over.

You are right that a non-split write without COMMIT_GRAPH_WRITE_APPEND would replace the graph with just the closure of the seeds, so this relies on fetch using split mode. I can extend the test to verify that commits which were in the graph before the fetch are still there afterwards.

Show 11 quoted lines

> >  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)
> >     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.
>
> I was wondering whether incremental commit graphs would also be part of
> the reasoning. Because in theory, now that we have those, we could even
> extend the commit graph on a fetch by just writing another layer.

Yes, that is exactly what happens: since fetch writes in split mode, the newly fetched history ends up in a new layer. The commit message should say so explicitly, and I will update it in the reroll.

Show 10 quoted lines

> > Since do_fetch() already knows which refs were updated, collect them
> > into an oidset and then pass them directly to write_commit_graph().
> > In split mode, close_reachable() walks from the updated tips and
> > stops at commits already present in the graph, efficiently adding
> > the newly fetched history.  This reachability closure also covers
> > auto-followed tags, since their targets are reachable from the
> > fetched tips that caused them to be auto-followed.
>
> Aha! So I wasn't that far off :) Now there's a follow-up question
> though: what happens in non-split mode?

The fetch path never uses non-split mode (see above). If that ever changes, the incremental path would need COMMIT_GRAPH_WRITE_APPEND, or a fallback to the reachable scan, to avoid losing coverage.

Show 16 quoted lines

> > After fetch_one() returns, call prepare_commit_graph() (which is
> > made non-static by this commit) to determine the graph-write mode:
> >
> >  - If no commit-graph exists yet, fall back to the full reachable
> >    scan so the first graph creation covers all refs.
> >
> >  - If a commit-graph exists and the fetch updated at least one ref,
> >    write incrementally using only the new refs as seeds.
> >
> >  - If a commit-graph exists but the fetch is a no-op, skip the
> >    commit-graph write entirely.
> >
> >  - For the multi-remote path (fetch --all), where child processes
> >    do the actual fetching, fall back to the full reachable scan.
>
> All of these make sense, but the above question is not answered yet.

I hope the answer above covers it. :)

> Curiously, you mention performance as motivating factor for this change
> but don't provide a benchmark demonstrating the benefit.

I left it out since the change avoids work rather than making existing work faster: the cost of the full scan grows with the number of refs, so the improvement depends mostly on the repository. But I agree that some numbers are useful. Here is a synthetic setup: git.git with 200K extra packed refs (~206K total), a local file:// remote, an existing split commit-graph (and a warmed up page-cache). Times are the median of 9 runs and I am looking at the trace2 region for fetch/write-commit-graph:

    scenario          before    after
    no-op fetch       380 ms    (skipped)
    1 ref updated     357 ms    9.3 ms
    10 refs updated   359 ms    8.9 ms

I will include these numbers in the cover letter of the reroll, or do you think it makes more sense to also have them in the commit message?

Show 18 quoted lines

> > diff --git a/builtin/fetch.c b/builtin/fetch.c
> > index 533fdfe7d8..8ad7331640 100644
> > --- a/builtin/fetch.c
> > +++ b/builtin/fetch.c
> > @@ -1903,10 +1903,30 @@ 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;
> > +             if (rm->status == REF_STATUS_REJECT_SHALLOW)
> > +                     continue;
>
> Hm. Shouldn't we also refuse almost all of the other values here? I'd
> expect that we only want to consider a tip when it has REF_STATUS_OK.

This confused me at first too. REF_STATUS_OK and most of the other values are only used on the push side. During fetch, the status stays at REF_STATUS_NONE, and the only value that fetch-pack sets is REF_STATUS_REJECT_SHALLOW, so that is the only one we need to filter. Requiring REF_STATUS_OK would skip every ref.

However, I could change it to use status != REF_STATUS_NONE -- those are the only two statuses we can get so both would work, but I guess which one is best depends on what kind of new statuses could be added in the future.

Refs whose local update gets rejected (e.g. a non-fast-forward without --force) are still harmless to include, since their commits are fully present in the object store.

Show 13 quoted lines

> > +             if (is_null_oid(&rm->old_oid))
> > +                     continue;
> > +             if (rm->peer_ref &&
> > +                 oideq(&rm->old_oid, &rm->peer_ref->old_oid))
> > +                     continue;
> > +             commit = lookup_commit_reference_gently(the_repository,
> > +                                                     &rm->old_oid, 1);
> > +             if (commit)
> > +                     oidset_insert(tips, &commit->object.oid);
>
> This is something that always trips me with `struct ref`, that I'm never
> quite sure what's what. So please forgive my ignorance, but why do we
> look up `rm->old_oid` here?

This tripped me up as well.  In the fetch ref_map:

    rm->old_oid            the value advertised by the remote, i.e.
                           the new tip we are fetching
    rm->peer_ref           the local ref it maps to via the refspec
                           (e.g. refs/remotes/origin/main), or NULL
                           if it only goes to FETCH_HEAD
    rm->peer_ref->old_oid  the current local value, before the update

So rm->old_oid is the new tip, and the oideq() check skips refs that did not change. rm->new_oid is not set on the ref_map during fetch; store_updated_refs() copies rm->old_oid into the new_oid of a separate struct ref for the local update.

As a concrete example, say "git fetch origin" with the default refspec sees that the remote's main moved from A to B, a new branch topic appeared at C, and stable is still at D:

    rm->name           old_oid  peer_ref->name             peer old_oid
    refs/heads/main    B        refs/remotes/origin/main   A
    refs/heads/topic   C        refs/remotes/origin/topic  (null)
    refs/heads/stable  D        refs/remotes/origin/stable D

This collects B and C as tips and skips stable. When fetching from a URL without a configured remote, e.g. "git fetch <url> main", the entry has no peer_ref (it only goes to FETCH_HEAD), so B is collected unconditionally.

Show 34 quoted lines

> > @@ -2535,6 +2559,12 @@ int cmd_fetch(int argc,
> >       int negotiate_only = 0;
> >       int porcelain = 0;
> >       int i;
> > +     enum {
> > +             GRAPH_WRITE_REACHABLE,
> > +             GRAPH_WRITE_TIPS,
> > +             GRAPH_WRITE_SKIP,
> > +     } graph_write_mode = GRAPH_WRITE_REACHABLE;
> > +     struct oidset updated_tips = OIDSET_INIT;
> >
> >       struct option builtin_fetch_options[] = {
> >               OPT__VERBOSITY(&verbosity),
> > @@ -2822,7 +2852,13 @@ int cmd_fetch(int argc,
> >               }
> >               trace2_region_enter("fetch", "fetch-one", the_repository);
> >               result = fetch_one(remote, argc, argv, prune_tags_ok, stdin_refspecs,
> > -                                &config, &filter_options);
> > +                                &config, &filter_options, &updated_tips);
> > +             if (prepare_commit_graph(the_repository)) {
> > +                     if (oidset_size(&updated_tips))
> > +                             graph_write_mode = GRAPH_WRITE_TIPS;
> > +                     else
> > +                             graph_write_mode = GRAPH_WRITE_SKIP;
> > +             }
> >               trace2_region_leave("fetch", "fetch-one", the_repository);
> >       } else {
> >               int max_children = max_jobs;
>
> It's a bit curious that we have `GRAPH_WRITE_SKIP` as an explicit value
> here as it can be trivially derived from `oidset_size()` anyway. But
> other than that this is the safeguard that you were talking about: when
> we have a commit graph already then we only update with new tips,
> otherwise we use a full reachability walk.

The oidset can be empty for two different reasons:

 1. the fetch was a no-op, in which case skipping is correct, or

 2. fetch_one() was never called because we took the multi-remote
    path, in which case we must fall back to the reachable scan.

Deriving the mode from oidset_size() alone would make "fetch --all" with an existing graph skip the write entirely. Setting the mode right where the fetch happens seemed like the best way to make this more explicit and easy to reason about.

> Thanks!
>
> Patrick

Thanks for the careful review! I will update the commit message to cover the points above, extend the test, and send a reroll (next week I suppose, don't want to rush it).

Kristofer