Re: [PATCH] commit-reach: parse commits in the given repository

[Patrick Steinhardt <ps@pks.im>]

On Wed, Sep 16, 2026 at 03:46:31PM +0200, Orestis Floros wrote:
> `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.

> 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