Am 10.01.2018 um 09:07 schrieb Jeff King:
> 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.
>> - /* 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é
