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

From: René Scharfe <l.s.r@web.de>
Date: Jan 11, 2018, 18:57 UTC
Message-ID: <b0fbedea-77d8-5571-6042-03b512c0680d@web.de>
In-Reply-To: <20180110080754.GE16315@sigill.intra.peff.net>

Am 10.01.2018 um 09:07 schrieb Jeff King:

Show 26 quoted lines

> On Mon, Dec 25, 2017 at 06:45:36PM +0100, René Scharfe wrote:
> 
>> 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...
> 
>>   	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...

Yes, namely from the global variables current_bad_oid and good_revs.

Show 22 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...
> 
>>   
>>   	/* 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".

... which is populated by get_bad_and_good_commits() using the global variables current_bad_oid and good_revs.

> 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).

That's done already under the covers. De-globalizing these variables would make this visible.

Another way would be to store the bad and good revs in a format that allows them to be used everywhere, thus avoiding confusing duplication/conversions. Commit pointers and arrays thereof should work everywhere we currently use object_ids and oid_arrays for bad and good revs, right?

René