Re: [PATCH] commit-reach: parse commits in the given repository
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Sep 23, 2026, 12:49 UTC
- Message-ID
- <arPK8phxWv1pNG_m@pks.im>
- In-Reply-To
- <20260916134632.1424829-1-orestisflo@gmail.com>
On Wed, Sep 16, 2026 at 03:46:31PM +0200, Orestis Floros wrote:
Show 33 quoted lines
> `can_all_from_reach()` and `can_all_from_reach_with_flag()` parse the > commits they walk in `the_repository`, even though their caller may be > working in a different repository. `repo_is_descendant_of()` is such a > caller: it is told which repository to work in, but as soon as > generation numbers are enabled it hands the commits over to > `can_all_from_reach()`, which then parses them elsewhere. > > This breaks merging a superproject whose submodule pointer advanced on > both sides. merge-ort resolves it by calling `repo_in_merge_bases()` on > the submodule, and with a commit-graph in both the superproject and the > submodule the merge dies: > > $ git merge side > fatal: invalid commit position. commit-graph is likely corrupt > > `merge_submodule()` looks the submodule commits up in the submodule, so > walking their ancestry pulls in parents whose commit-graph position was > recorded while reading the submodule's commit-graph. The walk then > parses those parents in `the_repository`, where the recorded position > indexes the superproject's commit-graph instead: `fill_commit_graph_info()` > dies when the position is out of bounds, and quietly returns another > commit's date, generation and parents when it is not. > > The latter used to be the only symptom. Before bb5da75d61 (commit: use > commit graph in `lookup_commit_reference_gently()`, 2026-02-16) the > initial lookup did not record commit-graph positions, so the walk simply > failed to find the submodule commits in the superproject: > > error: Could not read <commit> > Failed to merge submodule sub (commits don't follow merge-base) > > Pass the repository into both functions. git-fetch-pack(1) and > git-upload-pack(1) keep passing `the_repository`.
Thanks for the nice explanation.
> diff --git a/commit-reach.c b/commit-reach.c > index 5df471a313..3d579d8f7f 100644 > --- a/commit-reach.c > +++ b/commit-reach.c
I'm always a fan of removing this implicit dependency. Doubly so if it actually fixes a bug.
Show 11 quoted lines
> diff --git a/t/t6437-submodule-merge.sh b/t/t6437-submodule-merge.sh > index a564758f52..afb484b963 100755 > --- a/t/t6437-submodule-merge.sh > +++ b/t/t6437-submodule-merge.sh > @@ -517,4 +517,43 @@ test_expect_success 'merging should fail with no merge base' ' > ) > ' > > +test_expect_success 'setup for commit-graphs in superproject and submodule' ' > + git init commit-graph && > + (cd commit-graph &&
I wanted to complain about formatting at first, but I see that you simply follow the preexisting style in this file. So I guess this is okay.
I also double-checked that the test indeed catches the bug.
So overall, this looks good to me. Thanks!
Patrick