Hi all,
Thank you guys for insightful help. I just read the code and now I understand
what you guys are saying. Yeah, I can say the fix is "spot on".
But, to be honest, it's hard to see why you need "if (p)" at the first glance.
So, how about this fix, instead?
I also found a bug in show_list(). I'm attaching a patch as well.
--
yashi
From d149a1348e94ea0246a10181751ce3bf9ba48198 Mon Sep 17 00:00:00 2001
From: Yasushi SHOJI <yashi@xxxxxxxxxxxxxxxxx>
Date: Mon, 8 Jan 2018 22:31:10 +0900
Subject: [PATCH 1/2] bisect: debug: convert struct object to object_id
The commit f2fd0760f62e79609fef7bfd7ecebb002e8e4ced converted struct
object to object_id but a debug function show_list(), which is
ifdef'ed to noop, in bisect.c wasn't.
So fix it.
---
bisect.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/bisect.c b/bisect.c
index 0fca17c02..fb3e44903 100644
--- a/bisect.c
+++ b/bisect.c
@@ -132,7 +132,7 @@ static void show_list(const char *debug, int counted, int nr,
unsigned flags = commit->object.flags;
enum object_type type;
unsigned long size;
- char *buf = read_sha1_file(commit->object.sha1, &type, &size);
+ char *buf = read_sha1_file(commit->object.oid.hash, &type, &size);
const char *subject_start;
int subject_len;
@@ -144,10 +144,10 @@ static void show_list(const char *debug, int counted, int nr,
fprintf(stderr, "%3d", weight(p));
else
fprintf(stderr, "---");
- fprintf(stderr, " %.*s", 8, sha1_to_hex(commit->object.sha1));
+ fprintf(stderr, " %.*s", 8, sha1_to_hex(commit->object.oid.hash));
for (pp = commit->parents; pp; pp = pp->next)
fprintf(stderr, " %.*s", 8,
- sha1_to_hex(pp->item->object.sha1));
+ sha1_to_hex(pp->item->object.oid.hash));
subject_len = find_commit_subject(buf, &subject_start);
if (subject_len)
--
2.15.1
From 2ec8823de3bd0ca8253ddbd07f58b66afcfabe96 Mon Sep 17 00:00:00 2001
From: Yasushi SHOJI <yashi@xxxxxxxxxxxxxxxxx>
Date: Mon, 8 Jan 2018 22:35:12 +0900
Subject: [PATCH 2/2] bisect: handle empty list in best_bisection_sorted()
In 7c117184d7 ("bisect: fix off-by-one error in
`best_bisection_sorted()`", 2017-11-05) the more careful logic dealing
with freeing p->next in 50e62a8e70 ("rev-list: implement
--bisect-all", 2007-10-22) was removed.
This function, and also all other functions below find_bisection() to
be complete, will be called with an empty commit list if all commits
are _uninteresting_. So the functions must be prepared for it.
This commit, instead of restoring the check, moves the clean-up code
into the loop. To do that this commit changes the non-last-case
branch in the loop to a last-case branch, and clean-up unused trailing
elements there.
We could, on the other hand, do a big "if list" condition at the very
beginning. But, that doesn't sound right for the current code base.
No other functions does that but all blocks in the functions are
tolerant to an empty list.
By the way, as you can tell from the non-last-case branch we had in
the loop, this fix shouldn't cause a pipeline stall on every iteration
on modern processors with a branch predictor.
Signed-off-by: Yasushi ShOJI <yasushi.shoji@xxxxxxxxx>
---
bisect.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/bisect.c b/bisect.c
index fb3e44903..cec4a537f 100644
--- a/bisect.c
+++ b/bisect.c
@@ -218,7 +218,7 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n
cnt++;
}
QSORT(array, cnt, compare_commit_dist);
- for (p = list, i = 0; i < cnt; i++) {
+ for (p = list, i = 0; i < cnt; i++, p = p->next) {
struct object *obj = &(array[i].commit->object);
strbuf_reset(&buf);
@@ -226,11 +226,13 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n
add_name_decoration(DECORATION_NONE, buf.buf, obj);
p->item = array[i].commit;
- if (i < cnt - 1)
- p = p->next;
+
+ if (i == cnt) {
+ /* clean-up unused elements if any */
+ free_commit_list(p->next);
+ p->next = NULL;
+ }
}
- free_commit_list(p->next);
- p->next = NULL;
strbuf_release(&buf);
free(array);
return list;
--
2.15.1
