fetch: write commit-graph using updated refs only

When fetch.writeCommitGraph is enabled, the commit-graph is currently rebuilt from all reachable refs after every fetch. This is unnecessarily expensive on repositories with many refs, since add_ref_to_set() validates each ref against the odb.

This series optimizes the commit-graph write by using only the newly updated refs as seeds instead of scanning all refs. It introduces a three-mode enum (REACHABLE / TIPS / SKIP) to make the policy explicit:

* No-op fetch: skip the commit-graph write entirely
* Updated refs + existing graph: write incrementally from updated tips only
* No existing graph or multi-remote fetch: fall back to full reachable scan

Patch 1 adds a commit-info subcommand to test-tool read-graph for verifying graph contents in tests.

Patch 2 implements the optimization in builtin/fetch.c with four tests covering the incremental, unrelated-commit, no-op, and fallback cases.

Kristofer Karlsson (2):
test-tool read-graph: add commit-info subcommand
fetch: write commit-graph using updated refs only

 builtin/fetch.c            | 65 ++++++++++++++++++++++++++++++++------
 commit-graph.c             |  2 +-
 commit-graph.h             |  1 +
 t/helper/test-read-graph.c | 23 +++++++++++++-
 t/t5510-fetch.sh           | 58 ++++++++++++++++++++++++++++++++++
 5 files changed, 138 insertions(+), 11 deletions(-)

base-commit: 0f8e75abebff0877cae681a3d5ff31ac47f54220
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2239%2Fspkrka%2Fkrka%2Fincremental-commit-graph-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2239/spkrka/krka/incremental-commit-graph-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2239

gitgitgadget

From: Kristofer Karlsson <[email protected]>

The test infrastructure has no way to check whether a specific commit is present in the commit-graph, making it hard to verify graph state after operations like fetch.

Add a "commit-info" subcommand to test-tool read-graph that queries whether specific commits are present in the commit-graph and prints their generation numbers. Returns 1 if any commit is not found.

---
 t/helper/test-read-graph.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

index 9f07b9c25a..0ab9cf8f2b 100644
@@ -2,6 +2,9 @@
 
 #include "test-tool.h"
 #include "commit-graph.h"
+#include "commit.h"
+#include "hex.h"
+#include "object-name.h"
 #include "repository.h"
 #include "odb.h"
 #include "bloom.h"

@@ -91,7 +94,25 @@ int cmd__read_graph(int argc, const char **argv)
 		dump_graph_info(graph);
 	else if (!strcmp(argv[1], "bloom-filters"))
 		dump_graph_bloom_filters(graph);
