From: Patrick Steinhardt Date: Fri, 02 Oct 2026 11:22:03 GMT Subject: Re: [PATCH 2/2] fetch: write commit-graph using updated refs only Message-ID: In-Reply-To: On Fri, Oct 02, 2026 at 08:33:38AM +0000, Kristofer Karlsson via GitGitGadget wrote: > From: Kristofer Karlsson > > When fetch.writeCommitGraph was introduced in > > 50f26bd035 (fetch: add fetch.writeCommitGraph config > setting, 2019-09-02), > > the stated goal was to stay updated with the latest commits after > fetching new objects. The implementation used > write_commit_graph_reachable() because it was the only API available, > but two things have changed since then: > > 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. > 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. > 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). Yeah, the way we perform fetches can be a bit annoying at times, as all these subprocesses make it very hard to exchange information. > 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? > 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. > Full commit-graph coverage of all refs remains the responsibility > of "git maintenance", "git gc" and "git commit-graph write". > Regular Git operations may trigger "git maintenance run --auto", > which periodically rebuilds the commit-graph from all reachable > refs. Curiously, you mention performance as motivating factor for this change but don't provide a benchmark demonstrating the benefit. > 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. > + 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? > @@ -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. Thanks! Patrick