[PATCH] Create '--merges-only' option for 'git bisect'

[PATCH] Create '--merges-only' option for 'git bisect'

[Harald Nordgren]

When ran with '--merges-only', git bisect will only look at merge commits -- commits with 2 or more parents or the initial commit.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---

Notes:
    Proof of concept of a feature that I have wanted in Git for a while. In my daily work my company uses GitHub, which creates lots of merge commits. In general, tests are only ran on the tip of a branch before merging, so the different commits within a merge commit are allowed not to be buildable. Therefore 'git bisect' often doesn't work since it will give lots of false positives for anything that is not a merge commit. If we could have a feature to only bisect merge commits then it would be easier to pinpoint which merge causes any particular issue. After that, a bisect could be done within the merge to pinpoint futher. As a follow-up to this patch -- we could potentially create a feature that automatically continues into regular bisect within the bad merge commit after completed '--merges-only' bisection.

 bisect.c                        |  87 ++++++++++++++++++++++++++----
 bisect.h                        |   4 +-
 builtin/bisect--helper.c        |  28 +++++++++-
 builtin/rev-list.c              |   2 +-
 git-bisect.sh                   |   5 ++
 t/t6070-bisect-merge-commits.sh | 116 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 228 insertions(+), 14 deletions(-)
 create mode 100755 t/t6070-bisect-merge-commits.sh

