Re: [PATCH v2 5/9] bisect: avoid using the rev_info flag leak_pending

From: Jeff King <peff@peff.net>
Date: Jan 10, 2018, 08:07 UTC
Message-ID: <20180110080754.GE16315@sigill.intra.peff.net>
In-Reply-To: <07d0e0c6-d023-bd28-511b-78092cb35b4f@web.de>

On Mon, Dec 25, 2017 at 06:45:36PM +0100, René Scharfe wrote:

Show 5 quoted lines

> The leak_pending flag is so awkward to use that multiple comments had to
> be added around each occurrence.  We only use it for remembering the
> commits whose marks we have to clear after checking if all of the good
> ones are ancestors of the bad one.  This is easy, though: We need to do
> that for the bad and good commits, of course.

Are we sure that our list is the same as what is traversed? I won't be surprised if it is true, but it doesn't seem immediately obvious from the code:

> -static int check_ancestors(const char *prefix)
> +static int check_ancestors(int rev_nr, struct commit **rev, const char *prefix)
>  {

So now we take in a set of objects...

Show 5 quoted lines

>  	struct rev_info revs;
> -	struct object_array pending_copy;
>  	int res;
>  
>  	bisect_rev_setup(&revs, prefix, "^%s", "%s", 0);

But those objects aren't provided here. bisect_rev_setup() puts its own set of objects into the pending list...

Show 11 quoted lines

> -	/* Save pending objects, so they can be cleaned up later. */
> -	pending_copy = revs.pending;
> -	revs.leak_pending = 1;
> -
> -	/*
> -	 * bisect_common calls prepare_revision_walk right away, which
> -	 * (together with .leak_pending = 1) makes us the sole owner of
> -	 * the list of pending objects.
> -	 */
>  	bisect_common(&revs);
>  	res = (revs.commits != NULL);

And then we traverse, and then...

Show 6 quoted lines

>  
>  	/* Clean up objects used, as they will be reused. */
> -	clear_commit_marks_for_object_array(&pending_copy, ALL_REV_FLAGS);
> -
> -	object_array_clear(&pending_copy);
> +	clear_commit_marks_many(rev_nr, rev, ALL_REV_FLAGS);

...this is the first time we look at "rev".

If we already have the list of tips, could we just feed it ourselves to bisect_rev_setup (I think that would require us remembering which were "good" and "bad", but that doesn't seem like a big deal).

I'm not overly concerned that you've introduced a bug here, but just wondering if we could make this a bit more maintainable going forward.

-Peff