Re: [PATCH 2/2] fetch: write commit-graph using updated refs only
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 2, 2026, 11:22 UTC
- Message-ID
- <ar-T2y54X1uDQ4mX@pks.im>
- In-Reply-To
- <fee92f3c2009f8f282fe98e6b16d403704db9ad9.1790930019.git.gitgitgadget@gmail.com>
On Fri, Oct 02, 2026 at 08:33:38AM +0000, Kristofer Karlsson via GitGitGadget wrote:
Show 14 quoted lines
> From: Kristofer Karlsson <krka@spotify.com> > > 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.
Show 7 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.
Show 5 quoted lines
> 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.
Show 7 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?
Show 14 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.
Show 5 quoted lines
> 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.
Show 15 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.
Show 9 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?
Show 28 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.
Thanks!
Patrick