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

[Michael Forney <mforney@mforney.org>]

[-- Attachment #1: Type: text/plain, Size: 1967 bytes --]

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

[-- Attachment #2: t7421-submodule-commit-graph.sh --]
[-- Type: application/x-sh, Size: 721 bytes --]