Re: [PATCH 2/6] revision: parse parent in indegree_walk_step()

From: Jakub Narębski <jnareb@gmail.com>
Date: Aug 5, 2020, 23:16 UTC
Message-ID: <85pn84u1j9.fsf@gmail.com>
In-Reply-To: <d23f67dc80b85abe4eba9a9dfc39d50188e23bb7.1595927632.git.gitgitgadget@gmail.com>

"Abhishek Kumar via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 21 quoted lines

> From: Abhishek Kumar <abhishekkumar8222@gmail.com>
>
> In indegree_walk_step(), we add unvisited parents to the indegree queue.
> However, parents are not guaranteed to be parsed. As the indegree queue
> sorts by generation number, let's parse parents before inserting them to
> ensure the correct priority order.
>
> Signed-off-by: Abhishek Kumar <abhishekkumar8222@gmail.com>
> ---
>  revision.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/revision.c b/revision.c
> index 6aa7f4f567..23287d26c3 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -3343,6 +3343,9 @@ static void indegree_walk_step(struct rev_info *revs)
>  		struct commit *parent = p->item;
>  		int *pi = indegree_slab_at(&info->indegree, parent);
>
> +		if (parse_commit_gently(parent, 1) < 0)

All right, parse_commit_gently() avoids re-parsing objects, and makes use of the commit-graph data. If parents are not guaranteed to be parsed, this is a correct thing to do.

Though I do wonder how this issue got missed by the test suite, just like other reviewers...

> +			return ;

Why this need to be 'return' and not 'continue'?

> +
>  		if (*pi)
>  			(*pi)++;
>  		else

Best,

-- 
Jakub Narębski