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