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

[Patrick Steinhardt <ps@pks.im>]

On Fri, Oct 02, 2026 at 02:40:44PM +0200, Kristofer Karlsson wrote:
> On Fri, 2 Oct 2026 at 13:22, Patrick Steinhardt <ps@pks.im> wrote:
> >
> > >  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.

Awesome :)

[snip]
> > 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?

I think it makes sense to have it as part of the commit message.

> > > 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.

Okay, makes sense. I'd aim to be as defensive as possible, and defensive
here probably means that we should err on the side of covering too many
commits rather than covering not enough. And that's basically what
you're already doing anyway.

I think having a short comment that explains this would help though.

> 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.

Yup.

> > > +             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.

That part really is quite confusing. Thanks for explaining!

> > > @@ -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.

Ah, right, the second condition is what I forgot about.

Patrick