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

[Jeff King]

On Thu, Jan 11, 2018 at 07:57:51PM +0100, René Scharfe wrote:

> > 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?

I think bisect_rev_setup() has to munge that into "^" and non-"^"
arguments. Though arguably we could shove stuff into the pending commit
list directly.

I dunno. It may not be worth spending more time on it.

-Peff

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

[René Scharfe]

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(, 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();
>>  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(_copy, ALL_REV_FLAGS);
>> -
>> -object_array_clear(_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é

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

[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(, 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();
>   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(_copy, ALL_REV_FLAGS);
> -
> - object_array_clear(_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

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

[René Scharfe]

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.

Let check_good_are_ancestors_of_bad() create and own the array of bad
and good commits, and use it to clear the commit marks as well.

Signed-off-by: Rene Scharfe 
---
 bisect.c | 30 +-
 1 file changed, 9 insertions(+), 21 deletions(-)

diff --git a/bisect.c b/bisect.c
index 0fca17c02b..c02accaf3c 100644
--- a/bisect.c
+++ b/bisect.c
@@ -790,100 +790,88 @@ static void handle_skipped_merge_base(const struct 
object_id *mb)
  * - If one is "skipped", we can't know but we should warn.
  * - If we don't know, we should check it out and ask the user to test.
  */
-static void check_merge_bases(int no_checkout)
+static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
 {
struct commit_list *result;
-   int rev_nr;
-   struct commit **rev = get_bad_and_good_commits(_nr);
 
result = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1);
 
for (; result; result = result->next) {
const struct object_id *mb = >item->object.oid;
if (!oidcmp(mb, current_bad_oid)) {
handle_bad_merge_base();
} else if (0 <= oid_array_lookup(_revs, mb)) {
continue;
} else if (0 <= oid_array_lookup(_revs, mb)) {
handle_skipped_merge_base(mb);
} else {
printf(_("Bisecting: a merge base must be tested\n"));
exit(bisect_checkout(mb, no_checkout));
}
}
 
-   free(rev);
free_commit_list(result);
 }
 
-static int check_ancestors(const char *prefix)
+static int check_ancestors(int rev_nr, struct commit **rev, const char *prefix)
 {
struct rev_info revs;
-   struct object_array pending_copy;
int res;
 
bisect_rev_setup(, prefix, "^%s", "%s", 0);
 
-   /* 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();
res = (revs.commits != NULL);
 
/* Clean up objects used, as they will be reused. */
-   clear_commit_marks_for_object_array(_copy, ALL_REV_FLAGS);
-
-   object_array_clear(_copy);
+   clear_commit_marks_many(rev_nr, rev, ALL_REV_FLAGS);
 
return res;
 }
 
 /*
  * "check_good_are_ancestors_of_bad" checks that all "good" revs are
  * ancestor of the "bad" rev.
  *
  * If that's not the case, we need to check the merge bases.
  * If a merge base must be tested by the user, its source code will be
  * checked out to be tested by the user and we will exit.
  */
 static void check_good_are_ancestors_of_bad(const char *prefix, int 
no_checkout)
 {
char *filename = git_pathdup("BISECT_ANCESTORS_OK");
struct stat st;
-   int fd;
+   int fd, rev_nr;
+   struct commit **rev;
 
if (!current_bad_oid)
die(_("a %s revision is needed"), term_bad);
 
/* Check if file BISECT_ANCESTORS_OK exists. */
if (!stat(filename, ) && S_ISREG(st.st_mode))
goto done;
 
/* Bisecting with no good rev is ok. */
if (good_revs.nr == 0)
goto done;
 
/* Check if all good revs are ancestor of the bad rev. */
-   if (check_ancestors(prefix))
-   check_merge_bases(no_checkout);
+   rev = get_bad_and_good_commits(_nr);
+   if (check_ancestors(rev_nr, rev, prefix))
+   check_merge_bases(rev_nr, rev, no_checkout);
+   free(rev);
 
/* Create file BISECT_ANCESTORS_OK. */
fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
if (fd < 0)
warning_errno(_("could not create file '%s'"),
  filename);
else
close(fd);
  done:
free(filename);
 }
 
 /*
  * This does "git diff-tree --pretty COMMIT" without one fork+exec.
  */
-- 
2.15.1