diff --git a/bisect.c b/bisect.c
index a579b5088..e8a2f77c7 100644
--- a/bisect.c
+++ b/bisect.c
@@ -254,7 +254,7 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n
  */
 static struct commit_list *do_find_bisection(struct commit_list *list,
 					     int nr, int *weights,
-					     int find_all)
+					     int find_all, int only_merge_commits)
 {
 	int n, counted;
 	struct commit_list *p;
@@ -266,6 +266,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 		unsigned flags = commit->object.flags;
 
 		p->item->util = &weights[n++];
+
+		if (only_merge_commits) {
+			weight_set(p, -2);
+			counted++;
+			continue;
+		}
+
 		switch (count_interesting_parents(commit)) {
 		case 0:
 			if (!(flags & TREESAME)) {
@@ -305,11 +312,17 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 	 * way, and then fill the blanks using cheaper algorithm.
 	 */
 	for (p = list; p; p = p->next) {
+		int distance;
 		if (p->item->object.flags & UNINTERESTING)
 			continue;
 		if (weight(p) != -2)
 			continue;
-		weight_set(p, count_distance(p));
+
+		if (only_merge_commits)
+			distance = count_distance(p) - 1;
+		else
+			distance = count_distance(p);
+		weight_set(p, distance);
 		clear_distance(list);
 
 		/* Does it happen to be at exactly half-way? */
@@ -330,11 +343,17 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 			for (q = p->item->parents; q; q = q->next) {
 				if (q->item->object.flags & UNINTERESTING)
 					continue;
+				if (!q->item->util)
+					break;
 				if (0 <= weight(q))
 					break;
 			}
 			if (!q)
 				continue;
+			if (!q->item->util) {
+				counted++;
+				continue;
+			}
 
 			/*
 			 * weight for p is unknown but q is known.
@@ -364,8 +383,43 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 		return best_bisection_sorted(list, nr);
 }
 
+int merge_commit_or_root(const struct commit c)
+{
+	if (!c.parents)
+		return 1;
+
+	return !!c.parents->next;
+}
+
+void filter_non_merge_commits(struct commit_list **commit_list)
+{
+	struct commit_list *list1 = *commit_list;
+	struct commit_list *list2 = NULL;
+	*commit_list = NULL;
+
+	for ( ; list1; list1 = list1->next) {
+		if (merge_commit_or_root(*list1->item)) {
+			list2 = list1;
+			list1 = list1->next;
+			list2->next = NULL;
+			list2->item->parents = NULL;
+			*commit_list = list2;
+			break;
+		}
+	}
+
+	for ( ; list1; list1 = list1->next) {
+		list2->next = NULL;
+		if (merge_commit_or_root(*list1->item)) {
+			list2->next = list1;
+			list2 = list2->next;
+			list2->item->parents = list1;
+		}
+	}
+}
+
 void find_bisection(struct commit_list **commit_list, int *reaches,
-		    int *all, int find_all)
+		    int *all, int find_all, int only_merge_commits)
 {
 	int nr, on_list;
 	struct commit_list *list, *p, *best, *next, *last;
@@ -373,6 +427,10 @@ void find_bisection(struct commit_list **commit_list, int *reaches,
 
 	show_list("bisection 2 entry", 0, 0, *commit_list);
 
+	if (only_merge_commits) {
+		filter_non_merge_commits(commit_list);
+	}
+
 	/*
 	 * Count the number of total and tree-changing items on the
 	 * list, while reversing the list.
@@ -400,7 +458,7 @@ void find_bisection(struct commit_list **commit_list, int *reaches,
 	weights = xcalloc(on_list, sizeof(*weights));
 
 	/* Do the real work of finding bisection commit. */
-	best = do_find_bisection(list, nr, weights, find_all);
+	best = do_find_bisection(list, nr, weights, find_all, only_merge_commits);
 	if (best) {
 		if (!find_all) {
 			list->item = best->item;
@@ -878,7 +936,7 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
 /*
  * This does "git diff-tree --pretty COMMIT" without one fork+exec.
  */
-static void show_diff_tree(const char *prefix, struct commit *commit)
+static void show_diff_tree(const char *prefix, struct commit *commit, int only_merge_commits)
 {
 	struct rev_info opt;
 
@@ -893,6 +951,11 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
 	opt.use_terminator = 0;
 	opt.commit_format = CMIT_FMT_DEFAULT;
 
+	if (only_merge_commits) {
+		opt.ignore_merges = 0;
+		opt.combine_merges = 1;
+	}
+
 	/* diff-tree init */
 	if (!opt.diffopt.output_format)
 		opt.diffopt.output_format = DIFF_FORMAT_RAW;
@@ -938,7 +1001,7 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
  * If no_checkout is non-zero, the bisection process does not
  * checkout the trial commit but instead simply updates BISECT_HEAD.
  */
-int bisect_next_all(const char *prefix, int no_checkout)
+int bisect_next_all(const char *prefix, int no_checkout, int only_merge_commits)
 {
 	struct rev_info revs;
 	struct commit_list *tried;
@@ -957,7 +1020,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
 
 	bisect_common(&revs);
 
-	find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr);
+	find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr, only_merge_commits);
 	revs.commits = managed_skipped(revs.commits, &tried);
 
 	if (!revs.commits) {
@@ -983,10 +1046,14 @@ int bisect_next_all(const char *prefix, int no_checkout)
 	bisect_rev = &revs.commits->item->object.oid;
 
 	if (!oidcmp(bisect_rev, current_bad_oid)) {
+		char *format_string = NULL;
 		exit_if_skipped_commits(tried, current_bad_oid);
-		printf("%s is the first %s commit\n", oid_to_hex(bisect_rev),
-			term_bad);
-		show_diff_tree(prefix, revs.commits->item);
+		if (only_merge_commits)
+			format_string = "%s is the first %s merge\n";
+		else
+			format_string = "%s is the first %s commit\n";
+		printf(format_string, oid_to_hex(bisect_rev), term_bad);
+		show_diff_tree(prefix, revs.commits->item, only_merge_commits);
 		/* This means the bisection process succeeded. */
 		exit(10);
 	}
diff --git a/bisect.h b/bisect.h
index a5d9248a4..664ada180 100644
--- a/bisect.h
+++ b/bisect.h
@@ -9,7 +9,7 @@
  * best commit, as chosen by `find_all`.
  */
 extern void find_bisection(struct commit_list **list, int *reaches, int *all,
-			   int find_all);
+			   int find_all, int only_merge_commits);
 
 extern struct commit_list *filter_skipped(struct commit_list *list,
 					  struct commit_list **tried,
@@ -28,7 +28,7 @@ struct rev_list_info {
 	const char *header_prefix;
 };
 
-extern int bisect_next_all(const char *prefix, int no_checkout);
+extern int bisect_next_all(const char *prefix, int no_checkout, int only_merge_commits);
 
 extern int estimate_bisect_steps(int all);
 
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 4b5fadcbe..9d7a1dd23 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -106,6 +106,32 @@ static void check_expected_revs(const char **revs, int rev_nr)
 	}
 }
 
+static GIT_PATH_FUNC(git_path_bisect_merges_only, "MERGES_ONLY_BISECT")
+
+static int merges_only(void)
+{
+	const char *filename = git_path_bisect_merges_only();
+	struct stat st;
+	struct strbuf str = STRBUF_INIT;
+	FILE *fp;
+	int res = 0;
+
+	if (stat(filename, &st) || !S_ISREG(st.st_mode))
+		return 0;
+
+	fp = fopen_or_warn(filename, "r");
+	if (!fp)
+		return 0;
+
+	if (strbuf_getline_lf(&str, fp) != EOF)
+		res = atoi(str.buf);
+
+	strbuf_release(&str);
+	fclose(fp);
+
+	return res;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
@@ -137,7 +163,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 
 	switch (cmdmode) {
 	case NEXT_ALL:
-		return bisect_next_all(prefix, no_checkout);
+		return bisect_next_all(prefix, no_checkout, merges_only());
 	case WRITE_TERMS:
 		if (argc != 2)
 			return error(_("--write-terms requires two arguments"));
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index fadd3ec14..cacffe6c6 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -538,7 +538,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 	if (bisect_list) {
 		int reaches, all;
 
-		find_bisection(&revs.commits, &reaches, &all, bisect_find_all);
+		find_bisection(&revs.commits, &reaches, &all, bisect_find_all, 0);
 
 		if (bisect_show_vars)
 			return show_bisect_vars(&info, reaches, all);
diff --git a/git-bisect.sh b/git-bisect.sh
index 54cbfecc5..730c983c5 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -82,6 +82,7 @@ bisect_start() {
 	bad_seen=0
 	eval=''
 	must_write_terms=0
+	merges_only=0
 	revs=''
 	if test "z$(git rev-parse --is-bare-repository)" != zfalse
 	then
@@ -96,6 +97,9 @@ bisect_start() {
 			shift
 			break
 		;;
+		--merges-only)
+			merges_only=1
+			shift ;;
 		--no-checkout)
 			mode=--no-checkout
 			shift ;;
@@ -212,6 +216,7 @@ bisect_start() {
 		git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || exit
 	fi &&
 	echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
+	echo "$merges_only" >"$GIT_DIR/MERGES_ONLY_BISECT" || exit
 	#
 	# Check if we can proceed to the next bisect state.
 	#
diff --git a/t/t6070-bisect-merge-commits.sh b/t/t6070-bisect-merge-commits.sh
new file mode 100755
index 000000000..edfae0cfd
--- /dev/null
+++ b/t/t6070-bisect-merge-commits.sh
@@ -0,0 +1,116 @@
+#!/bin/sh
+#
+# Copyright (c) 2018 Harald Nordgren
+#
+test_description='Tests git bisect merge commit functionality'
+
+exec </dev/null
+
+. ./test-lib.sh
+
+add_line_into_file()
+{
+    _line=$1
+    _file=$2
+
+    if [ -f "$_file" ]; then
+        echo "$_line" >> $_file || return $?
+        MSG="Add <$_line> into <$_file>."
+    else
+        echo "$_line" > $_file || return $?
+        git add $_file || return $?
+        MSG="Create file <$_file> with <$_line> inside."
+    fi
+
+    test_tick
+    git commit --quiet -m "$MSG" $_file
+}
+
+create_merge_commit()
+{
+    _branch=$1
+    _file=$2
+
+     git checkout -b $_branch &&
+     add_line_into_file "hello" $_file &&
+     git checkout master &&
+     git merge $_branch --no-edit --no-ff
+}
+
+HASH1=
+HASH2=
+HASH3=
+HASH4=
+HASH5=
+HASH6=
+
+test_expect_success 'set up basic repo with 3 files and 3 merge commits' '
+    add_line_into_file "hello" hello &&
+    HASH1=$(git rev-parse --verify HEAD) &&
+
+    add_line_into_file "hello" hello &&
+    git checkout -b branch1 &&
+    add_line_into_file "hello" hello1 &&
+    git checkout master &&
+    git checkout -b branch2 &&
+    add_line_into_file "hello" hello2 &&
+    git checkout master &&
+    git checkout -b branch3 &&
+    add_line_into_file "hello" hello3 &&
+    git checkout master &&
+    git merge branch1 --no-edit --no-ff &&
+    HASH2=$(git rev-parse --verify HEAD) &&
+
+    add_line_into_file "hello" hello &&
+    add_line_into_file "hello" hello &&
+    git merge branch2 --no-edit --no-ff &&
+    git merge branch3 --no-edit --no-ff &&
+    HASH3=$(git rev-parse --verify HEAD) &&
+
+    add_line_into_file "hello" hello &&
+    HASH4=$(git rev-parse --verify HEAD) &&
+
+    create_merge_commit branch4 hello4 &&
+    create_merge_commit branch5 hello5 &&
+    create_merge_commit branch6 hello6 &&
+    create_merge_commit branch7 hello7 &&
+    create_merge_commit branch8 hello8 &&
+    create_merge_commit branch9 hello9 &&
+    create_merge_commit branch10 hello10 &&
+    create_merge_commit branch11 hello11 &&
+    create_merge_commit branch12 hello12 &&
+    create_merge_commit branch13 hello13 &&
+    create_merge_commit branch14 hello14 &&
+    create_merge_commit branch15 hello15 &&
+    create_merge_commit branch16 hello16 &&
+    HASH5=$(git rev-parse --verify HEAD) &&
+
+    create_merge_commit branch17 hello17 &&
+    create_merge_commit branch18 hello18 &&
+    create_merge_commit branch19 hello19 &&
+    create_merge_commit branch20 hello20 &&
+    HASH6=$(git rev-parse --verify HEAD)
+'
+
+test_expect_success 'bisect skip: successful result' '
+	test_when_finished git bisect reset &&
+	git bisect reset &&
+	git bisect start $HASH4 $HASH1 --merges-only &&
+	git bisect good &&
+	git bisect good &&
+	git bisect bad > my_bisect_log.txt &&
+	grep "$HASH3 is the first bad merge" my_bisect_log.txt
+'
+
+test_expect_success 'bisect skip: successful result' '
+	test_when_finished git bisect reset &&
+	git bisect reset &&
+	git bisect start $HASH6 $HASH2 --merges-only &&
+	git bisect good &&
+	git bisect good &&
+	git bisect bad &&
+	git bisect bad > my_bisect_log.txt &&
+	grep "$HASH5 is the first bad merge" my_bisect_log.txt
+'
+
+test_done
-- 
2.14.3 (Apple Git-98)

Re: [PATCH] Create '--merges-only' option for 'git bisect'

[Stefan Beller]

On Wed, Apr 11, 2018 at 3:55 PM, Harald Nordgren
<haraldnordgren@gmail.com> wrote:
> When ran with '--merges-only', git bisect will only look at merge commits -- commits with 2 or more parents or the initial commit.

There has been quite some talk on the mailing list, e.g.
https://public-inbox.org/git/20160427204551.GB4613@virgo.localdomain/
which suggests a --first-parent mode instead. For certain histories
these are the same,
but merges-only is more restrictive for back-and-forth-cross merges.



>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
>
> Notes:
>     Proof of concept of a feature that I have wanted in Git for a while. In my daily work my company uses GitHub, which creates lots of merge commits. In general, tests are only ran on the tip of a branch before merging, so the different commits within a merge commit are allowed not to be buildable. Therefore 'git bisect' often doesn't work since it will give lots of false positives for anything that is not a merge commit. If we could have a feature to only bisect merge commits then it would be easier to pinpoint which merge causes any particular issue. After that, a bisect could be done within the merge to pinpoint futher. As a follow-up to this patch -- we could potentially create a feature that automatically continues into regular bisect within the bad merge commit after completed '--merges-only' bisection.

The github workflow you mention sounds as if --first-parent would do, too?

Re: [PATCH] Create '--merges-only' option for 'git bisect'

[Harald Nordgren]

I think it looks similar. But if I'm reading that thread correctly
then there was never a patch created, right?

On Thu, Apr 12, 2018 at 1:33 AM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Apr 11, 2018 at 3:55 PM, Harald Nordgren
> <haraldnordgren@gmail.com> wrote:
>> When ran with '--merges-only', git bisect will only look at merge commits -- commits with 2 or more parents or the initial commit.
>
> There has been quite some talk on the mailing list, e.g.
> https://public-inbox.org/git/20160427204551.GB4613@virgo.localdomain/
> which suggests a --first-parent mode instead. For certain histories
> these are the same,
> but merges-only is more restrictive for back-and-forth-cross merges.
>
>
>
>>
>> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
>> ---
>>
>> Notes:
>>     Proof of concept of a feature that I have wanted in Git for a while. In my daily work my company uses GitHub, which creates lots of merge commits. In general, tests are only ran on the tip of a branch before merging, so the different commits within a merge commit are allowed not to be buildable. Therefore 'git bisect' often doesn't work since it will give lots of false positives for anything that is not a merge commit. If we could have a feature to only bisect merge commits then it would be easier to pinpoint which merge causes any particular issue. After that, a bisect could be done within the merge to pinpoint futher. As a follow-up to this patch -- we could potentially create a feature that automatically continues into regular bisect within the bad merge commit after completed '--merges-only' bisection.
>
> The github workflow you mention sounds as if --first-parent would do, too?

Re: [PATCH] Create '--merges-only' option for 'git bisect'

[Christian Couder]

On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren
<haraldnordgren@gmail.com> wrote:
> I think it looks similar. But if I'm reading that thread correctly
> then there was never a patch created, right?

(It is customary on this mailing list to reply after the sentences we
reply to. We don't "top post".)

On the GSoC idea pages (like https://git.github.io/SoC-2018-Ideas/) we
have been suggesting "Implement git bisect --first-parent" and there
are the following related links:

https://public-inbox.org/git/20150304053333.GA9584@peff.net/
https://public-inbox.org/git/4D3CDDF9.6080405@intel.com/

Tiago in Cc also tried at a recent London hackathon to implement it
and came up with the following:

https://github.com/tiagonbotelho/git/pull/1/files

I tried to help him by reworking his commit in the following branch:

https://github.com/chriscool/git/commits/myfirstparent

Re: [PATCH] Create '--merges-only' option for 'git bisect'

[Tiago Botelho]

On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren
> <haraldnordgren@gmail.com> wrote:
>> I think it looks similar. But if I'm reading that thread correctly
>> then there was never a patch created, right?
>
> (It is customary on this mailing list to reply after the sentences we
> reply to. We don't "top post".)
>
> On the GSoC idea pages (like https://git.github.io/SoC-2018-Ideas/) we
> have been suggesting "Implement git bisect --first-parent" and there
> are the following related links:
>
> https://public-inbox.org/git/20150304053333.GA9584@peff.net/
> https://public-inbox.org/git/4D3CDDF9.6080405@intel.com/
>
> Tiago in Cc also tried at a recent London hackathon to implement it
> and came up with the following:
>
> https://github.com/tiagonbotelho/git/pull/1/files
>
> I tried to help him by reworking his commit in the following branch:
>
> https://github.com/chriscool/git/commits/myfirstparent

Thank you for the cc Christian, I’ve been quite busy and was not able
to work on the PR for quite some time.

I intended to pick it back up again next week. If it is ok with Harald
I would love to finish the PR that I started,
since it is quite close to being finished (I think it was just specs
missing if I am not mistaken).

Kind regards,

Tiago Botelho

Re: [PATCH] Create '--merges-only' option for 'git bisect'

[Johannes Schindelin]

Hi Harald,

Thank you for working on this.

On Thu, 12 Apr 2018, Harald Nordgren wrote:

> When ran with '--merges-only', git bisect will only look at merge
> commits -- commits with 2 or more parents or the initial commit.

This is an excellent idea!

>     Proof of concept of a feature that I have wanted in Git for a while.
>     In my daily work my company uses GitHub, which creates lots of merge
>     commits. In general, tests are only ran on the tip of a branch
>     before merging, so the different commits within a merge commit are
>     allowed not to be buildable. Therefore 'git bisect' often doesn't
>     work since it will give lots of false positives for anything that is
>     not a merge commit. If we could have a feature to only bisect merge
>     commits then it would be easier to pinpoint which merge causes any
>     particular issue. After that, a bisect could be done within the
>     merge to pinpoint futher. As a follow-up to this patch -- we could
>     potentially create a feature that automatically continues into
>     regular bisect within the bad merge commit after completed
>     '--merges-only' bisection.

It also helps when bisecting breakages in `pu` (mainly because the
branches in `pu` use branch points that are often insanely far in the
past).

> diff --git a/bisect.c b/bisect.c
> index a579b5088..e8a2f77c7 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -254,7 +254,7 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n
>   */
>  static struct commit_list *do_find_bisection(struct commit_list *list,
>  					     int nr, int *weights,
> -					     int find_all)
> +					     int find_all, int only_merge_commits)

How about `int flags`, and defining FIND_ALL and ONLY_MERGE_COMMITS?

That way, it will be easy to add ONLY_FIRST_PARENTS without changing the
signature of do_find_bisection().

Ideally, a preparatory commit would change find_all -> flags (adding
FIND_ALL), and the second commit would then add support for
ONLY_MERGE_COMMITS.

> @@ -266,6 +266,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  		unsigned flags = commit->object.flags;
>  
>  		p->item->util = &weights[n++];
> +
> +		if (only_merge_commits) {
> +			weight_set(p, -2);
> +			counted++;
> +			continue;
> +		}
> +

This hunk is a little hard to understand when you come from elsewhere, as
I do. Could you maybe explain a little bit what the `weight_set(p, -2)`
does? This is probably good material for enhancing the commit message
(seeing as we understand the commit message to be kind of the "convince me
that this is a good change, and explain things that are not immediately
obvious from the diff" document).

Maybe it would be enough to describe the role of the weight, and what the
typical values look like.

> @@ -305,11 +312,17 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  	 * way, and then fill the blanks using cheaper algorithm.
>  	 */
>  	for (p = list; p; p = p->next) {
> +		int distance;
>  		if (p->item->object.flags & UNINTERESTING)
>  			continue;
>  		if (weight(p) != -2)
>  			continue;
> -		weight_set(p, count_distance(p));
> +
> +		if (only_merge_commits)
> +			distance = count_distance(p) - 1;
> +		else
> +			distance = count_distance(p);
> +		weight_set(p, distance);

A shorter way:

		weight_set(p, count_distance(p) - !!only_merge_commits);

Could you add a code comment above this line to explain why this is
needed? (I have to admit that I have no clue what the weights or the
distances are in this context, but I think that a 3-line explanation could
probably give me enough of an idea that I do not have to study an hour or
three to learn enough to understand this change.)

>  		clear_distance(list);
>  
>  		/* Does it happen to be at exactly half-way? */
> @@ -330,11 +343,17 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  			for (q = p->item->parents; q; q = q->next) {
>  				if (q->item->object.flags & UNINTERESTING)
>  					continue;
> +				if (!q->item->util)
> +					break;

So we use the `util` field now to do things, probably to flag some
property of the commit.

Maybe the commit message could prepare the reader for this, with a
paragraph starting with "We use the commits' `util` field to store the
information that [...]"?

Seeing as this loop iterates over the commit's parents, I guess we are
looking at merge commits, and drop out early if we find a parent that was
not marked as interesting earlier?

If this is the case, why do we not have to look further, at later parents,
whether they *do* have a non-NULL `util` attribute?

>  				if (0 <= weight(q))
>  					break;
>  			}
>  			if (!q)
>  				continue;
> +			if (!q->item->util) {
> +				counted++;
> +				continue;
> +			}

Okay, so here we skip the current commit if we found a parent commit that
has the `util` field set to NULL...

> @@ -364,8 +383,43 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  		return best_bisection_sorted(list, nr);
>  }
>  
> +int merge_commit_or_root(const struct commit c)

By value? Let's use `*c` instead of `c` here.

> +{
> +	if (!c.parents)
> +		return 1;
> +
> +	return !!c.parents->next;

Unless you need the return value to be 0 or 1 (instead of 0 or non-0),
this is conciser:

	return !c.parents || c.parents->next;

However, we should not detect root commits here. We are only interested in
merges, according to the new option! So we should filter out root commits,
too. So this is what you really want:

	int is_merge(const struct commit *c)
	{
		return c->parents && c->parents->next;
	}

> +void filter_non_merge_commits(struct commit_list **commit_list)

So does this keep non_merge_commits (similar to Makefile's $(filter ...)
function?) Or does it remove them?

Judging by the code, maybe "filter_out_non_merge_commits()" or
"drop_regular_commits()"? Or "keep_only_root_and_merge_commits()"?

> +{
> +	struct commit_list *list1 = *commit_list;
> +	struct commit_list *list2 = NULL;
> +	*commit_list = NULL;
> +
> +	for ( ; list1; list1 = list1->next) {
> +		if (merge_commit_or_root(*list1->item)) {
> +			list2 = list1;
> +			list1 = list1->next;
> +			list2->next = NULL;

Since commit_lists are malloc()ed, they also need to be free()d when
filtering something out.

> +			list2->item->parents = NULL;

Why do we touch the parents here? This is a bit dangerous, as the item
refers to the commit that `lookup_commit()` would produce again. And by
setting parents = NULL, we pretend that those commits are root commits
from now on.

> +			*commit_list = list2;
> +			break;

Also, using the non-descriptive names `list1` and `list2` makes code a bit
hard to understand. (And hence it makes it easy for bugs to creep in.)

> +		}
> +	}
> +
> +	for ( ; list1; list1 = list1->next) {

The previous for loop has the condition `list1` already. So at this point
we know that `list1 == NULL`, and therefore this for loop is dead code.

Did you mean `for (list1 = *commit_list; list1; list1 = list1->next)`?

> +		list2->next = NULL;

Where is list2 set? Only in the previous loop, if it found a merge or root
commit. Otherwise, list2 would still be NULL and dereferenced here!

> +		if (merge_commit_or_root(*list1->item)) {
> +			list2->next = list1;
> +			list2 = list2->next;
> +			list2->item->parents = list1;

And here, we set the parents to list1... Again, this is messing with the
commits that come back when `lookup_commit()` is called later.

Changing the parents of a commit is something I imagine breaks too many
assumption to be a safe thing.

I'll read on for a little bit, maybe I get a better understanding what we
need to do in this here function.

> +		}
> +	}
> +}
> +
>  void find_bisection(struct commit_list **commit_list, int *reaches,
> -		    int *all, int find_all)
> +		    int *all, int find_all, int only_merge_commits)

Let's use a flag here, just as above.

>  {
>  	int nr, on_list;
>  	struct commit_list *list, *p, *best, *next, *last;
> @@ -373,6 +427,10 @@ void find_bisection(struct commit_list **commit_list, int *reaches,
>  
>  	show_list("bisection 2 entry", 0, 0, *commit_list);
>  
> +	if (only_merge_commits) {
> +		filter_non_merge_commits(commit_list);
> +	}
> +
>  	/*
>  	 * Count the number of total and tree-changing items on the
>  	 * list, while reversing the list.
> @@ -400,7 +458,7 @@ void find_bisection(struct commit_list **commit_list, int *reaches,
>  	weights = xcalloc(on_list, sizeof(*weights));
>  
>  	/* Do the real work of finding bisection commit. */
> -	best = do_find_bisection(list, nr, weights, find_all);
> +	best = do_find_bisection(list, nr, weights, find_all, only_merge_commits);
>  	if (best) {
>  		if (!find_all) {
>  			list->item = best->item;
> @@ -878,7 +936,7 @@ static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
>  /*
>   * This does "git diff-tree --pretty COMMIT" without one fork+exec.
>   */
> -static void show_diff_tree(const char *prefix, struct commit *commit)
> +static void show_diff_tree(const char *prefix, struct commit *commit, int only_merge_commits)
>  {
>  	struct rev_info opt;
>  
> @@ -893,6 +951,11 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
>  	opt.use_terminator = 0;
>  	opt.commit_format = CMIT_FMT_DEFAULT;
>  
> +	if (only_merge_commits) {
> +		opt.ignore_merges = 0;
> +		opt.combine_merges = 1;
> +	}

This is most likely what we want, whether --only-merge-commits was passed
or not. Let's make it the default (in a separate commit)?

> @@ -938,7 +1001,7 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
>   * If no_checkout is non-zero, the bisection process does not
>   * checkout the trial commit but instead simply updates BISECT_HEAD.
>   */
> -int bisect_next_all(const char *prefix, int no_checkout)
> +int bisect_next_all(const char *prefix, int no_checkout, int only_merge_commits)

Let's use `flags` here, too.

>  {
>  	struct rev_info revs;
>  	struct commit_list *tried;
> @@ -957,7 +1020,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
>  
>  	bisect_common(&revs);
>  
> -	find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr);
> +	find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr, only_merge_commits);

With the `flags` change, we would probably just introduce a local variable
`int flags = 0;` and set `flags |= !!skipped_revs.nr;` here at first, and
only change the `flags` variable to a parameter when introducing the
--only-merge-commits flag.

> @@ -983,10 +1046,14 @@ int bisect_next_all(const char *prefix, int no_checkout)
>  	bisect_rev = &revs.commits->item->object.oid;
>  
>  	if (!oidcmp(bisect_rev, current_bad_oid)) {
> +		char *format_string = NULL;
>  		exit_if_skipped_commits(tried, current_bad_oid);
> -		printf("%s is the first %s commit\n", oid_to_hex(bisect_rev),
> -			term_bad);
> -		show_diff_tree(prefix, revs.commits->item);
> +		if (only_merge_commits)
> +			format_string = "%s is the first %s merge\n";
> +		else
> +			format_string = "%s is the first %s commit\n";

... why? We can reference merge commits as "commits", too... Why not leave
the message as-is?

> diff --git a/bisect.h b/bisect.h
> index a5d9248a4..664ada180 100644
> --- a/bisect.h
> +++ b/bisect.h
> @@ -9,7 +9,7 @@
>   * best commit, as chosen by `find_all`.
>   */
>  extern void find_bisection(struct commit_list **list, int *reaches, int *all,
> -			   int find_all);
> +			   int find_all, int only_merge_commits);

Oh, I missed that this is a global function. So the flags should not be
FIND_ALL and ONLY_MERGE_COMMITS but BISECT_FIND_ALL and
BISECT_ONLY_MERGE_COMMITS, and `#define`d above this function.

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 4b5fadcbe..9d7a1dd23 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -106,6 +106,32 @@ static void check_expected_revs(const char **revs, int rev_nr)
>  	}
>  }
>  
> +static GIT_PATH_FUNC(git_path_bisect_merges_only, "MERGES_ONLY_BISECT")

The other bisect-specific files start with BISECT_, let's do that here,
too (i.e. "BISECT_ONLY_MERGES")?

> +static int merges_only(void)
> +{
> +	const char *filename = git_path_bisect_merges_only();
> +	struct stat st;
> +	struct strbuf str = STRBUF_INIT;
> +	FILE *fp;
> +	int res = 0;
> +
> +	if (stat(filename, &st) || !S_ISREG(st.st_mode))
> +		return 0;
> +
> +	fp = fopen_or_warn(filename, "r");
> +	if (!fp)
> +		return 0;
> +
> +	if (strbuf_getline_lf(&str, fp) != EOF)
> +		res = atoi(str.buf);

Why not use the presence of the file as flag, rather than its contents?
Then the test becomes as simple as

	int merges_only = file_exists(git_path_bisect_merges_only());

> +
> +	strbuf_release(&str);
> +	fclose(fp);
> +
> +	return res;
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>  	enum {
> @@ -137,7 +163,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  
>  	switch (cmdmode) {
>  	case NEXT_ALL:
> -		return bisect_next_all(prefix, no_checkout);
> +		return bisect_next_all(prefix, no_checkout, merges_only());
>  	case WRITE_TERMS:
>  		if (argc != 2)
>  			return error(_("--write-terms requires two arguments"));
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index fadd3ec14..cacffe6c6 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -538,7 +538,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
>  	if (bisect_list) {
>  		int reaches, all;
>  
> -		find_bisection(&revs.commits, &reaches, &all, bisect_find_all);
> +		find_bisection(&revs.commits, &reaches, &all, bisect_find_all, 0);

This would become bisect_flags in the preparatory patch.

>  
>  		if (bisect_show_vars)
>  			return show_bisect_vars(&info, reaches, all);
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 54cbfecc5..730c983c5 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -82,6 +82,7 @@ bisect_start() {
>  	bad_seen=0
>  	eval=''
>  	must_write_terms=0
> +	merges_only=0
>  	revs=''
>  	if test "z$(git rev-parse --is-bare-repository)" != zfalse
>  	then
> @@ -96,6 +97,9 @@ bisect_start() {
>  			shift
>  			break
>  		;;
> +		--merges-only)
> +			merges_only=1
> +			shift ;;
>  		--no-checkout)
>  			mode=--no-checkout
>  			shift ;;
> @@ -212,6 +216,7 @@ bisect_start() {
>  		git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || exit
>  	fi &&
>  	echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
> +	echo "$merges_only" >"$GIT_DIR/MERGES_ONLY_BISECT" || exit

The git-bisect.sh script deviates from our coding convention, it seems.
Typically, we would write

	merges_only=
	[...]
		--merges-only)
			merges_only=t
			;;
	[...]
	test -z "$merges_only" || : >"$GIT_DIR/BISECT_ONLY_MERGES" || exit

I would prefer that here, too.

> diff --git a/t/t6070-bisect-merge-commits.sh b/t/t6070-bisect-merge-commits.sh
> new file mode 100755
> index 000000000..edfae0cfd
> --- /dev/null
> +++ b/t/t6070-bisect-merge-commits.sh
> @@ -0,0 +1,116 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2018 Harald Nordgren
> +#
> +test_description='Tests git bisect merge commit functionality'
> +
> +exec </dev/null

This is unnecessary, as our test-lib.sh already executes `exec 6<&0`.
Further, it is not only unnecessary, but also breaks debugging with the
`debug` shell function defined in test-lib-function.sh. Let's just drop
this statement.

> +
> +. ./test-lib.sh
> +
> +add_line_into_file()
> +{
> +    _line=$1
> +    _file=$2
> +
> +    if [ -f "$_file" ]; then
> +        echo "$_line" >> $_file || return $?
> +        MSG="Add <$_line> into <$_file>."
> +    else
> +        echo "$_line" > $_file || return $?
> +        git add $_file || return $?
> +        MSG="Create file <$_file> with <$_line> inside."
> +    fi
> +
> +    test_tick
> +    git commit --quiet -m "$MSG" $_file
> +}

It would appear that the contents of the files written using this function
are never tested.

Let's just use `test_commit` instead.

> +
> +create_merge_commit()
> +{
> +    _branch=$1
> +    _file=$2
> +
> +     git checkout -b $_branch &&
> +     add_line_into_file "hello" $_file &&
> +     git checkout master &&
> +     git merge $_branch --no-edit --no-ff
> +}

This not only creates a merge commit but also sets up a branch and adds a
commit to it. I'd much rather have a function that performs the merge
after calling `test_tick`, and then tags the result just like
`test_commit` would do.

> +HASH1=
> +HASH2=
> +HASH3=
> +HASH4=
> +HASH5=
> +HASH6=

These are too non-descriptive names to be useful. Remember that the
purpose of a regression test is to help debugging regressions. If I
encounter a regression while developing a patch series that breaks this
test, I really need to understand the regression test as quickly as
possible.

So let's use HEAD~4 and/or the tags creates by `test_commit` instead.

> +test_expect_success 'set up basic repo with 3 files and 3 merge commits' '

If we say 'setup' here, it is not only conciser, but also less prone to
become incorrect in the future.

> +    add_line_into_file "hello" hello &&
> +    HASH1=$(git rev-parse --verify HEAD) &&
> +
> +    add_line_into_file "hello" hello &&
> +    git checkout -b branch1 &&
> +    add_line_into_file "hello" hello1 &&
> +    git checkout master &&
> +    git checkout -b branch2 &&
> +    add_line_into_file "hello" hello2 &&
> +    git checkout master &&
> +    git checkout -b branch3 &&
> +    add_line_into_file "hello" hello3 &&
> +    git checkout master &&
> +    git merge branch1 --no-edit --no-ff &&
> +    HASH2=$(git rev-parse --verify HEAD) &&
> +
> +    add_line_into_file "hello" hello &&
> +    add_line_into_file "hello" hello &&
> +    git merge branch2 --no-edit --no-ff &&
> +    git merge branch3 --no-edit --no-ff &&
> +    HASH3=$(git rev-parse --verify HEAD) &&
> +
> +    add_line_into_file "hello" hello &&
> +    HASH4=$(git rev-parse --verify HEAD) &&
> +
> +    create_merge_commit branch4 hello4 &&
> +    create_merge_commit branch5 hello5 &&
> +    create_merge_commit branch6 hello6 &&
> +    create_merge_commit branch7 hello7 &&
> +    create_merge_commit branch8 hello8 &&
> +    create_merge_commit branch9 hello9 &&
> +    create_merge_commit branch10 hello10 &&
> +    create_merge_commit branch11 hello11 &&
> +    create_merge_commit branch12 hello12 &&
> +    create_merge_commit branch13 hello13 &&
> +    create_merge_commit branch14 hello14 &&
> +    create_merge_commit branch15 hello15 &&
> +    create_merge_commit branch16 hello16 &&
> +    HASH5=$(git rev-parse --verify HEAD) &&
> +
> +    create_merge_commit branch17 hello17 &&
> +    create_merge_commit branch18 hello18 &&
> +    create_merge_commit branch19 hello19 &&
> +    create_merge_commit branch20 hello20 &&
> +    HASH6=$(git rev-parse --verify HEAD)
> +'

It is next to impossible to understand this function in an 80x25 terminal
(which many of us old-timers still use). And it is not even necessary in
order to test the functionality you introduced...

So how about this strategy: first, paint a nice little diagram of the
commit graph you want to achieve, and make it not as insanely complex.
Then, set that up using `test_commit`, `git checkout -b` and `test_tick &&
git merge`. Something like this:

# We generate the following commit graph:
#
# A - B - C - F
#   \   \   /   \
#     D - E - G - H

test_merge () {
	test_tick &&
	git merge "$@" &&
	git tag "$1"
}

test_expect_success 'setup' '
	test_commit A &&
	test_commit B &&
	test_commit C &&
	git reset --hard A &&
	test_commit D &&
	test_merge -m E B &&
	git reset --hard C &&
	git merge -m F E &&
	git reset --hard G &&
	git merge -m H F
'

> +test_expect_success 'bisect skip: successful result' '

I do not actually see that `bisect skip` is tested here.

> +	test_when_finished git bisect reset &&
> +	git bisect reset &&
> +	git bisect start $HASH4 $HASH1 --merges-only &&
> +	git bisect good &&
> +	git bisect good &&
> +	git bisect bad > my_bisect_log.txt &&
> +	grep "$HASH3 is the first bad merge" my_bisect_log.txt
> +'

What we want to verify here is that we only encounter merge commits,
right? So that is what we should test:

test_expect_success '--merges-only' '
	write_script find-c.sh <<-\EOF &&
	case "$(git show --format=%p -s)" in
	*\ *) ;; # merge commit: okay
	*) git rev-parse HEAD >non-merge-found;;
	esac
	# detect whether we are "before" or "after" C
	test ! -f C.t
	EOF

	git bisect start HEAD A &&
	git bisect run ./find-c.sh >actual &&
	test_path_is_missing non-merge-found &&
	grep "$(git rev-parse F) is the first bad commit" actual
'

> +
> +test_expect_success 'bisect skip: successful result' '
> +	test_when_finished git bisect reset &&
> +	git bisect reset &&
> +	git bisect start $HASH6 $HASH2 --merges-only &&
> +	git bisect good &&
> +	git bisect good &&
> +	git bisect bad &&
> +	git bisect bad > my_bisect_log.txt &&
> +	grep "$HASH5 is the first bad merge" my_bisect_log.txt
> +'
> +
> +test_done

So after reading this, I am quite a bit more puzzled about that
commit->util field we seem to use. Could you clarify that in the commit
message, to give readers a head start?

Okay, now let's come back to that function accepting a commit_list and
dropping all non-merge commits from it.

First of all, I do not think that we actually make use of the parents
information after calling this function, right? If we did, I think it
would be horribly broken because all non-merge commits would now be
looking like root commits, and all the merge commits would pretend to have
*all* of the previous merge commits as parents (the first merge commit
would therefore look like a root commit, the second merge commit would
look like a non-merge, and so on).

So let's just drop the parent rewriting? If we do need to collapse the
commit graph to the merge commit nodes, then a much better idea would be
to use an oidset with commit_lists representing the collapsed vertices in
that graph (i.e. for every commit, it would replace the non-merge parents
with the respective closest ancestor that *is* a merge, and this list
would be constructed in reverse topological order to give linear
complexity).

Under the assumption that we can leave the commits' parents alone, this is
my proposed function to keep only merge commits in a commit_list:

static void keep_only_merge_commits(struct commit_list **p)
{
	struct commit_list *parents, *next;

	while (*p) {
		parents = (*p)->item->parents;
		if (parents && parents->next) {
			/* merge commit: keep */
			p = &(*p)->next;
			continue;
		}

		/* non-merge commit: free() and skip */
		next = (*p)->next;
		free(*p);
		*p = next;
	}
}

This makes use of the fact that both the original list of commits, as well
as the `next` field in the commit_list items, is a pointer to a
commit_list. So once we processed the first item of the list, we either
free it and replace the list by the remainder of the list, or we advance
to the "sub list" starting with the second item of the list (if there is
any left).

Ciao,
Johannes

Re: [PATCH] Create '--merges-only' option for 'git bisect'

[Johannes Schindelin]

Hi Stefan,

On Wed, 11 Apr 2018, Stefan Beller wrote:

> On Wed, Apr 11, 2018 at 3:55 PM, Harald Nordgren
> <haraldnordgren@gmail.com> wrote:
> > When ran with '--merges-only', git bisect will only look at merge commits -- commits with 2 or more parents or the initial commit.
> 
> There has been quite some talk on the mailing list, e.g.
> https://public-inbox.org/git/20160427204551.GB4613@virgo.localdomain/
> which suggests a --first-parent mode instead.

I like that mode, but I would love to have *both*. And from what I see, it
should be relatively easy to add the --first-parent mode on top of
Harald's patches.

> For certain histories these are the same, but merges-only is more
> restrictive for back-and-forth-cross merges.

You mean merges-only tests *more* in back-and-forth-cross-merges
scenarios?

Ciao,
Dscho

Re: [PATCH] Create '--merges-only' option for 'git bisect'

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3309 bytes --]

Hi Tiago,

On Thu, 12 Apr 2018, Tiago Botelho wrote:

> On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder
> <christian.couder@gmail.com> wrote:
> > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren
> > <haraldnordgren@gmail.com> wrote:
> >> I think it looks similar. But if I'm reading that thread correctly
> >> then there was never a patch created, right?
> >
> > (It is customary on this mailing list to reply after the sentences we
> > reply to. We don't "top post".)
> >
> > On the GSoC idea pages (like https://git.github.io/SoC-2018-Ideas/) we
> > have been suggesting "Implement git bisect --first-parent" and there
> > are the following related links:
> >
> > https://public-inbox.org/git/20150304053333.GA9584@peff.net/
> > https://public-inbox.org/git/4D3CDDF9.6080405@intel.com/
> >
> > Tiago in Cc also tried at a recent London hackathon to implement it
> > and came up with the following:
> >
> > https://github.com/tiagonbotelho/git/pull/1/files
> >
> > I tried to help him by reworking his commit in the following branch:
> >
> > https://github.com/chriscool/git/commits/myfirstparent
> 
> Thank you for the cc Christian, I’ve been quite busy and was not able
> to work on the PR for quite some time.
> 
> I intended to pick it back up again next week. If it is ok with Harald
> I would love to finish the PR that I started,
> since it is quite close to being finished (I think it was just specs
> missing if I am not mistaken).

That looks promising. Just like I suggested to Harald in another reply
[*1*] on this thread, you probably want to use `int flags` instead, and
turn `find_all` into BISECT_FIND_ALL in a preparatory commit.

Also, you will definitely want to add a test. Again, my reply to Harald
[*1*] should give you a head start there... You will want to imitate the
test case I outlined there, maybe something like:

# A - B - C - F
#   \   \   /   \
#     D - E - G - H

[... 'setup' as in my mail to Harald ...]

test_expect_success '--first-parent' '
	write_script find-e.sh <<-\EOF &&
	case "$(git show --format=%s -s)" in
	B|C|F) ;; # first parent lineage: okay
	*) git show -s --oneline HEAD >unexpected;;
	esac
	# detect whether we are "before" or "after" E
	test ! -f E.t
	EOF

	git bisect start --first-parent HEAD A &&
	git bisect run ./find-e.sh >actual &&
	test_path_is_missing unexpected &&
	grep "$(git rev-parse F) is the first bad commit" actual
'

Also, Tiago, reading through your patch (as on chriscool/git; do you have
your own fork? That would make it much easier to collaborate with you by
offering PRs), it looks more straight-forward than editing the commit_list
after the fact and adding magic weights ;-)

Except for one thing. I wonder why `bisect_next_all()` does not set
revs.first_parent_only after calling `bisect_rev_setup()`? You would still
need the changes in `count_distance()`, as it performs its own commit
graph traversal, but there is no need to enumerate too many commits in the
first place, right?

Harald, maybe --merges-only can be implemented on top of --first-parent,
with the `int flags` change I suggested?

Ciao,
Johannes

Footnote *1*:
https://public-inbox.org/git/nycvar.QRO.7.76.6.1804121143120.65@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz/

Re: [PATCH] Create '--merges-only' option for 'git bisect'

[Tiago Botelho]

On Thu, Apr 12, 2018 at 12:53 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Tiago,
>
> On Thu, 12 Apr 2018, Tiago Botelho wrote:
>
>> On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder
>> <christian.couder@gmail.com> wrote:
>> > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren
>> > <haraldnordgren@gmail.com> wrote:
>> >> I think it looks similar. But if I'm reading that thread correctly
>> >> then there was never a patch created, right?
>> >
>> > (It is customary on this mailing list to reply after the sentences we
>> > reply to. We don't "top post".)
>> >
>> > On the GSoC idea pages (like https://git.github.io/SoC-2018-Ideas/) we
>> > have been suggesting "Implement git bisect --first-parent" and there
>> > are the following related links:
>> >
>> > https://public-inbox.org/git/20150304053333.GA9584@peff.net/
>> > https://public-inbox.org/git/4D3CDDF9.6080405@intel.com/
>> >
>> > Tiago in Cc also tried at a recent London hackathon to implement it
>> > and came up with the following:
>> >
>> > https://github.com/tiagonbotelho/git/pull/1/files
>> >
>> > I tried to help him by reworking his commit in the following branch:
>> >
>> > https://github.com/chriscool/git/commits/myfirstparent
>>
>> Thank you for the cc Christian, I’ve been quite busy and was not able
>> to work on the PR for quite some time.
>>
>> I intended to pick it back up again next week. If it is ok with Harald
>> I would love to finish the PR that I started,
>> since it is quite close to being finished (I think it was just specs
>> missing if I am not mistaken).
>
> That looks promising. Just like I suggested to Harald in another reply
> [*1*] on this thread, you probably want to use `int flags` instead, and
> turn `find_all` into BISECT_FIND_ALL in a preparatory commit.
>
> Also, you will definitely want to add a test. Again, my reply to Harald
> [*1*] should give you a head start there... You will want to imitate the
> test case I outlined there, maybe something like:
>
> # A - B - C - F
> #   \   \   /   \
> #     D - E - G - H
>
> [... 'setup' as in my mail to Harald ...]
>
> test_expect_success '--first-parent' '
>         write_script find-e.sh <<-\EOF &&
>         case "$(git show --format=%s -s)" in
>         B|C|F) ;; # first parent lineage: okay
>         *) git show -s --oneline HEAD >unexpected;;
>         esac
>         # detect whether we are "before" or "after" E
>         test ! -f E.t
>         EOF
>
>         git bisect start --first-parent HEAD A &&
>         git bisect run ./find-e.sh >actual &&
>         test_path_is_missing unexpected &&
>         grep "$(git rev-parse F) is the first bad commit" actual
> '
>
> Also, Tiago, reading through your patch (as on chriscool/git; do you have
> your own fork? That would make it much easier to collaborate with you by
> offering PRs), it looks more straight-forward than editing the commit_list
> after the fact and adding magic weights ;-)
>
> Except for one thing. I wonder why `bisect_next_all()` does not set
> revs.first_parent_only after calling `bisect_rev_setup()`? You would still
> need the changes in `count_distance()`, as it performs its own commit
> graph traversal, but there is no need to enumerate too many commits in the
> first place, right?
>
> Harald, maybe --merges-only can be implemented on top of --first-parent,
> with the `int flags` change I suggested?
>
> Ciao,
> Johannes
>
> Footnote *1*:
> https://public-inbox.org/git/nycvar.QRO.7.76.6.1804121143120.65@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz/

Hi Johannes,

Thank you for the pointers!

I do have my own fork, you can see it here:
https://github.com/tiagonbotelho/git/pull/1/commits
which applies the changes Chris made to my first draft of the solution.

I still have to add him as co-author of that commit.

Cheers,
Tiago Botelho

Re: [PATCH] Create '--merges-only' option for 'git bisect'

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1843 bytes --]

Hi Tiago,

> > On Thu, 12 Apr 2018, Tiago Botelho wrote:
> >
> >> On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder
> >> <christian.couder@gmail.com> wrote:
> >> > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren
> >> > <haraldnordgren@gmail.com> wrote:
> >> >> I think it looks similar. But if I'm reading that thread correctly
> >> >> then there was never a patch created, right?
> >> >
> >> > (It is customary on this mailing list to reply after the sentences we
> >> > reply to. We don't "top post".)
> >> >
> >> > On the GSoC idea pages (like https://git.github.io/SoC-2018-Ideas/) we
> >> > have been suggesting "Implement git bisect --first-parent" and there
> >> > are the following related links:
> >> >
> >> > https://public-inbox.org/git/20150304053333.GA9584@peff.net/
> >> > https://public-inbox.org/git/4D3CDDF9.6080405@intel.com/
> >> >
> >> > Tiago in Cc also tried at a recent London hackathon to implement it
> >> > and came up with the following:
> >> >
> >> > https://github.com/tiagonbotelho/git/pull/1/files
> >> >
> >> > I tried to help him by reworking his commit in the following branch:
> >> >
> >> > https://github.com/chriscool/git/commits/myfirstparent
> >>
> >> Thank you for the cc Christian, I’ve been quite busy and was not able
> >> to work on the PR for quite some time.
> >>
> >> I intended to pick it back up again next week. If it is ok with
> >> Harald I would love to finish the PR that I started, since it is
> >> quite close to being finished (I think it was just specs missing if I
> >> am not mistaken).

It is now well after "next week". Are there any news? Or could you unblock
Harald by stating that you won't come back to it any time soon (in
particular since the PR is not quite as finished from my reading as you
made it sound...)?

Ciao,
Johannes

Re: [PATCH] Create '--merges-only' option for 'git bisect'

[Tiago Botelho]

On Mon, Apr 30, 2018 at 12:31 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Tiago,
>
>> > On Thu, 12 Apr 2018, Tiago Botelho wrote:
>> >
>> >> On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder
>> >> <christian.couder@gmail.com> wrote:
>> >> > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren
>> >> > <haraldnordgren@gmail.com> wrote:
>> >> >> I think it looks similar. But if I'm reading that thread correctly
>> >> >> then there was never a patch created, right?
>> >> >
>> >> > (It is customary on this mailing list to reply after the sentences we
>> >> > reply to. We don't "top post".)
>> >> >
>> >> > On the GSoC idea pages (like https://git.github.io/SoC-2018-Ideas/) we
>> >> > have been suggesting "Implement git bisect --first-parent" and there
>> >> > are the following related links:
>> >> >
>> >> > https://public-inbox.org/git/20150304053333.GA9584@peff.net/
>> >> > https://public-inbox.org/git/4D3CDDF9.6080405@intel.com/
>> >> >
>> >> > Tiago in Cc also tried at a recent London hackathon to implement it
>> >> > and came up with the following:
>> >> >
>> >> > https://github.com/tiagonbotelho/git/pull/1/files
>> >> >
>> >> > I tried to help him by reworking his commit in the following branch:
>> >> >
>> >> > https://github.com/chriscool/git/commits/myfirstparent
>> >>
>> >> Thank you for the cc Christian, I’ve been quite busy and was not able
>> >> to work on the PR for quite some time.
>> >>
>> >> I intended to pick it back up again next week. If it is ok with
>> >> Harald I would love to finish the PR that I started, since it is
>> >> quite close to being finished (I think it was just specs missing if I
>> >> am not mistaken).
>
> It is now well after "next week". Are there any news? Or could you unblock
> Harald by stating that you won't come back to it any time soon (in
> particular since the PR is not quite as finished from my reading as you
> made it sound...)?
>
> Ciao,
> Johannes

I've been working on the feature for the past week
https://github.com/tiagonbotelho/git/commit/709e2e248ebfb1deab12fd7d3da4611002dfaf86#diff-118df990fd68a0929bca5441fea06fc7

I have some comments sent by Christian I plan on fixing this week

Thank you,

-- 
Tiago Botelho

Re: [PATCH] Create '--merges-only' option for 'git bisect'

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2461 bytes --]

Hi Tiago,

On Mon, 30 Apr 2018, Tiago Botelho wrote:

> On Mon, Apr 30, 2018 at 12:31 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> >> > On Thu, 12 Apr 2018, Tiago Botelho wrote:
> >> >
> >> >> On Thu, Apr 12, 2018 at 9:58 AM, Christian Couder
> >> >> <christian.couder@gmail.com> wrote:
> >> >> > On Thu, Apr 12, 2018 at 9:49 AM, Harald Nordgren
> >> >> > <haraldnordgren@gmail.com> wrote:
> >> >> >> I think it looks similar. But if I'm reading that thread correctly
> >> >> >> then there was never a patch created, right?
> >> >> >
> >> >> > (It is customary on this mailing list to reply after the sentences we
> >> >> > reply to. We don't "top post".)
> >> >> >
> >> >> > On the GSoC idea pages (like https://git.github.io/SoC-2018-Ideas/) we
> >> >> > have been suggesting "Implement git bisect --first-parent" and there
> >> >> > are the following related links:
> >> >> >
> >> >> > https://public-inbox.org/git/20150304053333.GA9584@peff.net/
> >> >> > https://public-inbox.org/git/4D3CDDF9.6080405@intel.com/
> >> >> >
> >> >> > Tiago in Cc also tried at a recent London hackathon to implement it
> >> >> > and came up with the following:
> >> >> >
> >> >> > https://github.com/tiagonbotelho/git/pull/1/files
> >> >> >
> >> >> > I tried to help him by reworking his commit in the following branch:
> >> >> >
> >> >> > https://github.com/chriscool/git/commits/myfirstparent
> >> >>
> >> >> Thank you for the cc Christian, I’ve been quite busy and was not able
> >> >> to work on the PR for quite some time.
> >> >>
> >> >> I intended to pick it back up again next week. If it is ok with
> >> >> Harald I would love to finish the PR that I started, since it is
> >> >> quite close to being finished (I think it was just specs missing if I
> >> >> am not mistaken).
> >
> > It is now well after "next week". Are there any news? Or could you unblock
> > Harald by stating that you won't come back to it any time soon (in
> > particular since the PR is not quite as finished from my reading as you
> > made it sound...)?
> >
> > Ciao,
> > Johannes
> 
> I've been working on the feature for the past week
> https://github.com/tiagonbotelho/git/commit/709e2e248ebfb1deab12fd7d3da4611002dfaf86#diff-118df990fd68a0929bca5441fea06fc7
> 
> I have some comments sent by Christian I plan on fixing this week

Okay, great! I was concerned because of the long silence.

Ciao,
Johannes