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

[Patrick Steinhardt <ps@pks.im>]

On Tue, Oct 06, 2026 at 09:46:32AM +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),

Tiny nit, not worth a reroll and something I missed in the first round:
it's rather uncustomary to have this commit stand out like this, we
typically have it embedded in the free-flowing text.

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

Likewise.

>     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.
> 
> 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).
> 
> Since do_fetch() already knows which refs were updated, collect them
> into an oidset and then pass them directly to write_commit_graph().
> fetch always writes the commit-graph in split mode, so this adds a
> new layer on top of the existing chain rather than replacing it:
> close_reachable() walks from the updated tips and stops at commits
> already present in the graph, so the new layer only contains the
> newly fetched history, and commits covered by the existing layers
> remain covered.  This relies on split mode; a non-split write would
> replace the graph with just the closure of the seeds.

The part about split commit graphs is important to point out here, as
this is what we rely on to make this whole infra even work. The other
parts about how we collect the object IDs feels overly verbose though,
as you're basically just explaining the diff without providing much
context.

> The reachability closure also covers auto-followed tags, since their
> targets are reachable from the fetched tips that caused them to be
> auto-followed.

This piece of information feels a bit random to me. Tags aren't even
part of the commit graph, are they? And for auto-followed tags we'd
of course naturally cover the commits they point to, but that's just
business as usual and nothing that we specifically had to make sure
keeps on working, right?. So I wonder why this is explicitly being
pointed out now.

> Refs that are rejected because they would require changes to
> .git/shallow are skipped, just like store_updated_refs() does.  Their
> objects are received but their history is incomplete, so walking from
> them would make the commit-graph write fail.

And this bordering on the line of getting too verbose, as well. You
already explain this in code with a comment already, so you're basically
just repeating that.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 533fdfe7d8..574c361530 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1903,10 +1903,34 @@ 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;
> +		/*
> +		 * Like store_updated_refs(), skip shallow-rejected refs:
> +		 * they are not stored, and their history is incomplete.
> +		 */

Okay. It's unclear why the reference to `store_updated_refs()` exists
here, as it doesn't seem to give me any useful context. But the other
part about why we skip this is helpful.

> diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
> index f323ceebd2..624bd124be 100755
> --- a/t/t5537-fetch-shallow.sh
> +++ b/t/t5537-fetch-shallow.sh
> @@ -135,6 +135,34 @@ test_expect_success 'fetch that requires changes in .git/shallow is filtered' '
>  	)
>  '
>  
> +test_expect_success 'fetch.writeCommitGraph skips refs that require changes in .git/shallow' '
> +	git clone --no-local --depth=2 .git shallow-graph &&
> +	(
> +		cd shallow-graph &&
> +		git checkout --orphan no-shallow &&
> +		commit no-shallow
> +	) &&

Can't we instead:

    git -C shallow-graph checkout --orphan no-shallow &&
    test_commit -C shallow-graph no-shallow

> +	git init notshallow-graph &&
> +	git -C notshallow-graph -c fetch.writeCommitGraph=true \
> +		fetch ../shallow-graph/.git "refs/heads/*:refs/remotes/shallow/*" &&
> +	(
> +		cd shallow-graph &&
> +		commit no-shallow-2
> +	) &&

And likewise, `test_commit -C shallow-graph no-shallow-2`?

> +	rejected=$(git -C shallow-graph rev-parse main) &&
> +	(
> +		cd notshallow-graph &&
> +		git -c fetch.writeCommitGraph=true \
> +			fetch ../shallow-graph/.git "refs/heads/*:refs/remotes/shallow/*" &&
> +		git for-each-ref --format="%(refname)" >actual.refs &&
> +		echo refs/remotes/shallow/no-shallow >expect.refs &&
> +		test_cmp expect.refs actual.refs &&
> +		test-tool read-graph commit-info shallow/no-shallow &&
> +		test_expect_code 1 \
> +			test-tool read-graph commit-info $rejected 2>/dev/null

Okay. So if I understand correctly, this test here verifies that we can
read the non-shallow commit from the graph, but not the shallow one.
Makes sense.

> +	)
> +'

Thanks!

Patrick