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

From: Taylor Blau <me@ttaylorr.com>
Date: Jul 28, 2020, 15:30 UTC
Message-ID: <20200728153042.GC87373@syl.lan>
In-Reply-To: <b997b649-cfeb-4b55-9c83-1c0ee2a5677c@gmail.com>

On Tue, Jul 28, 2020 at 09:00:51AM -0400, Derrick Stolee wrote:

Show 11 quoted lines

> On 7/28/2020 5:13 AM, Abhishek Kumar via GitGitGadget wrote:
> > 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.
>
> You mentioned this in your blog post. I'm sorry that such a small
> issue caused you pain. Perhaps you could summarize a little bit of
> how that investigation led you to find this issue?

Indeed ;-). I feel like forgetting to call 'parse_commit_gently()' is a rite of passage for this part of the code in some sense.

Show 5 quoted lines

> Question: is this something that is only necessary when we change
> the generation number, or is it something that is only _exposed_
> by the test suite when we change the generation number? It seems that
> it is likely to be an existing bug, but it might be hard to expose
> in a test case.

I tend to agree that this bug probably existed before Abhishek's changes, but that it's probably more trouble than it's worth to tickle with a test case. So, I'd be fine with this fix as it is (provided that the style nit is addressed below, too).

Show 20 quoted lines

> > 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)
> > +			return ;
>
> Drop the extra space.
>
> Thanks,
> -Stolee

Thanks, Taylor