| Message ID | d23f67dc80b85abe4eba9a9dfc39d50188e23bb7.1595927632.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Implement Corrected Commit Date | expand |
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? 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. > 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
On Tue, Jul 28, 2020 at 09:00:51AM -0400, Derrick Stolee wrote: > 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. > 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). > > 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
"Abhishek Kumar via GitGitGadget" <gitgitgadget@gmail.com> writes: > 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,
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 ; + if (*pi) (*pi)++; else