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

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