[2/2] submodule: use submodule repository when preparing summary
diff mbox series

Message ID 20200623205659.14297-2-mforney@mforney.org
State New
Headers show
Series
  • [1/2] revision: use repository from rev_info when parsing commits
Related show

Commit Message

Michael Forney June 23, 2020, 8:56 p.m. UTC
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(-)

Comments

Derrick Stolee June 24, 2020, 2:35 p.m. UTC | #1
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
Junio C Hamano June 24, 2020, 4:12 p.m. UTC | #2
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.
Michael Forney June 30, 2020, 11:04 a.m. UTC | #3
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

Patch
diff mbox series

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;
 	}