patch, 2 partsrevision: use repository from rev_info when parsing commits

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

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

> 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).

>   fatal: invalid commit position. commit-graph is likely corrupt
>
> Since revision.c was the only user of the parse_commit_gently
> compatibility define, remove it from commit.h.
>
> Signed-off-by: Michael Forney <mforney@mforney.org>

> This is needed when repo_init_revisions() is called with a repository
> that is not the_repository to ensure appropriate repository is used
> in repo_parse_commit_internal(). If the wrong repository is used,
> a fatal error is the commit-graph machinery occurs:
> 
>   fatal: invalid commit position. commit-graph is likely corrupt
> 
> Since revision.c was the only user of the parse_commit_gently
> compatibility define, remove it from commit.h.

> Signed-off-by: Michael Forney <mforney@mforney.org>
> ---
>  commit.h   |  1 -
>  revision.c | 18 +++++++++---------
>  2 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/commit.h b/commit.h
> -#define parse_commit_gently(item, quiet) repo_parse_commit_gently(the_repository, item, quiet)

> diff --git a/revision.c b/revision.c
> index ebb4d2a0f2..2b6bf47c81 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -439,7 +439,7 @@ static struct commit *handle_commit(struct rev_info *revs,
>  	if (object->type == OBJ_COMMIT) {
>  		struct commit *commit = (struct commit *)object;
>  
> -		if (parse_commit(commit) < 0)
> +		if (repo_parse_commit(revs->repo, commit) < 0)

> On 6/23/2020 4:56 PM, Michael Forney wrote:
>> This is needed when repo_init_revisions() is called with a repository
>> that is not the_repository to ensure appropriate repository is used
>> in repo_parse_commit_internal(). If the wrong repository is used,
>> a fatal error is the commit-graph machinery occurs:
>> 
>>   fatal: invalid commit position. commit-graph is likely corrupt
>> 
>> Since revision.c was the only user of the parse_commit_gently
>> compatibility define, remove it from commit.h.
>
> Is this demonstrable in a test case, to prevent regressions?

> Derrick Stolee <stolee@gmail.com> writes:
> 
>> On 6/23/2020 4:56 PM, Michael Forney wrote:
>>> This is needed when repo_init_revisions() is called with a repository
>>> that is not the_repository to ensure appropriate repository is used
>>> in repo_parse_commit_internal(). If the wrong repository is used,
>>> a fatal error is the commit-graph machinery occurs:
>>>
>>>   fatal: invalid commit position. commit-graph is likely corrupt
>>>
>>> Since revision.c was the only user of the parse_commit_gently
>>> compatibility define, remove it from commit.h.
>>
>> Is this demonstrable in a test case, to prevent regressions?
> 
> It appears that Michael tried and failed.  Even if we do not
> currently have a caller that asks these functions in revision.c to
> work on a repository that is not the primary one (i.e. in a
> submodule), in which case these patches may not be fixing any bug
> that can be triggered in the current code, it is quite obvious that
> these functions misbehave once a caller starts asking them to work
> on a repository other than the primary one.
> 
> So, given that ... 
> 
>>
>> I counted 9 copies of parse_commit[_gently]() in my version
>> of revision.c, so it looks like you caught them all.
> 
> ... we should be able to proceed with the code as-is, I guess