Re: [PATCH 2/2] submodule: use submodule repository when preparing summary

From: Michael Forney <mforney@mforney.org>
Date: Jun 30, 2020, 11:04 UTC
Message-ID: <CAGw6cBsctN0-BP6k7p71-edsHR4BxJWai4Qz5m5gi4J6pYh=Kw@mail.gmail.com>
In-Reply-To: <33de1078-5f19-e76c-2a30-1754494d1e31@gmail.com>

On 2020-06-24, Derrick Stolee <stolee@gmail.com> wrote:

> Perhaps the test I requested in patch 1 is only appropriate
> here? Or, maybe the test should be test_expect_failure in the
> first patch and switched to test_expect_success here?

I made a good effort to write a test, but I am still unable to reliably trigger the offending codepath, which is:

submodule.c:prepare_submodule_summary revision.c:prepare_revision_walk revision.c:limit_list revision.c:process_parents commit.c:repo_parse_commit_gently commit.c:repo_parse_commit_internal (needs !item->object.parsed and use_commit_graph) commit-graph.c:parse_commit_in_graph commit-graph.c:parse_commit_in_graph_one commit-graph.c:fill_commit_in_graph (needs pos >= number of commits in commit-graph in parent repository)

The trick seems to be ensuring that the parent commit of the first commit in the range of commits changed in a submodule does not get parsed during show_submodule_header, is not a loose object in the repository, and has an index in the commit-graph that is larger than the size of the commit-graph in the parent repository. This seems to depend on the order of commits in the commit-graph, which seems to be random (perhaps based on commit hashes?).

I attached my best attempt at a test to trigger the error. The probability of the test failing correctly (without the fix applied) seems to depend on how many commits are present in submodule before the first commit in the range of changed commits. This can be controlled by adjusting the `seq 1 X` in the for loop. The lowest number of commits with which I have been able to reproduce the bug is 3, where it occurs around 1% of the time, and if I set it to 200, I can reproduce the bug around 99% of the time.

I don't really want to spend more time on this than I already have. Can the bug fix be applied without a test? If not, hopefully someone can volunteer to craft a reliable test (assuming that this is even possible).

-Michael