Repository navigation
fetch: write commit-graph using updated refs only #2239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1903,10 +1903,34 @@ static int commit_ref_transaction(struct ref_transaction **transaction, | |
| return retcode; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Tue, Oct 06, 2026 at 09:46:32AM +0000, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <[email protected]>
>
> 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!
PatrickThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): On Wed, 7 Oct 2026 at 08:39, Patrick Steinhardt <[email protected]> wrote:
>
> On Tue, Oct 06, 2026 at 09:46:32AM +0000, Kristofer Karlsson via GitGitGadget wrote:
> > From: Kristofer Karlsson <[email protected]>
> >
> > 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.
Will fix, since I am rerolling anyway.
(And will keep in mind for the future.)
> > 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.
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.
> > 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.
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.
> > 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.
Yes, removing this.
>
> > 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.
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.
> > + 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
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.
>
> > + 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`?
Yes, will fix that too.
Thanks,
KristoferThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Patrick Steinhardt wrote on the Git mailing list (how to reply to this email): On Wed, Oct 07, 2026 at 02:22:57PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> 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.
Everything from here...
> 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.
... 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!
PatrickThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): On Thu, 8 Oct 2026 at 08:01, Patrick Steinhardt <[email protected]> wrote:
>
> Everything from here...
>
> > 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.
>
> ... 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!
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 |
||
| } | ||
|
|
||
| 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; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):