Re: [PATCH v6] Implement --first-parent for git rev-list --bisect
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Sep 2, 2018, 07:34 UTC
- Message-ID
- <CACsJy8AFo+mb8R-O-JKRPZk__csq6mbVXbnZhSd-nZ08zWfSeg@mail.gmail.com>
- In-Reply-To
- <xmqqlg8qh22q.fsf@gitster-ct.c.googlers.com>
On Tue, Aug 28, 2018 at 6:45 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 23 quoted lines
> > @@ -146,10 +147,14 @@ static void show_list(const char *debug, int counted, int nr,
>
> An unrelated tangent, but I think I just spotted a bug in the
> existing code on the line immediately before this hunk, which reads
>
> if (commit->util)
> fprintf(stderr, "%3d", weight(p));
>
> I think this was a bug introduced at bb408ac9 ("bisect.c: use
> commit-slab for commit weight instead of commit->util", 2018-05-19)
> where the internal implementation of weight() was changed not to
> touch commit->util but instead to use a separate commit-slab storage
>
> Looking at the code before that conversion, it seems that we were
> using ->util to store a pointer to an integer, so we had the ability
> to differenciate non-negative weight (i.e. weight already computed
> for the commit), negative weight (i.e. not computed yet, but will
> be), and commits to which the concept of weight is not applicable.
> When we went to the commit-slab with the change, we lost the ability
> to represent the third case. I am offhand not sure what the best
> remedy would be. Perhaps stuff a so-far unused value like -3 to the
> weight() and use weight(p) == -3 instead of the old !commit->util or
> something like that?Hmm.. no? the commit-slab stores the pointer to the weight, not the weight itself, so we still have the ability to check the third case, I think.
Show 6 quoted lines
> (Duy CC'ed to help checking my sanity on this point). > > In any case, this is an existing bug in a debug helper, and the > focus of this patch is not about fixing that bug, so you can and > should leave it as-is, until this patch successfully adds the > "bisection following only the first parent" feature.
Yes. I'll post a patch soon to fix this "commit->util" leftover.
-- Duy