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

[Patrick Steinhardt <ps@pks.im>]

On Fri, Oct 02, 2026 at 08:33:38AM +0000, Kristofer Karlsson via GitGitGadget wrote:
> 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.

>  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