| Message ID | 20200623205659.14297-2-mforney@mforney.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 85a1ec2c32f4f41dd4526651df4b3666a41334fe |
| Headers | show |
| Series | [1/2] revision: use repository from rev_info when parsing commits | expand |
On 6/23/2020 4:56 PM, Michael Forney wrote: > In show_submodule_header(), we gather the left and right commits > of the submodule repository, as well as the merge bases. However, > prepare_submodule_summary() initializes the rev_info with the_repository, > so we end up parsing the commit in the wrong repository. > > This results in a fatal error in parse_commit_in_graph(), since the > passed item does not belong to the repository's commit graph. > > Signed-off-by: Michael Forney <mforney@mforney.org> > --- > submodule.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/submodule.c b/submodule.c > index e2ef5698c8..785ab47629 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -438,13 +438,13 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt, > */ > } > > -static int prepare_submodule_summary(struct rev_info *rev, const char *path, > - struct commit *left, struct commit *right, > +static int prepare_submodule_summary(struct repository *r, struct rev_info *rev, > + const char *path, struct commit *left, struct commit *right, > struct commit_list *merge_bases) > { > struct commit_list *list; > > - repo_init_revisions(the_repository, rev, NULL); > + repo_init_revisions(r, rev, NULL); This is how we properly initialize the repository in the rev_info. It's unfortunate that this use of the_repository was pretty clearly incorrect. This is submodule.c, so every instance of the_repository should be examined carefully. Taking a brief look right now, the rest seem to be correct in that they are finding submodules within the super-repo. The only issue will arise when recursing into submodules, which is known to be broken in-process and are handled with subprocesses instead. > setup_revisions(0, NULL, rev, NULL); > rev->left_right = 1; > rev->first_parent_only = 1; > @@ -632,7 +632,7 @@ void show_submodule_summary(struct diff_options *o, const char *path, > goto out; > > /* Treat revision walker failure the same as missing commits */ > - if (prepare_submodule_summary(&rev, path, left, right, merge_bases)) { > + if (prepare_submodule_summary(sub, &rev, path, left, right, merge_bases)) { > diff_emit_submodule_error(o, "(revision walker failed)\n"); > goto out; > } 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? Thanks, -Stolee
Derrick Stolee <stolee@gmail.com> writes: > 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? Either is OK, but it is probably easier to read to have just one addition in 2/2 to expect succeses. Temporarily revierting with "git show ':!t' | git apply -R" before running test when you want to reallly see how the original crashed is easy and simple enough. Thanks.
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
On 2020-06-30, Michael Forney <mforney@mforney.org> wrote: > 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. > > 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). Still looking for any help with this. It seems pretty clear that this is a bug (I am not the only one who has hit this), and I'd really like to see the issue fixed. I gave my best shot at a test, but I don't think it's acceptable to commit a test that gives a false positive some percentage of the time. I see that my patch is still blocked as "Needs tests" in the what's cooking summary, but I really don't know how to proceed from here.
diff --git a/submodule.c b/submodule.c index e2ef5698c8..785ab47629 100644 --- a/submodule.c +++ b/submodule.c @@ -438,13 +438,13 @@ void handle_ignore_submodules_arg(struct diff_options *diffopt, */ } -static int prepare_submodule_summary(struct rev_info *rev, const char *path, - struct commit *left, struct commit *right, +static int prepare_submodule_summary(struct repository *r, struct rev_info *rev, + const char *path, struct commit *left, struct commit *right, struct commit_list *merge_bases) { struct commit_list *list; - repo_init_revisions(the_repository, rev, NULL); + repo_init_revisions(r, rev, NULL); setup_revisions(0, NULL, rev, NULL); rev->left_right = 1; rev->first_parent_only = 1; @@ -632,7 +632,7 @@ void show_submodule_summary(struct diff_options *o, const char *path, goto out; /* Treat revision walker failure the same as missing commits */ - if (prepare_submodule_summary(&rev, path, left, right, merge_bases)) { + if (prepare_submodule_summary(sub, &rev, path, left, right, merge_bases)) { diff_emit_submodule_error(o, "(revision walker failed)\n"); goto out; }
In show_submodule_header(), we gather the left and right commits of the submodule repository, as well as the merge bases. However, prepare_submodule_summary() initializes the rev_info with the_repository, so we end up parsing the commit in the wrong repository. This results in a fatal error in parse_commit_in_graph(), since the passed item does not belong to the repository's commit graph. Signed-off-by: Michael Forney <mforney@mforney.org> --- submodule.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)