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

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

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

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

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

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