Re: Possible git bug in commit-graph: "invalid commit position"

From: Michael Forney <mforney@mforney.org>
Date: Jun 21, 2020, 20:45 UTC
Message-ID: <CAGw6cBuEshq18O_PrrGYuJi5VZ82XK3T9KuShneUqO2Ju0jtHw@mail.gmail.com>
In-Reply-To: <20200515120333.GA4677@pflmari>

On 2020-05-15, Alex Riesen <alexander.riesen@cetitec.com> wrote:

Show 22 quoted lines

> Gary Oberbrunner, Tue, Feb 04, 2020 23:33:42 +0100:
>> Sorry for the long reply delay; the bug went away and only just showed
>> up again. Here's the info you requested.
>> I'm now running git 2.25.0.
>
> I hit a very similar problem today with 2.26.0. Also in a submodule.
>
> Removing and regenerating the commit graph did not help and I did not have
> the
> commit-graphs directory (only a file). "git commit-graph verify" does not
> find
> anything. Switching writeCommitGraph on and regenerating the commit graph
> makes no difference.
>
> I can trigger it reliably by visiting the broken(?) commit in supermodule
> with:
>
>     git show --submodule=log <commit>
>
> I see nothing special in the commit invovled. It is just a single commit in
> my
> case, and the commit is a merge of two branches.

I hit this bug a while back, and it went away after I deleted the commit-graph in the submodule and regenerated it (IIRC).

I just ran into it again (on 2.27.0), and this time, I did some digging.

I have a repository containing a number of submodules, and the bug appeared after I updated one of the submodules, and then looked at `git log -p` with diff.submodule = log. Just like Alex, I can reliably trigger the error with `git show --submodule=log <commit>`.

I rebuilt git with some print statements to try to see what's going on, and got the following:

	/src/oasis/.git/modules/pkg/file/src c81d1ccbf4c224af50e6d556419961dba72666c7
		pos: 4986, num_commits: 6452, num_commits_in_base: 0
	/src/oasis/.git/modules/pkg/file/src 9f2f793847c6aeab9501287b6847dc842c84630f
		pos: 3964, num_commits: 6452, num_commits_in_base: 0
	/src/oasis/.git/modules/pkg/file/src fd7eb1f793944635b92bfa56a84a4dc1dbefb119
		pos: 6383, num_commits: 6452, num_commits_in_base: 0
	/src/oasis/.git/modules/pkg/file/src d955cefc956ba537cfc0556023a65fe80bd2d82b
		pos: 5436, num_commits: 6452, num_commits_in_base: 0
	/src/oasis/.git/modules/pkg/file/src 0c79c693d6a86f7ad7ada2a9a1eb3bdf483f77cc
		pos: 301, num_commits: 6452, num_commits_in_base: 0
	.git fa09b87efa9b9664e4e53ab768cfa5f51a6c6fa2
		pos: 6292, num_commits: 5177, num_commits_in_base: 0
	fatal: invalid commit position. commit-graph is likely corrupt

Using `git commit-graph verify`, I confirmed that the main repository's commit graph contains 5177 commits, and the submodule repository's commit-graph contains 6452 commits. Commit fa09b8 is part of the submodule, not the main repository, so it makes sense that it is an invalid commit for the main repositories commit-graph.

So, this seems a little fishy. fill_commit_in_graph is getting called with the main repository and a commit belonging to the submodule. Looking through the call stack in gdb, I see that the initial calls to fill_commit_in_graph come from show_submodule_header, which computes left, right, and merge_bases. Then, those commits are passed to prepare_submodule_summary, but this function does *not* accept a submodule parameter. prepare_submodule_summary calls repo_init_revisions with the_repository, which seems to be the source of the problem. I think it should be using the submodule repository instead.

I changed prepare_submodule_summary to accept a repository and to use that instead, but the issue persisted. Digging deeper, this is because revision.c:process_parents uses parse_commit_gently, which is a synonym for repo_parse_commit_gently(the_repository, ...). I changed it to use repo_parse_commit_gently(revs->repo, ...), and this time, the problem went away.

I'm not very familiar with the git codebase, but am I on the right track here? I also noticed a number of other calls to parse_commit_gently in revision.c, and I think those should pass revs->repo as well. Does that sound right? If so, I can send a patch to fix these issues.

-Michael