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

On Fri, Oct 02, 2026 at 02:40:44PM +0200, Kristofer Karlsson wrote:

On Fri, 2 Oct 2026 at 13:22, Patrick Steinhardt [off-list ref] wrote:

quoted

quoted

 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]

quoted

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.

quoted

quoted

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.

quoted

quoted

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

quoted

quoted

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