-	else {
+	else if (!strcmp(argv[1], "commit-info")) {
+		int i;
+		for (i = 2; i < argc; i++) {
+			struct object_id oid;
+			struct commit *c;
+
+			if (repo_get_oid(the_repository, argv[i], &oid))
+				die("not a valid object name: '%s'", argv[i]);
+			c = lookup_commit_in_graph(the_repository, &oid);
+			if (!c) {
+				fprintf(stderr, "%s: not in graph\n", argv[i]);
+				ret = 1;
+				continue;
+			}
+			printf("%s generation %"PRIuMAX"\n",
+			       oid_to_hex(&oid),
+			       (uintmax_t)commit_graph_generation(c));
+		}
+	} else {
 		fprintf(stderr, "unknown sub-command: '%s'\n", argv[1]);
 		ret = 1;
 	}

gitgitgadget

From: Kristofer Karlsson <[email protected]>

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.

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.

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

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.

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.

---
 builtin/fetch.c  | 65 +++++++++++++++++++++++++++++++++++++++++-------
 commit-graph.c   |  2 +-
 commit-graph.h   |  1 +
 t/t5510-fetch.sh | 58 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 116 insertions(+), 10 deletions(-)

index 533fdfe7d8..8ad7331640 100644
@@ -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;
+		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);
+	}
+}
+
 static int do_fetch(struct transport *transport,
 		    struct refspec *rs,
 		    const struct fetch_config *config,
-		    struct list_objects_filter_options *filter_options)
+		    struct list_objects_filter_options *filter_options,
+		    struct oidset *updated_tips)
 {
 	struct ref_transaction *transaction = NULL;
 	struct ref *ref_map = NULL;

@@ -2111,6 +2131,8 @@ static int do_fetch(struct transport *transport,
 
 	commit_fetch_head(&fetch_head);
 
+	collect_updated_tips(updated_tips, ref_map);
+
 	if (set_upstream) {
 		struct branch *branch = branch_get("HEAD");
 		struct ref *rm;

@@ -2427,7 +2449,8 @@ static inline void fetch_one_setup_partial(struct remote *remote,
 static int fetch_one(struct remote *remote, int argc, const char **argv,
 		     int prune_tags_ok, int use_stdin_refspecs,
 		     const struct fetch_config *config,
-		     struct list_objects_filter_options *filter_options)
+		     struct list_objects_filter_options *filter_options,
+		     struct oidset *updated_tips)
 {
 	struct refspec rs = REFSPEC_INIT_FETCH(the_hash_algo);
 	int i;

@@ -2494,7 +2517,8 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
 	sigchain_push_common(unlock_pack_on_signal);
 	atexit(unlock_pack_atexit);
 	sigchain_push(SIGPIPE, SIG_IGN);
-	exit_code = do_fetch(gtransport, &rs, config, filter_options);
+	exit_code = do_fetch(gtransport, &rs, config, filter_options,
+			     updated_tips);
 	sigchain_pop(SIGPIPE);
 	refspec_clear(&rs);
 	transport_disconnect(gtransport);

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

@@ -2899,11 +2935,21 @@ int cmd_fetch(int argc,
 		if (progress)
 			commit_graph_flags |= COMMIT_GRAPH_WRITE_PROGRESS;
 
-		trace2_region_enter("fetch", "write-commit-graph", the_repository);
-		write_commit_graph_reachable(the_repository->objects->sources,
-					     commit_graph_flags,
-					     NULL);
-		trace2_region_leave("fetch", "write-commit-graph", the_repository);
+		if (graph_write_mode != GRAPH_WRITE_SKIP) {
+			trace2_region_enter("fetch", "write-commit-graph",
+					    the_repository);
+			if (graph_write_mode == GRAPH_WRITE_TIPS)
+				write_commit_graph(
+					the_repository->objects->sources,
+					NULL, &updated_tips,
+					commit_graph_flags, NULL);
+			else
+				write_commit_graph_reachable(
+					the_repository->objects->sources,
+					commit_graph_flags, NULL);
+			trace2_region_leave("fetch", "write-commit-graph",
+					    the_repository);
+		}
 	}
 
 	if (enable_auto_gc) {

@@ -2927,6 +2973,7 @@ int cmd_fetch(int argc,
 	}
 
  cleanup:
+	oidset_clear(&updated_tips);
 	string_list_clear(&list, 0);
 	list_objects_filter_release(&filter_options);
 	return result;

index 983c11ce85..d042752ff4 100644
@@ -733,7 +733,7 @@ struct commit_graph *read_commit_graph_one(struct odb_source *source)
  * On the first invocation, this function attempts to load the commit
  * graph if the repository is configured to have one.
  */
-static struct commit_graph *prepare_commit_graph(struct repository *r)
+struct commit_graph *prepare_commit_graph(struct repository *r)
 {
 	struct odb_source *source;
 

index 13ca4ff010..7e48b0ccc0 100644
@@ -31,6 +31,7 @@ struct string_list;
 
 char *get_commit_graph_filename(struct odb_source *source);
 char *get_commit_graph_chain_filename(struct odb_source *source);
+struct commit_graph *prepare_commit_graph(struct repository *r);
 int open_commit_graph(const char *graph_file, int *fd, struct stat *st);
 int open_commit_graph_chain(const char *chain_file, int *fd, struct stat *st,
 			    const struct git_hash_algo *hash_algo);

gitgitgadget

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.

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.

Yeah, the way we perform fetches can be a bit annoying at times, as all these subprocesses make it very hard to exchange information.

Aha! So I wasn't that far off :) Now there's a follow-up question
though: what happens in non-split mode?

All of these make sense, but the above question is not answered yet.

Curiously, you mention performance as motivating factor for this change but don't provide a benchmark demonstrating the benefit.

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.

Yes, that is exactly what happens: since fetch writes in split mode, the newly fetched history ends up in a new layer. The commit message should say so explicitly, and I will update it in the reroll.

The fetch path never uses non-split mode (see above). If that ever changes, the incremental path would need COMMIT_GRAPH_WRITE_APPEND, or a fallback to the reachable scan, to avoid losing coverage.

I hope the answer above covers it. :)

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?

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.

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.

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.

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.

Thanks for the careful review! I will update the commit message to cover the points above, extend the test, and send a reroll (next week I suppose, don't want to rush it).

Kristofer

Awesome :)

[snip]

I think it makes sense to have it as part of the commit message.

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.

Yup.

That part really is quite confusing. Thanks for explaining!

Ah, right, the second condition is what I forgot about.

Patrick

Will do!

Agreed, will add a code comment + keep using REF_STATUS_REJECT_SHALLOW As you say, it makes sense to skip as few refs as possible here. I will also add a test case for shallow to prove that the check is important.

Thanks,
Kristofer

When fetch.writeCommitGraph is enabled, the commit-graph is rebuilt from all reachable refs after every fetch. This is unnecessarily expensive on repositories with many refs, since add_ref_to_set() validates each ref against the odb.

This series optimizes the commit-graph write by using only the newly updated refs as seeds instead of scanning all refs. Since fetch writes the commit-graph in split mode, the newly fetched history is added as a new layer on top of the existing chain. A three-mode enum (REACHABLE / TIPS / SKIP) makes the policy explicit:

* No-op fetch: skip the commit-graph write entirely
* Updated refs + existing graph: write incrementally from updated tips only
* No existing graph or multi-remote fetch: fall back to full reachable scan

Patch 1 adds a commit-info subcommand to test-tool read-graph for verifying graph contents in tests.

Patch 2 implements the optimization in builtin/fetch.c with tests covering the incremental, unrelated-commit, no-op, fallback, and shallow-rejected cases.

Benchmark on a synthetic setup: git.git with 200K extra packed refs (~206K total), a local file:// remote, an existing split commit-graph and a warm page cache. Times are the median of 9 runs of the trace2 region 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

Changes since v1:

* Explain in the commit message that fetch writes in split mode, so the new tips are added as a new layer on top of the existing chain, and that the incremental path relies on this (a non-split write would replace the graph with only the closure of the seeds).
* Extend the incremental test to check that a local-only commit, which was in the graph before the fetch but is not reachable from the fetched tips, is still in the graph afterwards.
* Add benchmark numbers to the commit message.
* Add a comment explaining why shallow-rejected refs are skipped when collecting the updated tips (like store_updated_refs(), since their history is incomplete), and mention it in the commit message.
* Add a test in t5537 for a fetch with fetch.writeCommitGraph where a ref is rejected because it would require changes to .git/shallow. Without the check, the commit-graph write dies on the missing parent.

Kristofer Karlsson (2):
test-tool read-graph: add commit-info subcommand
fetch: write commit-graph using updated refs only

 builtin/fetch.c            | 69 +++++++++++++++++++++++++++++++++-----
 commit-graph.c             |  2 +-
 commit-graph.h             |  1 +
 t/helper/test-read-graph.c | 23 ++++++++++++-
 t/t5510-fetch.sh           | 59 ++++++++++++++++++++++++++++++++
 t/t5537-fetch-shallow.sh   | 28 ++++++++++++++++
 6 files changed, 171 insertions(+), 11 deletions(-)

base-commit: 0f8e75abebff0877cae681a3d5ff31ac47f54220
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2239%2Fspkrka%2Fkrka%2Fincremental-commit-graph-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2239/spkrka/krka/incremental-commit-graph-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2239

Range-diff vs v1:

 1:  36cc3ff3b3 = 1:  36cc3ff3b3 test-tool read-graph: add commit-info subcommand
 2:  fee92f3c20 ! 2:  7507354cc9 fetch: write commit-graph using updated refs only
     @@ Commit message
      
          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.
     +    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 reachability closure also covers auto-followed tags, since their
     +    targets are reachable from the fetched tips that caused them to be
     +    auto-followed.
     +
     +    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.
      
          After fetch_one() returns, call prepare_commit_graph() (which is
          made non-static by this commit) to determine the graph-write mode:
     @@ Commit message
          which periodically rebuilds the commit-graph from all reachable
          refs.
      
     +    The effect was measured on a synthetic setup: git.git with 200K
     +    extra packed refs (~206K total), a local file:// remote, an existing
     +    split commit-graph and a warm page cache.  The times below are the
     +    median of 9 runs of the trace2 region 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
     +
          Signed-off-by: Kristofer Karlsson <[email protected]>
      
       ## builtin/fetch.c ##
     @@ builtin/fetch.c: out:
      +	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.
     ++		 */
      +		if (rm->status == REF_STATUS_REJECT_SHALLOW)
      +			continue;
      +		if (is_null_oid(&rm->old_oid))
     @@ t/t5510-fetch.sh: test_expect_success 'fetch.writeCommitGraph' '
      +	git init incremental-source &&
      +	test_commit -C incremental-source one &&
      +	git clone incremental-source incremental-dest &&
     ++	test_commit -C incremental-dest local &&
      +	git -C incremental-dest commit-graph write --reachable --split &&
      +	test_commit -C incremental-source two &&
      +	test_commit -C incremental-source three &&
      +	(
      +		cd incremental-dest &&
      +		git -c fetch.writeCommitGraph=true fetch origin &&
     -+		test-tool read-graph commit-info three two
     ++		test-tool read-graph commit-info three two local
      +	)
      +'
      +
     @@ t/t5510-fetch.sh: test_expect_success 'fetch.writeCommitGraph' '
       test_expect_success 'fetch.writeCommitGraph with submodules' '
       	test_config_global protocol.file.allow always &&
       	git clone dups super &&
     +
     + ## t/t5537-fetch-shallow.sh ##
     +@@ t/t5537-fetch-shallow.sh: 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
     ++	) &&
     ++	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
     ++	) &&
     ++	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
     ++	)
     ++'
     ++
     + test_expect_success 'fetch --update-shallow' '
     + 	(
     + 	cd shallow &&

gitgitgadget

From: Kristofer Karlsson <[email protected]>

The test infrastructure has no way to check whether a specific commit is present in the commit-graph, making it hard to verify graph state after operations like fetch.

Add a "commit-info" subcommand to test-tool read-graph that queries whether specific commits are present in the commit-graph and prints their generation numbers. Returns 1 if any commit is not found.

---
 t/helper/test-read-graph.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

index 9f07b9c25a..0ab9cf8f2b 100644
@@ -2,6 +2,9 @@
 
 #include "test-tool.h"
 #include "commit-graph.h"
+#include "commit.h"
+#include "hex.h"
+#include "object-name.h"
 #include "repository.h"
 #include "odb.h"
 #include "bloom.h"

@@ -91,7 +94,25 @@ int cmd__read_graph(int argc, const char **argv)
 		dump_graph_info(graph);
 	else if (!strcmp(argv[1], "bloom-filters"))
 		dump_graph_bloom_filters(graph);
-	else {
+	else if (!strcmp(argv[1], "commit-info")) {
+		int i;
+		for (i = 2; i < argc; i++) {
+			struct object_id oid;
+			struct commit *c;
+
+			if (repo_get_oid(the_repository, argv[i], &oid))
+				die("not a valid object name: '%s'", argv[i]);
+			c = lookup_commit_in_graph(the_repository, &oid);
+			if (!c) {
+				fprintf(stderr, "%s: not in graph\n", argv[i]);
+				ret = 1;
+				continue;
+			}
+			printf("%s generation %"PRIuMAX"\n",
+			       oid_to_hex(&oid),
+			       (uintmax_t)commit_graph_generation(c));
+		}
+	} else {
 		fprintf(stderr, "unknown sub-command: '%s'\n", argv[1]);
 		ret = 1;
 	}

gitgitgadget

From: Kristofer Karlsson <[email protected]>

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.

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.

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 reachability closure also covers auto-followed tags, since their targets are reachable from the fetched tips that caused them to be auto-followed.

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.

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.

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.

The effect was measured on a synthetic setup: git.git with 200K extra packed refs (~206K total), a local file:// remote, an existing split commit-graph and a warm page cache. The times below are the median of 9 runs of the trace2 region 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

---
 builtin/fetch.c          | 69 ++++++++++++++++++++++++++++++++++------
 commit-graph.c           |  2 +-
 commit-graph.h           |  1 +
 t/t5510-fetch.sh         | 59 ++++++++++++++++++++++++++++++++++
 t/t5537-fetch-shallow.sh | 28 ++++++++++++++++
 5 files changed, 149 insertions(+), 10 deletions(-)

index 533fdfe7d8..574c361530 100644
@@ -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.
+		 */
+		if (rm->status == REF_STATUS_REJECT_SHALLOW)
+			continue;
+		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);
+	}
+}
+
 static int do_fetch(struct transport *transport,
 		    struct refspec *rs,
 		    const struct fetch_config *config,
-		    struct list_objects_filter_options *filter_options)
+		    struct list_objects_filter_options *filter_options,
+		    struct oidset *updated_tips)
 {
 	struct ref_transaction *transaction = NULL;
 	struct ref *ref_map = NULL;

@@ -2111,6 +2135,8 @@ static int do_fetch(struct transport *transport,
 
 	commit_fetch_head(&fetch_head);
 
+	collect_updated_tips(updated_tips, ref_map);
+
 	if (set_upstream) {
 		struct branch *branch = branch_get("HEAD");
 		struct ref *rm;

@@ -2427,7 +2453,8 @@ static inline void fetch_one_setup_partial(struct remote *remote,
 static int fetch_one(struct remote *remote, int argc, const char **argv,
 		     int prune_tags_ok, int use_stdin_refspecs,
 		     const struct fetch_config *config,
-		     struct list_objects_filter_options *filter_options)
+		     struct list_objects_filter_options *filter_options,
+		     struct oidset *updated_tips)
 {
 	struct refspec rs = REFSPEC_INIT_FETCH(the_hash_algo);
 	int i;

@@ -2494,7 +2521,8 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
 	sigchain_push_common(unlock_pack_on_signal);
 	atexit(unlock_pack_atexit);
 	sigchain_push(SIGPIPE, SIG_IGN);
-	exit_code = do_fetch(gtransport, &rs, config, filter_options);
+	exit_code = do_fetch(gtransport, &rs, config, filter_options,
+			     updated_tips);
 	sigchain_pop(SIGPIPE);
 	refspec_clear(&rs);
 	transport_disconnect(gtransport);

@@ -2535,6 +2563,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 +2856,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;

@@ -2899,11 +2939,21 @@ int cmd_fetch(int argc,
 		if (progress)
 			commit_graph_flags |= COMMIT_GRAPH_WRITE_PROGRESS;
 
-		trace2_region_enter("fetch", "write-commit-graph", the_repository);
-		write_commit_graph_reachable(the_repository->objects->sources,
-					     commit_graph_flags,
-					     NULL);
-		trace2_region_leave("fetch", "write-commit-graph", the_repository);
+		if (graph_write_mode != GRAPH_WRITE_SKIP) {
+			trace2_region_enter("fetch", "write-commit-graph",
+					    the_repository);
+			if (graph_write_mode == GRAPH_WRITE_TIPS)
+				write_commit_graph(
+					the_repository->objects->sources,
+					NULL, &updated_tips,
+					commit_graph_flags, NULL);
+			else
+				write_commit_graph_reachable(
+					the_repository->objects->sources,
+					commit_graph_flags, NULL);
+			trace2_region_leave("fetch", "write-commit-graph",
+					    the_repository);
+		}
 	}
 
 	if (enable_auto_gc) {

@@ -2927,6 +2977,7 @@ int cmd_fetch(int argc,
 	}
 
  cleanup:
+	oidset_clear(&updated_tips);
 	string_list_clear(&list, 0);
 	list_objects_filter_release(&filter_options);
 	return result;

index 983c11ce85..d042752ff4 100644
@@ -733,7 +733,7 @@ struct commit_graph *read_commit_graph_one(struct odb_source *source)
  * On the first invocation, this function attempts to load the commit
  * graph if the repository is configured to have one.
  */
-static struct commit_graph *prepare_commit_graph(struct repository *r)
+struct commit_graph *prepare_commit_graph(struct repository *r)
 {
 	struct odb_source *source;
 

index 13ca4ff010..7e48b0ccc0 100644
@@ -31,6 +31,7 @@ struct string_list;
 
 char *get_commit_graph_filename(struct odb_source *source);
 char *get_commit_graph_chain_filename(struct odb_source *source);
+struct commit_graph *prepare_commit_graph(struct repository *r);
 int open_commit_graph(const char *graph_file, int *fd, struct stat *st);
 int open_commit_graph_chain(const char *chain_file, int *fd, struct stat *st,
 			    const struct git_hash_algo *hash_algo);

index f323ceebd2..624bd124be 100755
@@ -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
+	) &&
+	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
+	) &&
+	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
+	)
+'
+
 test_expect_success 'fetch --update-shallow' '
 	(
 	cd shallow &&

gitgitgadget

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.

Likewise.

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.

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.

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.

Will fix, since I am rerolling anyway.
(And will keep in mind for the future.)

Will simplify and shorten it significantly. Something like this:

This relies on the commit-graph write being additive, keeping the commits that are already in the graph. fetch already operates in this mode (COMMIT_GRAPH_WRITE_SPLIT) and now that becomes required for correctness. Without that mode, the write would replace the commit-graph and lose other commits.

That's fair -- I added it because I wanted to convince myself that auto-followed tags don't need special handling, but as you say, their commits are reachable from the fetched tips anyway. Will remove.

Yes, removing this.

Right, I will simplify the text here a bit to:

Shallow-rejected refs are not stored and their history is incomplete, so skip them.

The reference was meant to point out that store_updated_refs() skips these refs as well (which is also why the full reachable scan never runs into them), but that is indirect, so best to just remove it.

Sometimes the blocks help for clarity, but this is short enough anyway so you're right it's not needed.

I will update to that, using --no-tag, since the fetch would otherwise auto-follow the new tags and they would show up in the for-each-ref check.

Yes, will fix that too.

Thanks,
Kristofer

When fetch.writeCommitGraph is enabled, the commit-graph is rebuilt from all reachable refs after every fetch. This is unnecessarily expensive on repositories with many refs, since add_ref_to_set() validates each ref against the odb.

This series optimizes the commit-graph write by using only the newly updated refs as seeds instead of scanning all refs. Since fetch writes the commit-graph in split mode, the newly fetched history is added as a new layer on top of the existing chain. A three-mode enum (REACHABLE / TIPS / SKIP) makes the policy explicit:

* No-op fetch: skip the commit-graph write entirely
* Updated refs + existing graph: write incrementally from updated tips only
* No existing graph or multi-remote fetch: fall back to full reachable scan

Patch 1 adds a commit-info subcommand to test-tool read-graph for verifying graph contents in tests.

Patch 2 implements the optimization in builtin/fetch.c with tests covering the incremental, unrelated-commit, no-op, fallback, and shallow-rejected cases.

Benchmark on a synthetic setup: git.git with 200K extra packed refs (~206K total), a local file:// remote, an existing split commit-graph and a warm page cache. Times are the median of 9 runs of the trace2 region 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

Changes since v2:

* Trim the commit message: inline the commit references, keep the explanation of why the incremental write relies on split mode, and drop the paragraphs that only restated the diff (collecting the tips, auto-followed tags, skipping shallow-rejected refs).
* Reword the comment on skipping shallow-rejected refs without the reference to store_updated_refs().
* Simplify the t5537 test by using "test_commit -C ... --no-tag" instead of subshells.

Changes since v1:

* Explain in the commit message that fetch writes in split mode, so the new tips are added as a new layer on top of the existing chain, and that the incremental path relies on this (a non-split write would replace the graph with only the closure of the seeds).
* Extend the incremental test to check that a local-only commit, which was in the graph before the fetch but is not reachable from the fetched tips, is still in the graph afterwards.
* Add benchmark numbers to the commit message.
* Add a comment explaining why shallow-rejected refs are skipped when collecting the updated tips (like store_updated_refs(), since their history is incomplete), and mention it in the commit message.
* Add a test in t5537 for a fetch with fetch.writeCommitGraph where a ref is rejected because it would require changes to .git/shallow. Without the check, the commit-graph write dies on the missing parent.

Kristofer Karlsson (2):
test-tool read-graph: add commit-info subcommand
fetch: write commit-graph using updated refs only

 builtin/fetch.c            | 69 +++++++++++++++++++++++++++++++++-----
 commit-graph.c             |  2 +-
 commit-graph.h             |  1 +
 t/helper/test-read-graph.c | 23 ++++++++++++-
 t/t5510-fetch.sh           | 59 ++++++++++++++++++++++++++++++++
 t/t5537-fetch-shallow.sh   | 22 ++++++++++++
 6 files changed, 165 insertions(+), 11 deletions(-)

base-commit: 0f8e75abebff0877cae681a3d5ff31ac47f54220
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2239%2Fspkrka%2Fkrka%2Fincremental-commit-graph-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2239/spkrka/krka/incremental-commit-graph-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2239

Range-diff vs v2:

 1:  36cc3ff3b3 = 1:  36cc3ff3b3 test-tool read-graph: add commit-info subcommand
 2:  7507354cc9 ! 2:  01da9857bc fetch: write commit-graph using updated refs only

@@ Metadata
## Commit message ##
fetch: write commit-graph using updated refs only

- 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:
+ 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.

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

@@ Commit message
(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 reachability closure also covers auto-followed tags, since their
- targets are reachable from the fetched tips that caused them to be
- auto-followed.
-
- 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.
+ This relies on the commit-graph write being additive, keeping the
+ commits that are already in the graph. fetch already operates in
+ this mode (COMMIT_GRAPH_WRITE_SPLIT) and now that becomes
+ required for correctness. Without that mode, the write would
+ replace the commit-graph and lose other commits.

After fetch_one() returns, call prepare_commit_graph() (which is made non-static by this commit) to determine the graph-write mode:
@@ builtin/fetch.c: out:
+ 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.
++ * Shallow-rejected refs are not stored and their history
++ * is incomplete, so skip them.
+ */
+ if (rm->status == REF_STATUS_REJECT_SHALLOW)
+ continue;
@@ t/t5537-fetch-shallow.sh: test_expect_success 'fetch that requires changes in .g

+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
-+ ) &&
++ git -C shallow-graph checkout --orphan no-shallow &&
++ test_commit -C shallow-graph --no-tag 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
-+ ) &&
++ test_commit -C shallow-graph --no-tag no-shallow-2 &&
+ rejected=$(git -C shallow-graph rev-parse main) &&
+ (
+ cd notshallow-graph &&

gitgitgadget

From: Kristofer Karlsson <[email protected]>

The test infrastructure has no way to check whether a specific commit is present in the commit-graph, making it hard to verify graph state after operations like fetch.

Add a "commit-info" subcommand to test-tool read-graph that queries whether specific commits are present in the commit-graph and prints their generation numbers. Returns 1 if any commit is not found.

---
 t/helper/test-read-graph.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

index 9f07b9c25a..0ab9cf8f2b 100644
@@ -2,6 +2,9 @@
 
 #include "test-tool.h"
 #include "commit-graph.h"
+#include "commit.h"
+#include "hex.h"
+#include "object-name.h"
 #include "repository.h"
 #include "odb.h"
 #include "bloom.h"

@@ -91,7 +94,25 @@ int cmd__read_graph(int argc, const char **argv)
 		dump_graph_info(graph);
 	else if (!strcmp(argv[1], "bloom-filters"))
 		dump_graph_bloom_filters(graph);
-	else {
+	else if (!strcmp(argv[1], "commit-info")) {
+		int i;
+		for (i = 2; i < argc; i++) {
+			struct object_id oid;
+			struct commit *c;
+
+			if (repo_get_oid(the_repository, argv[i], &oid))
+				die("not a valid object name: '%s'", argv[i]);
+			c = lookup_commit_in_graph(the_repository, &oid);
+			if (!c) {
+				fprintf(stderr, "%s: not in graph\n", argv[i]);
+				ret = 1;
+				continue;
+			}
+			printf("%s generation %"PRIuMAX"\n",
+			       oid_to_hex(&oid),
+			       (uintmax_t)commit_graph_generation(c));
+		}
+	} else {
 		fprintf(stderr, "unknown sub-command: '%s'\n", argv[1]);
 		ret = 1;
 	}

gitgitgadget

From: Kristofer Karlsson <[email protected]>

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.

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.

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

This relies on the commit-graph write being additive, keeping the commits that are already in the graph. fetch already operates in this mode (COMMIT_GRAPH_WRITE_SPLIT) and now that becomes required for correctness. Without that mode, the write would replace the commit-graph and lose other commits.

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.

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.

The effect was measured on a synthetic setup: git.git with 200K extra packed refs (~206K total), a local file:// remote, an existing split commit-graph and a warm page cache. The times below are the median of 9 runs of the trace2 region 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

---
 builtin/fetch.c          | 69 ++++++++++++++++++++++++++++++++++------
 commit-graph.c           |  2 +-
 commit-graph.h           |  1 +
 t/t5510-fetch.sh         | 59 ++++++++++++++++++++++++++++++++++
 t/t5537-fetch-shallow.sh | 22 +++++++++++++
 5 files changed, 143 insertions(+), 10 deletions(-)

index 533fdfe7d8..d73eca77aa 100644
@@ -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;
+		/*
+		 * Shallow-rejected refs are not stored and their history
+		 * is incomplete, so skip them.
+		 */
+		if (rm->status == REF_STATUS_REJECT_SHALLOW)
+			continue;
+		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);
+	}
+}
+
 static int do_fetch(struct transport *transport,
 		    struct refspec *rs,
 		    const struct fetch_config *config,
-		    struct list_objects_filter_options *filter_options)
+		    struct list_objects_filter_options *filter_options,
+		    struct oidset *updated_tips)
 {
 	struct ref_transaction *transaction = NULL;
 	struct ref *ref_map = NULL;

@@ -2111,6 +2135,8 @@ static int do_fetch(struct transport *transport,
 
 	commit_fetch_head(&fetch_head);
 
+	collect_updated_tips(updated_tips, ref_map);
+
 	if (set_upstream) {
 		struct branch *branch = branch_get("HEAD");
 		struct ref *rm;

@@ -2427,7 +2453,8 @@ static inline void fetch_one_setup_partial(struct remote *remote,
 static int fetch_one(struct remote *remote, int argc, const char **argv,
 		     int prune_tags_ok, int use_stdin_refspecs,
 		     const struct fetch_config *config,
-		     struct list_objects_filter_options *filter_options)
+		     struct list_objects_filter_options *filter_options,
+		     struct oidset *updated_tips)
 {
 	struct refspec rs = REFSPEC_INIT_FETCH(the_hash_algo);
 	int i;

@@ -2494,7 +2521,8 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
 	sigchain_push_common(unlock_pack_on_signal);
 	atexit(unlock_pack_atexit);
 	sigchain_push(SIGPIPE, SIG_IGN);
-	exit_code = do_fetch(gtransport, &rs, config, filter_options);
+	exit_code = do_fetch(gtransport, &rs, config, filter_options,
+			     updated_tips);
 	sigchain_pop(SIGPIPE);
 	refspec_clear(&rs);
 	transport_disconnect(gtransport);

@@ -2535,6 +2563,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 +2856,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;

@@ -2899,11 +2939,21 @@ int cmd_fetch(int argc,
 		if (progress)
 			commit_graph_flags |= COMMIT_GRAPH_WRITE_PROGRESS;
 
-		trace2_region_enter("fetch", "write-commit-graph", the_repository);
-		write_commit_graph_reachable(the_repository->objects->sources,
-					     commit_graph_flags,
-					     NULL);
-		trace2_region_leave("fetch", "write-commit-graph", the_repository);
+		if (graph_write_mode != GRAPH_WRITE_SKIP) {
+			trace2_region_enter("fetch", "write-commit-graph",
+					    the_repository);
+			if (graph_write_mode == GRAPH_WRITE_TIPS)
+				write_commit_graph(
+					the_repository->objects->sources,
+					NULL, &updated_tips,
+					commit_graph_flags, NULL);
+			else
+				write_commit_graph_reachable(
+					the_repository->objects->sources,
+					commit_graph_flags, NULL);
+			trace2_region_leave("fetch", "write-commit-graph",
+					    the_repository);
+		}
 	}
 
 	if (enable_auto_gc) {

@@ -2927,6 +2977,7 @@ int cmd_fetch(int argc,
 	}
 
  cleanup:
+	oidset_clear(&updated_tips);
 	string_list_clear(&list, 0);
 	list_objects_filter_release(&filter_options);
 	return result;

index 983c11ce85..d042752ff4 100644
@@ -733,7 +733,7 @@ struct commit_graph *read_commit_graph_one(struct odb_source *source)
  * On the first invocation, this function attempts to load the commit
  * graph if the repository is configured to have one.
  */
-static struct commit_graph *prepare_commit_graph(struct repository *r)
+struct commit_graph *prepare_commit_graph(struct repository *r)
 {
 	struct odb_source *source;
 

index 13ca4ff010..7e48b0ccc0 100644
@@ -31,6 +31,7 @@ struct string_list;
 
 char *get_commit_graph_filename(struct odb_source *source);
 char *get_commit_graph_chain_filename(struct odb_source *source);
+struct commit_graph *prepare_commit_graph(struct repository *r);
 int open_commit_graph(const char *graph_file, int *fd, struct stat *st);
 int open_commit_graph_chain(const char *chain_file, int *fd, struct stat *st,
 			    const struct git_hash_algo *hash_algo);

index f323ceebd2..16bfaba0f0 100755
@@ -135,6 +135,28 @@ 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 &&
+	git -C shallow-graph checkout --orphan no-shallow &&
+	test_commit -C shallow-graph --no-tag no-shallow &&
+	git init notshallow-graph &&
+	git -C notshallow-graph -c fetch.writeCommitGraph=true \
+		fetch ../shallow-graph/.git "refs/heads/*:refs/remotes/shallow/*" &&
+	test_commit -C shallow-graph --no-tag 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
+	)
+'
+
 test_expect_success 'fetch --update-shallow' '
 	(
 	cd shallow &&

gitgitgadget

Everything from here...

... to here is still overly verbose, especially the last paragraph. But I haven't been complaining about that in the last round, and the rest reads significantly better now. So this is not worth another reroll, if you ask me.

Other than that I'm happy with this series now, thanks!

Patrick

I thought the last paragraph was useful for motivating the change, but I agree it could be written more compactly.

Will change it if I need to reroll anyway, but will keep it as-is otherwise.

Thanks for reviewing this!
Kristofer