[PATCH v6] Implement --first-parent for git rev-list --bisect

[PATCH v6] Implement --first-parent for git rev-list --bisect

[Tiago Botelho]

This will enable users to implement bisecting on first parents
which can be useful for when the commits from a feature branch
that we want to merge are not always tested.

Signed-off-by: Tiago Botelho <tiagonbotelho@hotmail.com>
---

This patch refactors **only** the tests according to the suggestions made by Junio in v5
more specifically `https://public-inbox.org/git/xmqqa7rhi40f.fsf@gitster-ct.c.googlers.com/`.

The discussion between Junio, Christian and Johannes got pretty confusing in the sense
that I am unsure if this is the actual problem I am supposed to solve in order to get the
patch accepted, if it is not I will keep iterating on it until it is good enough to be
merged

 bisect.c                   | 43 ++++++++++++++++++++++------------
 bisect.h                   |  1 +
 builtin/rev-list.c         |  3 +++
 revision.c                 |  3 ---
 t/t6002-rev-list-bisect.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 90 insertions(+), 18 deletions(-)

diff --git a/bisect.c b/bisect.c
index 4eafc8262..cb80f29c5 100644
--- a/bisect.c
+++ b/bisect.c
@@ -82,15 +82,16 @@ static inline void weight_set(struct commit_list *elem, int weight)
 	*((int*)(elem->item->util)) = weight;
 }
 
-static int count_interesting_parents(struct commit *commit)
+static int count_interesting_parents(struct commit *commit, unsigned bisect_flags)
 {
 	struct commit_list *p;
 	int count;
 
 	for (count = 0, p = commit->parents; p; p = p->next) {
-		if (p->item->object.flags & UNINTERESTING)
-			continue;
-		count++;
+		if (!(p->item->object.flags & UNINTERESTING))
+		    count++;
+		if (bisect_flags & BISECT_FIRST_PARENT)
+			break;
 	}
 	return count;
 }
@@ -117,10 +118,10 @@ static inline int halfway(struct commit_list *p, int nr)
 }
 
 #if !DEBUG_BISECT
-#define show_list(a,b,c,d) do { ; } while (0)
+#define show_list(a,b,c,d,e) do { ; } while (0)
 #else
 static void show_list(const char *debug, int counted, int nr,
-		      struct commit_list *list)
+		      struct commit_list *list, unsigned bisect_flags)
 {
 	struct commit_list *p;
 
@@ -146,10 +147,14 @@ static void show_list(const char *debug, int counted, int nr,
 		else
 			fprintf(stderr, "---");
 		fprintf(stderr, " %.*s", 8, oid_to_hex(&commit->object.oid));
-		for (pp = commit->parents; pp; pp = pp->next)
+		for (pp = commit->parents; pp; pp = pp->next) {
 			fprintf(stderr, " %.*s", 8,
 				oid_to_hex(&pp->item->object.oid));
 
+			if (bisect_flags & BISECT_FIRST_PARENT)
+				break;
+		}
+
 		subject_len = find_commit_subject(buf, &subject_start);
 		if (subject_len)
 			fprintf(stderr, " %.*s", subject_len, subject_start);
@@ -267,13 +272,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 		unsigned flags = commit->object.flags;
 
 		p->item->util = &weights[n++];
-		switch (count_interesting_parents(commit)) {
+		switch (count_interesting_parents(commit, bisect_flags)) {
 		case 0:
 			if (!(flags & TREESAME)) {
 				weight_set(p, 1);
 				counted++;
 				show_list("bisection 2 count one",
-					  counted, nr, list);
+					  counted, nr, list, bisect_flags);
 			}
 			/*
 			 * otherwise, it is known not to reach any
@@ -289,7 +294,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 		}
 	}
 
-	show_list("bisection 2 initialize", counted, nr, list);
+	show_list("bisection 2 initialize", counted, nr, list, bisect_flags);
 
 	/*
 	 * If you have only one parent in the resulting set
@@ -319,7 +324,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 		counted++;
 	}
 
-	show_list("bisection 2 count_distance", counted, nr, list);
+	show_list("bisection 2 count_distance", counted, nr, list, bisect_flags);
 
 	while (counted < nr) {
 		for (p = list; p; p = p->next) {
@@ -329,6 +334,11 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 			if (0 <= weight(p))
 				continue;
 			for (q = p->item->parents; q; q = q->next) {
+				if ((bisect_flags & BISECT_FIRST_PARENT)) {
+					if (weight(q) < 0)
+						q = NULL;
+					break;
+				}
 				if (q->item->object.flags & UNINTERESTING)
 					continue;
 				if (0 <= weight(q))
@@ -346,7 +356,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 				weight_set(p, weight(q)+1);
 				counted++;
 				show_list("bisection 2 count one",
-					  counted, nr, list);
+					  counted, nr, list, bisect_flags);
 			}
 			else
 				weight_set(p, weight(q));
@@ -357,7 +367,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 		}
 	}
 
-	show_list("bisection 2 counted all", counted, nr, list);
+	show_list("bisection 2 counted all", counted, nr, list, bisect_flags);
 
 	if (!find_all)
 		return best_bisection(list, nr);
@@ -372,7 +382,7 @@ void find_bisection(struct commit_list **commit_list, int *reaches,
 	struct commit_list *list, *p, *best, *next, *last;
 	int *weights;
 
-	show_list("bisection 2 entry", 0, 0, *commit_list);
+	show_list("bisection 2 entry", 0, 0, *commit_list, bisect_flags);
 
 	/*
 	 * Count the number of total and tree-changing items on the
@@ -395,7 +405,7 @@ void find_bisection(struct commit_list **commit_list, int *reaches,
 		on_list++;
 	}
 	list = last;
-	show_list("bisection 2 sorted", 0, nr, list);
+	show_list("bisection 2 sorted", 0, nr, list, bisect_flags);
 
 	*all = nr;
 	weights = xcalloc(on_list, sizeof(*weights));
@@ -962,6 +972,9 @@ int bisect_next_all(const char *prefix, int no_checkout)
 	if (skipped_revs.nr)
 		bisect_flags |= BISECT_FIND_ALL;
 
+	if (revs.first_parent_only)
+		bisect_flags |= BISECT_FIRST_PARENT;
+
 	find_bisection(&revs.commits, &reaches, &all, bisect_flags);
 	revs.commits = managed_skipped(revs.commits, &tried);
 
diff --git a/bisect.h b/bisect.h
index 1d40a33ad..e445567c2 100644
--- a/bisect.h
+++ b/bisect.h
@@ -2,6 +2,7 @@
 #define BISECT_H
 
 #define BISECT_FIND_ALL		(1u<<0)
+#define BISECT_FIRST_PARENT    	(1u<<1)
 
 /*
  * Find bisection. If something is found, `reaches` will be the number of
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 8752f5bbe..66439e1b3 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -538,6 +538,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	if (bisect_list) {
 		int reaches, all;
 
+		if (revs.first_parent_only)
+			bisect_flags |= BISECT_FIRST_PARENT;
+
 		find_bisection(&revs.commits, &reaches, &all, bisect_flags);
 
 		if (bisect_show_vars)
diff --git a/revision.c b/revision.c
index 4e0e193e5..b9d063805 100644
--- a/revision.c
+++ b/revision.c
@@ -2474,9 +2474,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 	if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
 		die("cannot use --grep-reflog without --walk-reflogs");
 
-	if (revs->first_parent_only && revs->bisect)
-		die(_("--first-parent is incompatible with --bisect"));
-
 	if (revs->expand_tabs_in_log < 0)
 		revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
 
diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
index a66140803..1bc297de5 100755
--- a/t/t6002-rev-list-bisect.sh
+++ b/t/t6002-rev-list-bisect.sh
@@ -263,4 +263,62 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' '
 	test_cmp expect.sorted actual.sorted
 '
 
+# We generate the following commit graph:
+#
+#   B ------ C
+#  /          \
+# A            FX
+#  \          /
+#   D - CC - EX
+
+test_expect_success 'setup' '
+  test_commit A &&
+  test_commit B &&
+  test_commit C &&
+  git reset --hard A &&
+  test_commit D &&
+  test_commit CC &&
+  test_commit EX &&
+  test_merge FX C
+'
+
+test_output_expect_success "--bisect --first-parent" 'git rev-list --bisect --first-parent FX ^A' <<EOF
+$(git rev-parse CC)
+EOF
+
+test_output_expect_success "--first-parent" 'git rev-list --first-parent FX ^A' <<EOF
+$(git rev-parse FX)
+$(git rev-parse EX)
+$(git rev-parse CC)
+$(git rev-parse D)
+EOF
+
+test_output_expect_success "--bisect-vars --first-parent" 'git rev-list --bisect-vars --first-parent FX ^A' <<EOF
+bisect_rev='$(git rev-parse CC)'
+bisect_nr=1
+bisect_good=1
+bisect_bad=1
+bisect_all=4
+bisect_steps=1
+EOF
+
+test_expect_success "--bisect-all --first-parent" '
+cat >expect <<EOF &&
+$(git rev-parse CC) (dist=2)
+$(git rev-parse EX) (dist=1)
+$(git rev-parse D) (dist=1)
+$(git rev-parse FX) (dist=0)
+EOF
+
+# Make sure we have the same entries, nothing more, nothing less
+git rev-list --bisect-all --first-parent FX ^A >actual &&
+  sort actual >actual.sorted &&
+  sort expect >expect.sorted &&
+  test_cmp expect.sorted actual.sorted &&
+  # Make sure the entries are sorted in the dist order
+  sed -e "s/.*(dist=\([1-9]*[0-9]\)).*/\1/" actual >actual.dists &&
+  sort -r actual.dists >actual.dists.sorted &&
+  test_cmp actual.dists.sorted actual.dists
+'
+
 test_done
-- 
2.16.3

Re: [PATCH v6] Implement --first-parent for git rev-list --bisect

[Johannes Schindelin]

Hi Tiago,

On Tue, 28 Aug 2018, Tiago Botelho wrote:

> This will enable users to implement bisecting on first parents
> which can be useful for when the commits from a feature branch
> that we want to merge are not always tested.

This message is still lacking the explanation I asked for, namely for the
lines:

	@@ -329,6 +334,11 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
	 	if (0 <= weight(p))
	 		continue;
	 	for (q = p->item->parents; q; q = q->next) {
	+		if ((bisect_flags & BISECT_FIRST_PARENT)) {
	+			if (weight(q) < 0)
	+				q = NULL;
	+			break;
	+		}
	 	if (q->item->object.flags & UNINTERESTING)
	 		continue;
	 	if (0 <= weight(q))

> Signed-off-by: Tiago Botelho <tiagonbotelho@hotmail.com>
> ---
> 
> This patch refactors **only** the tests according to the suggestions made by Junio in v5
> more specifically `https://public-inbox.org/git/xmqqa7rhi40f.fsf@gitster-ct.c.googlers.com/`.
> 
> The discussion between Junio, Christian and Johannes got pretty confusing in the sense
> that I am unsure if this is the actual problem I am supposed to solve in order to get the
> patch accepted, if it is not I will keep iterating on it until it is good enough to be
> merged

I would have preferred to reuse the already existing commits generated in
the `setup` phase rather than generating yet another batch, and to not
introduce an inconsistent way to present a commit graph (compare your
diagram with the one in
https://github.com/git/git/blob/v2.18.0/t/t6002-rev-list-bisect.sh#L64-L90
i.e. *in the same file*), and I mentioned this at least twice before, but
maybe it got lost (and I could imagine that Christian's insistence on
pushing the patch through with as little changes as he can get away with
does not help), but at least now the added tests look slightly more
understandable than before (despite funny indentation and too-long lines),
so I'll stop imitating a broken record at this point.

Ciao,
Johannes

> 
>  bisect.c                   | 43 ++++++++++++++++++++++------------
>  bisect.h                   |  1 +
>  builtin/rev-list.c         |  3 +++
>  revision.c                 |  3 ---
>  t/t6002-rev-list-bisect.sh | 58 ++++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 90 insertions(+), 18 deletions(-)
> 
> diff --git a/bisect.c b/bisect.c
> index 4eafc8262..cb80f29c5 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -82,15 +82,16 @@ static inline void weight_set(struct commit_list *elem, int weight)
>  	*((int*)(elem->item->util)) = weight;
>  }
>  
> -static int count_interesting_parents(struct commit *commit)
> +static int count_interesting_parents(struct commit *commit, unsigned bisect_flags)
>  {
>  	struct commit_list *p;
>  	int count;
>  
>  	for (count = 0, p = commit->parents; p; p = p->next) {
> -		if (p->item->object.flags & UNINTERESTING)
> -			continue;
> -		count++;
> +		if (!(p->item->object.flags & UNINTERESTING))
> +		    count++;
> +		if (bisect_flags & BISECT_FIRST_PARENT)
> +			break;
>  	}
>  	return count;
>  }
> @@ -117,10 +118,10 @@ static inline int halfway(struct commit_list *p, int nr)
>  }
>  
>  #if !DEBUG_BISECT
> -#define show_list(a,b,c,d) do { ; } while (0)
> +#define show_list(a,b,c,d,e) do { ; } while (0)
>  #else
>  static void show_list(const char *debug, int counted, int nr,
> -		      struct commit_list *list)
> +		      struct commit_list *list, unsigned bisect_flags)
>  {
>  	struct commit_list *p;
>  
> @@ -146,10 +147,14 @@ static void show_list(const char *debug, int counted, int nr,
>  		else
>  			fprintf(stderr, "---");
>  		fprintf(stderr, " %.*s", 8, oid_to_hex(&commit->object.oid));
> -		for (pp = commit->parents; pp; pp = pp->next)
> +		for (pp = commit->parents; pp; pp = pp->next) {
>  			fprintf(stderr, " %.*s", 8,
>  				oid_to_hex(&pp->item->object.oid));
>  
> +			if (bisect_flags & BISECT_FIRST_PARENT)
> +				break;
> +		}
> +
>  		subject_len = find_commit_subject(buf, &subject_start);
>  		if (subject_len)
>  			fprintf(stderr, " %.*s", subject_len, subject_start);
> @@ -267,13 +272,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  		unsigned flags = commit->object.flags;
>  
>  		p->item->util = &weights[n++];
> -		switch (count_interesting_parents(commit)) {
> +		switch (count_interesting_parents(commit, bisect_flags)) {
>  		case 0:
>  			if (!(flags & TREESAME)) {
>  				weight_set(p, 1);
>  				counted++;
>  				show_list("bisection 2 count one",
> -					  counted, nr, list);
> +					  counted, nr, list, bisect_flags);
>  			}
>  			/*
>  			 * otherwise, it is known not to reach any
> @@ -289,7 +294,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  		}
>  	}
>  
> -	show_list("bisection 2 initialize", counted, nr, list);
> +	show_list("bisection 2 initialize", counted, nr, list, bisect_flags);
>  
>  	/*
>  	 * If you have only one parent in the resulting set
> @@ -319,7 +324,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  		counted++;
>  	}
>  
> -	show_list("bisection 2 count_distance", counted, nr, list);
> +	show_list("bisection 2 count_distance", counted, nr, list, bisect_flags);
>  
>  	while (counted < nr) {
>  		for (p = list; p; p = p->next) {
> @@ -329,6 +334,11 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  			if (0 <= weight(p))
>  				continue;
>  			for (q = p->item->parents; q; q = q->next) {
> +				if ((bisect_flags & BISECT_FIRST_PARENT)) {
> +					if (weight(q) < 0)
> +						q = NULL;
> +					break;
> +				}
>  				if (q->item->object.flags & UNINTERESTING)
>  					continue;
>  				if (0 <= weight(q))
> @@ -346,7 +356,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  				weight_set(p, weight(q)+1);
>  				counted++;
>  				show_list("bisection 2 count one",
> -					  counted, nr, list);
> +					  counted, nr, list, bisect_flags);
>  			}
>  			else
>  				weight_set(p, weight(q));
> @@ -357,7 +367,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  		}
>  	}
>  
> -	show_list("bisection 2 counted all", counted, nr, list);
> +	show_list("bisection 2 counted all", counted, nr, list, bisect_flags);
>  
>  	if (!find_all)
>  		return best_bisection(list, nr);
> @@ -372,7 +382,7 @@ void find_bisection(struct commit_list **commit_list, int *reaches,
>  	struct commit_list *list, *p, *best, *next, *last;
>  	int *weights;
>  
> -	show_list("bisection 2 entry", 0, 0, *commit_list);
> +	show_list("bisection 2 entry", 0, 0, *commit_list, bisect_flags);
>  
>  	/*
>  	 * Count the number of total and tree-changing items on the
> @@ -395,7 +405,7 @@ void find_bisection(struct commit_list **commit_list, int *reaches,
>  		on_list++;
>  	}
>  	list = last;
> -	show_list("bisection 2 sorted", 0, nr, list);
> +	show_list("bisection 2 sorted", 0, nr, list, bisect_flags);
>  
>  	*all = nr;
>  	weights = xcalloc(on_list, sizeof(*weights));
> @@ -962,6 +972,9 @@ int bisect_next_all(const char *prefix, int no_checkout)
>  	if (skipped_revs.nr)
>  		bisect_flags |= BISECT_FIND_ALL;
>  
> +	if (revs.first_parent_only)
> +		bisect_flags |= BISECT_FIRST_PARENT;
> +
>  	find_bisection(&revs.commits, &reaches, &all, bisect_flags);
>  	revs.commits = managed_skipped(revs.commits, &tried);
>  
> diff --git a/bisect.h b/bisect.h
> index 1d40a33ad..e445567c2 100644
> --- a/bisect.h
> +++ b/bisect.h
> @@ -2,6 +2,7 @@
>  #define BISECT_H
>  
>  #define BISECT_FIND_ALL		(1u<<0)
> +#define BISECT_FIRST_PARENT    	(1u<<1)
>  
>  /*
>   * Find bisection. If something is found, `reaches` will be the number of
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index 8752f5bbe..66439e1b3 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -538,6 +538,9 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>  	if (bisect_list) {
>  		int reaches, all;
>  
> +		if (revs.first_parent_only)
> +			bisect_flags |= BISECT_FIRST_PARENT;
> +
>  		find_bisection(&revs.commits, &reaches, &all, bisect_flags);
>  
>  		if (bisect_show_vars)
> diff --git a/revision.c b/revision.c
> index 4e0e193e5..b9d063805 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2474,9 +2474,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
>  	if (!revs->reflog_info && revs->grep_filter.use_reflog_filter)
>  		die("cannot use --grep-reflog without --walk-reflogs");
>  
> -	if (revs->first_parent_only && revs->bisect)
> -		die(_("--first-parent is incompatible with --bisect"));
> -
>  	if (revs->expand_tabs_in_log < 0)
>  		revs->expand_tabs_in_log = revs->expand_tabs_in_log_default;
>  
> diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
> index a66140803..1bc297de5 100755
> --- a/t/t6002-rev-list-bisect.sh
> +++ b/t/t6002-rev-list-bisect.sh
> @@ -263,4 +263,62 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' '
>  	test_cmp expect.sorted actual.sorted
>  '
>  
> +# We generate the following commit graph:
> +#
> +#   B ------ C
> +#  /          \
> +# A            FX
> +#  \          /
> +#   D - CC - EX
> +
> +test_expect_success 'setup' '
> +  test_commit A &&
> +  test_commit B &&
> +  test_commit C &&
> +  git reset --hard A &&
> +  test_commit D &&
> +  test_commit CC &&
> +  test_commit EX &&
> +  test_merge FX C
> +'
> +
> +test_output_expect_success "--bisect --first-parent" 'git rev-list --bisect --first-parent FX ^A' <<EOF
> +$(git rev-parse CC)
> +EOF
> +
> +test_output_expect_success "--first-parent" 'git rev-list --first-parent FX ^A' <<EOF
> +$(git rev-parse FX)
> +$(git rev-parse EX)
> +$(git rev-parse CC)
> +$(git rev-parse D)
> +EOF
> +
> +test_output_expect_success "--bisect-vars --first-parent" 'git rev-list --bisect-vars --first-parent FX ^A' <<EOF
> +bisect_rev='$(git rev-parse CC)'
> +bisect_nr=1
> +bisect_good=1
> +bisect_bad=1
> +bisect_all=4
> +bisect_steps=1
> +EOF
> +
> +test_expect_success "--bisect-all --first-parent" '
> +cat >expect <<EOF &&
> +$(git rev-parse CC) (dist=2)
> +$(git rev-parse EX) (dist=1)
> +$(git rev-parse D) (dist=1)
> +$(git rev-parse FX) (dist=0)
> +EOF
> +
> +# Make sure we have the same entries, nothing more, nothing less
> +git rev-list --bisect-all --first-parent FX ^A >actual &&
> +  sort actual >actual.sorted &&
> +  sort expect >expect.sorted &&
> +  test_cmp expect.sorted actual.sorted &&
> +  # Make sure the entries are sorted in the dist order
> +  sed -e "s/.*(dist=\([1-9]*[0-9]\)).*/\1/" actual >actual.dists &&
> +  sort -r actual.dists >actual.dists.sorted &&
> +  test_cmp actual.dists.sorted actual.dists
> +'
> +
>  test_done
> -- 
> 2.16.3
> 
> 

Re: [PATCH v6] Implement --first-parent for git rev-list --bisect

[Junio C Hamano]

Tiago Botelho <tiagonbotelho@gmail.com> writes:

> diff --git a/bisect.c b/bisect.c
> index 4eafc8262..cb80f29c5 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -82,15 +82,16 @@ static inline void weight_set(struct commit_list *elem, int weight)
>  	*((int*)(elem->item->util)) = weight;
>  }
>  
> -static int count_interesting_parents(struct commit *commit)
> +static int count_interesting_parents(struct commit *commit, unsigned bisect_flags)
>  {
>  	struct commit_list *p;
>  	int count;
>  
>  	for (count = 0, p = commit->parents; p; p = p->next) {
> -		if (p->item->object.flags & UNINTERESTING)
> -			continue;
> -		count++;
> +		if (!(p->item->object.flags & UNINTERESTING))
> +		    count++;
> +		if (bisect_flags & BISECT_FIRST_PARENT)
> +			break;
>  	}
>  	return count;
>  }

We either say 0 ("the first parent is uninteresting") or 1 ("it is")
under BISECT_FIRST_PARENT mode.  OK.

> @@ -117,10 +118,10 @@ static inline int halfway(struct commit_list *p, int nr)
>  }
>  
>  #if !DEBUG_BISECT
> -#define show_list(a,b,c,d) do { ; } while (0)
> +#define show_list(a,b,c,d,e) do { ; } while (0)
>  #else
>  static void show_list(const char *debug, int counted, int nr,
> -		      struct commit_list *list)
> +		      struct commit_list *list, unsigned bisect_flags)
>  {
>  	struct commit_list *p;
>  
> @@ -146,10 +147,14 @@ static void show_list(const char *debug, int counted, int nr,

An unrelated tangent, but I think I just spotted a bug in the
existing code on the line immediately before this hunk, which reads

		if (commit->util)
			fprintf(stderr, "%3d", weight(p));

I think this was a bug introduced at bb408ac9 ("bisect.c: use
commit-slab for commit weight instead of commit->util", 2018-05-19)
where the internal implementation of weight() was changed not to
touch commit->util but instead to use a separate commit-slab storage

Looking at the code before that conversion, it seems that we were
using ->util to store a pointer to an integer, so we had the ability
to differenciate non-negative weight (i.e. weight already computed
for the commit), negative weight (i.e. not computed yet, but will
be), and commits to which the concept of weight is not applicable.
When we went to the commit-slab with the change, we lost the ability
to represent the third case.  I am offhand not sure what the best
remedy would be.  Perhaps stuff a so-far unused value like -3 to the
weight() and use weight(p) == -3 instead of the old !commit->util or
something like that?

(Duy CC'ed to help checking my sanity on this point).

In any case, this is an existing bug in a debug helper, and the
focus of this patch is not about fixing that bug, so you can and
should leave it as-is, until this patch successfully adds the
"bisection following only the first parent" feature.

>  		else
>  			fprintf(stderr, "---");
>  		fprintf(stderr, " %.*s", 8, oid_to_hex(&commit->object.oid));
> -		for (pp = commit->parents; pp; pp = pp->next)
> +		for (pp = commit->parents; pp; pp = pp->next) {
>  			fprintf(stderr, " %.*s", 8,
>  				oid_to_hex(&pp->item->object.oid));
>  
> +			if (bisect_flags & BISECT_FIRST_PARENT)
> +				break;
> +		}
> +

I am not sure if we want to do this.  This is a debugging aid that
allows us to peek into the internal state of the commit history
graph being worked on; don't we rather want to make sure that tips
of merged side branches are not participating (e.g. commit->util
that records "weight" is used differently between the first and
other parent commits)?

>  		subject_len = find_commit_subject(buf, &subject_start);
>  		if (subject_len)
>  			fprintf(stderr, " %.*s", subject_len, subject_start);
> @@ -267,13 +272,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  		unsigned flags = commit->object.flags;
>  
>  		p->item->util = &weights[n++];
> -		switch (count_interesting_parents(commit)) {
> +		switch (count_interesting_parents(commit, bisect_flags)) {
>  		case 0:
>  			if (!(flags & TREESAME)) {
>  				weight_set(p, 1);
>  				counted++;
>  				show_list("bisection 2 count one",
> -					  counted, nr, list);
> +					  counted, nr, list, bisect_flags);
>  			}
>  			/*
>  			 * otherwise, it is known not to reach any

Because count_interesting_parents() returns either 0 or 1 under
BISECT_FIRST_PARENT mode, we will never set -2 in this switch()
statement.  We either paint the bottom boundary with the correct
weight, or -1 (i.e. "its weight is one plus its first parent's
weight" marker).  OK.

> @@ -289,7 +294,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  		}
>  	}
>  
> -	show_list("bisection 2 initialize", counted, nr, list);
> +	show_list("bisection 2 initialize", counted, nr, list, bisect_flags);
>  
>  	/*
>  	 * If you have only one parent in the resulting set
> @@ -319,7 +324,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  		counted++;
>  	}
>  
> -	show_list("bisection 2 count_distance", counted, nr, list);
> +	show_list("bisection 2 count_distance", counted, nr, list, bisect_flags);

The loop before this part (not shown) is about counting weight for
merges the hard way, which does not apply to us under the new
BISECT_FIRST_PARENT mode.  IOW, "initialize" and "count_distance"
should show exactly the same output from show_list().

>  	while (counted < nr) {
>  		for (p = list; p; p = p->next) {
> @@ -329,6 +334,11 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  			if (0 <= weight(p))
>  				continue;

If we know the weight of 'p' already, then we are already done with
it.  In the normal code, that means we are done with all the merges,
so the loop immediately below deals only with a single strand of
pearls.  However, under BISECT_FIRST_PARENT mode, that is
different.  Let's see.

>  			for (q = p->item->parents; q; q = q->next) {
> +				if ((bisect_flags & BISECT_FIRST_PARENT)) {
> +					if (weight(q) < 0)
> +						q = NULL;
> +					break;
> +				}

Under first-parent mode, we uncondtionally break out of the loop
after inspecting the first parent, even if it is a merge.  The code
after this loop (outside the context) says

			if (!q)
				continue;

which is to say "we are not ready to set the weight of p based on
the weight of q, because the latter is unknown".  So that is done by
setting q to NULL, which makes sort of sense.

>  				if (q->item->object.flags & UNINTERESTING)
>  					continue;
> ...
>  				if (0 <= weight(q))

Post context of this hunk is "break;", meaning that if we find an
interesting parent 

What the above original tells us is that when we are doing a full
bisection, we do not look at weight of uninteresting parent and
continue.  In that case we know p is not a merge so we will exit
this for() loop with q set to NULL.

Shouldn't we keep that property even under BISECT_FIRST_PARENT mode?
What I am wondering is the block added to the top of this for() loop.

Shouldn't it be more like this?

                                if ((bisect_flags & BISECT_FIRST_PARENT)) {
					if ((q->item->object.flags & UNINTERESTING) ||
					    (weight(q) < 0))
						q = NULL;
					break;
				}

I do not even recall if weight(q) is defined for an uninteresting q
offhand; your patch requires weight(q) is defined and is negative
for such a 'q' to be correct.  Code would be more trivially correct
with an explicit check for UNINTERESTING, I would think.

In any case, after finding a parent (or not finding, signalled by
being NULL) whose weight is either the same or one less than our
weight, we leave the above for() loop and then ...

> @@ -346,7 +356,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  				weight_set(p, weight(q)+1);
>  				counted++;
>  				show_list("bisection 2 count one",
> -					  counted, nr, list);
> +					  counted, nr, list, bisect_flags);
>  			}
>  			else
>  				weight_set(p, weight(q));

... we set our weight based on its weight.  OK.

The remainder of the change looked OK (I didn't look at the the test).

Thanks.

Re: [PATCH v6] Implement --first-parent for git rev-list --bisect

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi Tiago,
>
> On Tue, 28 Aug 2018, Tiago Botelho wrote:
>
>> This will enable users to implement bisecting on first parents
>> which can be useful for when the commits from a feature branch
>> that we want to merge are not always tested.
>
> This message is still lacking the explanation I asked for, namely for the
> lines:
>
> 	@@ -329,6 +334,11 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
> 	 	if (0 <= weight(p))
> 	 		continue;
> 	 	for (q = p->item->parents; q; q = q->next) {
> 	+		if ((bisect_flags & BISECT_FIRST_PARENT)) {
> 	+			if (weight(q) < 0)
> 	+				q = NULL;
> 	+			break;
> 	+		}
> 	 	if (q->item->object.flags & UNINTERESTING)
> 	 		continue;
> 	 	if (0 <= weight(q))

I've just finished scanning the discussion thread on public-inbox
for v5, v4, v3, v2 and the initial round of this series, but found
your comments only on the tests.  If you have a pointer that would
be great; it also is OK to say what kind of explanation is needed
for that addition again.  

FWIW I too was puzzled about the correctness of the added logic
above, especially the part that reads weight(q) before checking if
it is not UNINTERESTING, but I covered it on a separate message.

> I would have preferred to reuse the already existing commits generated in
> the `setup` phase rather than generating yet another batch, and to not
> introduce an inconsistent way to present a commit graph (compare your
> diagram with the one in
> https://github.com/git/git/blob/v2.18.0/t/t6002-rev-list-bisect.sh#L64-L90
> i.e. *in the same file*)

As I already said in the previous round, I do agree with these.
That is, ...

>> diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
>> index a66140803..1bc297de5 100755
>> --- a/t/t6002-rev-list-bisect.sh
>> +++ b/t/t6002-rev-list-bisect.sh
>> @@ -263,4 +263,62 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' '
>>  	test_cmp expect.sorted actual.sorted
>>  '
>>  
>> +# We generate the following commit graph:
>> +#
>> +#   B ------ C
>> +#  /          \
>> +# A            FX
>> +#  \          /
>> +#   D - CC - EX
>> +
>> +test_expect_success 'setup' '
>> +  test_commit A &&
>> +  test_commit B &&
>> +  test_commit C &&
>> +  git reset --hard A &&
>> +  test_commit D &&
>> +  test_commit CC &&
>> +  test_commit EX &&
>> +  test_merge FX C
>> +'

... the above graph construction should not be necessary.  An
earlier part of t6002 would have already created a history of
suitable shape to use for writing the following tests.

>> +test_output_expect_success "--bisect --first-parent" 'git rev-list --bisect --first-parent FX ^A' <<EOF
>> +$(git rev-parse CC)
>> +EOF
>> +
>> +test_output_expect_success "--first-parent" 'git rev-list --first-parent FX ^A' <<EOF
>> +$(git rev-parse FX)
>> +$(git rev-parse EX)
>> +$(git rev-parse CC)
>> +$(git rev-parse D)
>> +EOF
>> +
>> +test_output_expect_success "--bisect-vars --first-parent" 'git rev-list --bisect-vars --first-parent FX ^A' <<EOF
>> +bisect_rev='$(git rev-parse CC)'
>> +bisect_nr=1
>> +bisect_good=1
>> +bisect_bad=1
>> +bisect_all=4
>> +bisect_steps=1
>> +EOF
>> +
>> +test_expect_success "--bisect-all --first-parent" '
>> +cat >expect <<EOF &&
>> +$(git rev-parse CC) (dist=2)
>> +$(git rev-parse EX) (dist=1)
>> +$(git rev-parse D) (dist=1)
>> +$(git rev-parse FX) (dist=0)
>> +EOF
>> +
>> +# Make sure we have the same entries, nothing more, nothing less
>> +git rev-list --bisect-all --first-parent FX ^A >actual &&
>> +  sort actual >actual.sorted &&
>> +  sort expect >expect.sorted &&
>> +  test_cmp expect.sorted actual.sorted &&
>> +  # Make sure the entries are sorted in the dist order
>> +  sed -e "s/.*(dist=\([1-9]*[0-9]\)).*/\1/" actual >actual.dists &&
>> +  sort -r actual.dists >actual.dists.sorted &&
>> +  test_cmp actual.dists.sorted actual.dists
>> +'
>> +
>>  test_done
>> -- 
>> 2.16.3
>> 
>> 

Re: [PATCH v6] Implement --first-parent for git rev-list --bisect

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> I would have preferred to reuse the already existing commits generated in
>> the `setup` phase rather than generating yet another batch, and to not
>> introduce an inconsistent way to present a commit graph (compare your
>> diagram with the one in
>> https://github.com/git/git/blob/v2.18.0/t/t6002-rev-list-bisect.sh#L64-L90
>> i.e. *in the same file*)
>
> As I already said in the previous round, I do agree with these.
> That is, ...
>
>>>  
>>> +# We generate the following commit graph:
>>> +#
>>> +#   B ------ C
> ... the above graph construction should not be necessary.  An
> earlier part of t6002 would have already created a history of
> suitable shape to use for writing the following tests.

Something like the following, perhaps?  I have a feeling that use of
test-output-expect-success is trying to be too cute without making
the result easier to read, and also makes it harder to debug the
test script when it does not work as expected (e.g. you need to see
where the output from the actual command is stored by going to the
definition of the shell function), and would have preferred to see
these three tests written without it (and instead using explicit
'expect' vs 'actual' comparison), but at least the patch below shows
what I meant when I suggested updating the tests to reuse the
existing history (I do not speak for Johannes, but I am guessing we
are on the same page on this one).

Thanks.

 t/t6002-rev-list-bisect.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
index a661408038..d27d0087d6 100755
--- a/t/t6002-rev-list-bisect.sh
+++ b/t/t6002-rev-list-bisect.sh
@@ -263,4 +263,50 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' '
 	test_cmp expect.sorted actual.sorted
 '
 
+# See the drawing near the top --- e4 is in the middle of the first parent chain
+printf "%s\n" e4 |
+test_output_expect_success '--bisect --first-parent' '
+	git rev-list --bisect --first-parent E ^F
+'
+
+printf "%s\n" E e1 e2 e3 e4 e5 e6 e7 e8 |
+test_output_expect_success "--first-parent" '
+	git rev-list --first-parent E ^F
+'
+
+test_output_expect_success '--bisect-vars --first-parent' '
+	git rev-list --bisect-vars --first-parent E ^F
+' <<-EOF
+	bisect_rev='e5'
+	bisect_nr=4
+	bisect_good=4
+	bisect_bad=3
+	bisect_all=9
+	bisect_steps=2
+EOF
+
+test_expect_success '--bisect-all --first-parent' '
+	cat >expect <<-EOF &&
+	$(git rev-parse tags/e5) (dist=4)
+	$(git rev-parse tags/e4) (dist=4)
+	$(git rev-parse tags/e6) (dist=3)
+	$(git rev-parse tags/e3) (dist=3)
+	$(git rev-parse tags/e7) (dist=2)
+	$(git rev-parse tags/e2) (dist=2)
+	$(git rev-parse tags/e8) (dist=1)
+	$(git rev-parse tags/e1) (dist=1)
+	$(git rev-parse tags/E) (dist=0)
+	EOF
+
+	# Make sure we have the same entries, nothing more, nothing less
+	git rev-list --bisect-all --first-parent E ^F >actual &&
+	sort actual >actual.sorted &&
+	sort expect >expect.sorted &&
+	test_cmp expect.sorted actual.sorted &&
+	# Make sure the entries are sorted in the dist order
+	sed -e "s/.*(dist=\([1-9]*[0-9]\)).*/\1/" actual >actual.dists &&
+	sort -r -n actual.dists >actual.dists.sorted &&
+	test_cmp actual.dists.sorted actual.dists
+'
+
 test_done

Re: [PATCH v6] Implement --first-parent for git rev-list --bisect

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Something like the following, perhaps?

Having said all that.

> +# See the drawing near the top --- e4 is in the middle of the first parent chain
> +printf "%s\n" e4 |
> +test_output_expect_success '--bisect --first-parent' '
> +	git rev-list --bisect --first-parent E ^F
> +'
> +
> +printf "%s\n" E e1 e2 e3 e4 e5 e6 e7 e8 |
> +test_output_expect_success "--first-parent" '
> +	git rev-list --first-parent E ^F
> +'

The above two are probably not controversial.

> +test_output_expect_success '--bisect-vars --first-parent' '
> +	git rev-list --bisect-vars --first-parent E ^F
> +' <<-EOF
> +	bisect_rev='e5'
> +	bisect_nr=4
> +	bisect_good=4
> +	bisect_bad=3
> +	bisect_all=9
> +	bisect_steps=2
> +EOF

Perhaps this one isn't either.


> +test_expect_success '--bisect-all --first-parent' '
> +	cat >expect <<-EOF &&
> +	$(git rev-parse tags/e5) (dist=4)
> +	$(git rev-parse tags/e4) (dist=4)
> +	$(git rev-parse tags/e6) (dist=3)
> +	$(git rev-parse tags/e3) (dist=3)
> +	$(git rev-parse tags/e7) (dist=2)
> +	$(git rev-parse tags/e2) (dist=2)
> +	$(git rev-parse tags/e8) (dist=1)
> +	$(git rev-parse tags/e1) (dist=1)
> +	$(git rev-parse tags/E) (dist=0)
> +	EOF
> +
> +	# Make sure we have the same entries, nothing more, nothing less
> +	git rev-list --bisect-all --first-parent E ^F >actual &&
> +	sort actual >actual.sorted &&
> +	sort expect >expect.sorted &&
> +	test_cmp expect.sorted actual.sorted &&

By sorting both, we attempt allow them to come out in any random
order when sorting done by --bisect-all gets tiebroken differently
between commits with the same dist value.  As Johannes said, this is
a bit too strict to insist that E *must* get dist 0, when the only
thing we may care about are e2 and e7 get the same dist value, which
is larger than the dist value given to E, etc.  If we wanted to ease
the over-strictness, we could omit " (dist=N)" from the 'expect' file,
and then

	sed -e 's/ (dist=[0-9]*)$//' actual | sort >actual.sorted &&
	sort expect >expect.sorted &&
	test_cmp expect.sorted actual.sorted &&

to compare only the object names.

This over-strictness would have caught a bug in Git if we reverted
this new feature (i.e. "rev-list --first-parent --bisect-all" does
not refuse to work---it just does not do what we expect), which
gives distance of -1 in the output (!).  From that point of view, I
think it is also OK for the test to be spelling the values out like
the above.

> +	# Make sure the entries are sorted in the dist order
> +	sed -e "s/.*(dist=\([0-9]*\)).*/\1/" actual >actual.dists &&
> +	sort -r -n actual.dists >actual.dists.sorted &&

I forgot to mention but I added "-n" here to make sure we
numerically sort the list of distance values.  Also you had a bogus
regexp to catch digits inherited from "something like this" I showed
in an older discussion (I think it is sufficient to say [0-9]* here).

The above makes sure that commits that have larger distance number
are listed earlier in the output, and we do not care if which one of
e4 or e5, both of which have distant number 4, is shown first---we'd
be happy as long as all the ones at distance 4 are shown before the
others at smaller distance number.  So with this in place, I think
we can omit the exact distance number from the earlier part of this
test, but as I said, it would have caught the bug that produces a
negative distance value in the output, so I am still on the fence,
leaning a bit toward being stricter than necessary.

> +	test_cmp actual.dists.sorted actual.dists
> +'
> +
>  test_done

Re: [PATCH v6] Implement --first-parent for git rev-list --bisect

[Duy Nguyen]

On Tue, Aug 28, 2018 at 6:45 PM Junio C Hamano <gitster@pobox.com> wrote:
> > @@ -146,10 +147,14 @@ static void show_list(const char *debug, int counted, int nr,
>
> An unrelated tangent, but I think I just spotted a bug in the
> existing code on the line immediately before this hunk, which reads
>
>                 if (commit->util)
>                         fprintf(stderr, "%3d", weight(p));
>
> I think this was a bug introduced at bb408ac9 ("bisect.c: use
> commit-slab for commit weight instead of commit->util", 2018-05-19)
> where the internal implementation of weight() was changed not to
> touch commit->util but instead to use a separate commit-slab storage
>
> Looking at the code before that conversion, it seems that we were
> using ->util to store a pointer to an integer, so we had the ability
> to differenciate non-negative weight (i.e. weight already computed
> for the commit), negative weight (i.e. not computed yet, but will
> be), and commits to which the concept of weight is not applicable.
> When we went to the commit-slab with the change, we lost the ability
> to represent the third case.  I am offhand not sure what the best
> remedy would be.  Perhaps stuff a so-far unused value like -3 to the
> weight() and use weight(p) == -3 instead of the old !commit->util or
> something like that?

Hmm.. no? the commit-slab stores the pointer to the weight, not the
weight itself, so we still have the ability to check the third case, I
think.

> (Duy CC'ed to help checking my sanity on this point).
>
> In any case, this is an existing bug in a debug helper, and the
> focus of this patch is not about fixing that bug, so you can and
> should leave it as-is, until this patch successfully adds the
> "bisection following only the first parent" feature.

Yes. I'll post a patch soon to fix this "commit->util" leftover.
-- 
Duy

[PATCH] bisect.c: make show_list() build again

[Nguyễn Thái Ngọc Duy]

This function only compiles when DEBUG_BISECT is 1, which is often not
the case. As a result there are two commits [1] [2] that break it but
the breakages went unnoticed because the code did not compile by
default. Update the function and include the new header file to make this
function build again.

In order to stop this from happening again, the function is now
compiled unconditionally but exits early unless DEBUG_BISECT is
non-zero. A smart compiler generates no extra code (not even a
function call). But even if it does not, this function does not seem
to be in a hot path that the extra cost becomes a big problem.

[1] bb408ac95d (bisect.c: use commit-slab for commit weight instead of
    commit->util - 2018-05-19)

[2] cbd53a2193 (object-store: move object access functions to
    object-store.h - 2018-05-15)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 bisect.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/bisect.c b/bisect.c
index e1275ba79e..e65f6821b8 100644
--- a/bisect.c
+++ b/bisect.c
@@ -13,6 +13,7 @@
 #include "sha1-array.h"
 #include "argv-array.h"
 #include "commit-slab.h"
+#include "object-store.h"
 
 static struct oid_array good_revs;
 static struct oid_array skipped_revs;
@@ -120,14 +121,14 @@ static inline int halfway(struct commit_list *p, int nr)
 	}
 }
 
-#if !DEBUG_BISECT
-#define show_list(a,b,c,d) do { ; } while (0)
-#else
 static void show_list(const char *debug, int counted, int nr,
 		      struct commit_list *list)
 {
 	struct commit_list *p;
 
+	if (!DEBUG_BISECT)
+		return;
+
 	fprintf(stderr, "%s (%d/%d)\n", debug, counted, nr);
 
 	for (p = list; p; p = p->next) {
@@ -145,7 +146,7 @@ static void show_list(const char *debug, int counted, int nr,
 			(flags & TREESAME) ? ' ' : 'T',
 			(flags & UNINTERESTING) ? 'U' : ' ',
 			(flags & COUNTED) ? 'C' : ' ');
-		if (commit->util)
+		if (*commit_weight_at(&commit_weight, p->item))
 			fprintf(stderr, "%3d", weight(p));
 		else
 			fprintf(stderr, "---");
@@ -160,7 +161,6 @@ static void show_list(const char *debug, int counted, int nr,
 		fprintf(stderr, "\n");
 	}
 }
-#endif /* DEBUG_BISECT */
 
 static struct commit_list *best_bisection(struct commit_list *list, int nr)
 {
-- 
2.19.0.rc0.337.ge906d732e7

Re: [PATCH] bisect.c: make show_list() build again

[Christian Couder]

On Sun, Sep 2, 2018 at 9:42 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:

> In order to stop this from happening again, the function is now
> compiled unconditionally but exits early unless DEBUG_BISECT is
> non-zero.

Thanks for going the extra mile and doing this!

I wonder if we should also try to make the show_list() function part
of the trace_*() functions to make it even more regular. This can be a
separate patch or topic though.

Re: [PATCH] bisect.c: make show_list() build again

[Duy Nguyen]

On Sun, Sep 2, 2018 at 9:57 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> On Sun, Sep 2, 2018 at 9:42 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>
> > In order to stop this from happening again, the function is now
> > compiled unconditionally but exits early unless DEBUG_BISECT is
> > non-zero.
>
> Thanks for going the extra mile and doing this!
>
> I wonder if we should also try to make the show_list() function part
> of the trace_*() functions to make it even more regular. This can be a
> separate patch or topic though.

Yeah that's probably a good idea (though I'm not familiar with
bisect.c enough to take that step).
-- 
Duy

Re: [PATCH] bisect.c: make show_list() build again

[Christian Couder]

On Mon, Sep 3, 2018 at 7:31 PM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sun, Sep 2, 2018 at 9:57 AM Christian Couder
> <christian.couder@gmail.com> wrote:
>>
>> Thanks for going the extra mile and doing this!
>>
>> I wonder if we should also try to make the show_list() function part
>> of the trace_*() functions to make it even more regular. This can be a
>> separate patch or topic though.
>
> Yeah that's probably a good idea (though I'm not familiar with
> bisect.c enough to take that step).

Maybe this could be a GSoC micro project or a left over bit.

Re: [PATCH v6] Implement --first-parent for git rev-list --bisect

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> Hmm.. no? the commit-slab stores the pointer to the weight, not the
> weight itself, so we still have the ability to check the third case, I
> think.

Good, thanks.