git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [PATCH 0/3] Introduce --first-parent flag for git bisect
@ 2020-07-28 15:44 Aaron Lipman via GitGitGadget
  2020-07-28 15:44 ` [PATCH 1/3] rev-list: allow bisect and first-parent flags Aaron Lipman via GitGitGadget
                   ` (3 more replies)
  0 siblings, 4 replies; 32+ messages in thread
From: Aaron Lipman via GitGitGadget @ 2020-07-28 15:44 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman

I've always wished git bisect had a first-parent flag, e.g. "git bisect
start --first-parent"

This option would be particularly useful in avoiding false positives when
bisecting, if a merged branch contained broken or non-buildable commits, but
the merge itself was OK.

There have been a couple attempts/iterations towards this functionality in
the past two years or so, and I'd like to get this across the finish line.

The previous iterations have focussed on the preliminary step of editing
functions in bisect.c to accept a first_parent_only parameter, enabling the
--bisect and --first-parent flags to be used in conjunction with git
rev-list. In addition to updating git rev-list, I intend to enable the
--first-parent flag on git bisect as well.

I've taken some of the feedback from previous iterations into account,
specifically tidying up the code that assigns weights to commits in
do_find_bisection(), utilizing existing commit graphs when running tests,
and clarifying the test for "git rev-list --bisect-all --first-parent"

Previous two iterations (most recent first):
https://lore.kernel.org/git/20191105052141.15913-1-workingjubilee@gmail.com/
https://lore.kernel.org/git/20180828123234.44582-1-tiagonbotelho@hotmail.com/

Related discussion about combining multiple boolean params passed to
find_bisection() into a single unsigned integer:
https://lore.kernel.org/git/20180415085841.1269-1-haraldnordgren@gmail.com/

Aaron Lipman (3):
  rev-list: allow bisect and first-parent flags
  bisect: introduce first-parent flag
  bisect: combine args passed to find_bisection()

 Documentation/git-bisect.txt       | 13 +++++-
 Documentation/rev-list-options.txt |  7 ++--
 bisect.c                           | 66 +++++++++++++++++++++---------
 bisect.h                           |  7 +++-
 builtin/bisect--helper.c           | 16 ++++++--
 builtin/rev-list.c                 |  9 +++-
 revision.c                         |  3 --
 t/t6000-rev-list-misc.sh           |  4 +-
 t/t6002-rev-list-bisect.sh         | 45 ++++++++++++++++++++
 t/t6030-bisect-porcelain.sh        | 17 ++++++++
 10 files changed, 151 insertions(+), 36 deletions(-)


base-commit: 00a7a21b97a47889cf66de32f8defed023c14c2c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-686%2Falipman88%2Fbisect_first_parent_final_draft-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-686/alipman88/bisect_first_parent_final_draft-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/686
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 1/3] rev-list: allow bisect and first-parent flags
  2020-07-28 15:44 [PATCH 0/3] Introduce --first-parent flag for git bisect Aaron Lipman via GitGitGadget
@ 2020-07-28 15:44 ` Aaron Lipman via GitGitGadget
  2020-07-28 20:23   ` Junio C Hamano
  2020-07-28 15:44 ` [PATCH 2/3] bisect: introduce first-parent flag Aaron Lipman via GitGitGadget
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 32+ messages in thread
From: Aaron Lipman via GitGitGadget @ 2020-07-28 15:44 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman, Aaron Lipman

From: Aaron Lipman <alipman88@gmail.com>

Add first_parent_only parameter to find_bisection(), removing the
barrier that prevented combining the --bisect and --first-parent flags
when using git rev-list

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
Based-on-patch-by: Tiago Botelho <tiagonbotelho@hotmail.com>
---
 Documentation/rev-list-options.txt |  7 ++---
 bisect.c                           | 43 +++++++++++++++++-----------
 bisect.h                           |  2 +-
 builtin/rev-list.c                 |  2 +-
 revision.c                         |  3 --
 t/t6000-rev-list-misc.sh           |  4 +--
 t/t6002-rev-list-bisect.sh         | 45 ++++++++++++++++++++++++++++++
 7 files changed, 79 insertions(+), 27 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index b01b2b6773..d3117ce51b 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -128,8 +128,7 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).
 	because merges into a topic branch tend to be only about
 	adjusting to updated upstream from time to time, and
 	this option allows you to ignore the individual commits
-	brought in to your history by such a merge. Cannot be
-	combined with --bisect.
+	brought in to your history by such a merge.
 
 --not::
 	Reverses the meaning of the '{caret}' prefix (or lack thereof)
@@ -207,7 +206,7 @@ ifndef::git-rev-list[]
 	Pretend as if the bad bisection ref `refs/bisect/bad`
 	was listed and as if it was followed by `--not` and the good
 	bisection refs `refs/bisect/good-*` on the command
-	line. Cannot be combined with --first-parent.
+	line.
 endif::git-rev-list[]
 
 --stdin::
@@ -743,7 +742,7 @@ outputs 'midpoint', the output of the two commands
 would be of roughly the same length.  Finding the change which
 introduces a regression is thus reduced to a binary search: repeatedly
 generate and test new 'midpoint's until the commit chain is of length
-one. Cannot be combined with --first-parent.
+one.
 
 --bisect-vars::
 	This calculates the same as `--bisect`, except that refs in
diff --git a/bisect.c b/bisect.c
index d5e830410f..762f68c8e9 100644
--- a/bisect.c
+++ b/bisect.c
@@ -37,7 +37,7 @@ static const char *term_good;
  * We care just barely enough to avoid recursing for
  * non-merge entries.
  */
-static int count_distance(struct commit_list *entry)
+static int count_distance(struct commit_list *entry, int first_parent_only)
 {
 	int nr = 0;
 
@@ -52,10 +52,10 @@ static int count_distance(struct commit_list *entry)
 		commit->object.flags |= COUNTED;
 		p = commit->parents;
 		entry = p;
-		if (p) {
+		if (p && !first_parent_only) {
 			p = p->next;
 			while (p) {
-				nr += count_distance(p);
+				nr += count_distance(p, first_parent_only);
 				p = p->next;
 			}
 		}
@@ -88,15 +88,16 @@ static inline void weight_set(struct commit_list *elem, int weight)
 	**commit_weight_at(&commit_weight, elem->item) = weight;
 }
 
-static int count_interesting_parents(struct commit *commit)
+static int count_interesting_parents(struct commit *commit, int first_parent_only)
 {
 	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 (first_parent_only)
+			break;
 	}
 	return count;
 }
@@ -259,7 +260,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 first_parent_only)
 {
 	int n, counted;
 	struct commit_list *p;
@@ -271,7 +272,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 		unsigned flags = commit->object.flags;
 
 		*commit_weight_at(&commit_weight, p->item) = &weights[n++];
-		switch (count_interesting_parents(commit)) {
+		switch (count_interesting_parents(commit, first_parent_only)) {
 		case 0:
 			if (!(flags & TREESAME)) {
 				weight_set(p, 1);
@@ -314,7 +315,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 			continue;
 		if (weight(p) != -2)
 			continue;
-		weight_set(p, count_distance(p));
+		weight_set(p, count_distance(p, first_parent_only));
 		clear_distance(list);
 
 		/* Does it happen to be at exactly half-way? */
@@ -330,13 +331,21 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 			struct commit_list *q;
 			unsigned flags = p->item->object.flags;
 
+			/* if commit p has already been weighted, continue. */
 			if (0 <= weight(p))
 				continue;
+
+			/*
+			 * otherwise, find the first interesting
+			 * already-weighted parent of p and store as q.
+			 */
 			for (q = p->item->parents; q; q = q->next) {
-				if (q->item->object.flags & UNINTERESTING)
-					continue;
-				if (0 <= weight(q))
+				if (!(q->item->object.flags & UNINTERESTING) && 0 <= weight(q)) {
+					break;
+				} else if (first_parent_only) {
+					q = NULL;
 					break;
+				}
 			}
 			if (!q)
 				continue;
@@ -370,7 +379,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 }
 
 void find_bisection(struct commit_list **commit_list, int *reaches,
-		    int *all, int find_all)
+		    int *all, int find_all, int first_parent_only)
 {
 	int nr, on_list;
 	struct commit_list *list, *p, *best, *next, *last;
@@ -406,7 +415,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, first_parent_only);
 	if (best) {
 		if (!find_all) {
 			list->item = best->item;
@@ -991,6 +1000,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
 	enum bisect_error res = BISECT_OK;
 	struct object_id *bisect_rev;
 	char *steps_msg;
+	int first_parent_only = 0; /* TODO: pass --first-parent flag from git bisect start */
 
 	read_bisect_terms(&term_bad, &term_good);
 	if (read_bisect_refs())
@@ -1001,11 +1011,12 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
 		return res;
 
 	bisect_rev_setup(r, &revs, prefix, "%s", "^%s", 1);
+	revs.first_parent_only = first_parent_only;
 	revs.limited = 1;
 
 	bisect_common(&revs);
 
-	find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr);
+	find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only);
 	revs.commits = managed_skipped(revs.commits, &tried);
 
 	if (!revs.commits) {
diff --git a/bisect.h b/bisect.h
index 8bad8d8391..a63af0505f 100644
--- a/bisect.h
+++ b/bisect.h
@@ -12,7 +12,7 @@ struct repository;
  * best commit, as chosen by `find_all`.
  */
 void find_bisection(struct commit_list **list, int *reaches, int *all,
-		    int find_all);
+		    int find_all, int first_parent_only);
 
 struct commit_list *filter_skipped(struct commit_list *list,
 				   struct commit_list **tried,
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index f520111eda..d1a14596b2 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -638,7 +638,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, revs.first_parent_only);
 
 		if (bisect_show_vars)
 			return show_bisect_vars(&info, reaches, all);
diff --git a/revision.c b/revision.c
index 6aa7f4f567..f0ad2603c1 100644
--- a/revision.c
+++ b/revision.c
@@ -2869,9 +2869,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->line_level_traverse &&
 	    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
 		die(_("-L does not yet support diff formats besides -p and -s"));
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 3dc1ad8f71..26e3533562 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -125,8 +125,8 @@ test_expect_success 'rev-list can negate index objects' '
 	test_cmp expect actual
 '
 
-test_expect_success '--bisect and --first-parent can not be combined' '
-	test_must_fail git rev-list --bisect --first-parent HEAD
+test_expect_success '--bisect and --first-parent can be combined' '
+	git rev-list --bisect --first-parent HEAD
 '
 
 test_expect_success '--header shows a NUL after each commit' '
diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
index a661408038..7fc0f75ca5 100755
--- a/t/t6002-rev-list-bisect.sh
+++ b/t/t6002-rev-list-bisect.sh
@@ -263,4 +263,49 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' '
 	test_cmp expect.sorted actual.sorted
 '
 
+test_output_expect_success "--bisect --first-parent" 'git rev-list --bisect --first-parent E ^F' <<EOF
+e4
+EOF
+
+test_output_expect_success "--first-parent" 'git rev-list --first-parent E ^F' <<EOF
+E
+e1
+e2
+e3
+e4
+e5
+e6
+e7
+e8
+EOF
+
+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.unsorted <<EOF &&
+$(git rev-parse E) (tag: E, dist=0)
+$(git rev-parse e1) (tag: e1, dist=1)
+$(git rev-parse e2) (tag: e2, dist=2)
+$(git rev-parse e3) (tag: e3, dist=3)
+$(git rev-parse e4) (tag: e4, dist=4)
+$(git rev-parse e5) (tag: e5, dist=4)
+$(git rev-parse e6) (tag: e6, dist=3)
+$(git rev-parse e7) (tag: e7, dist=2)
+$(git rev-parse e8) (tag: e8, dist=1)
+EOF
+
+# expect results to be ordered by distance (descending),
+# commit hash (ascending)
+sort -k4,4r -k1,1 expect.unsorted > expect &&
+git rev-list --bisect-all --first-parent E ^F > actual &&
+test_cmp actual expect
+'
+
 test_done
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 2/3] bisect: introduce first-parent flag
  2020-07-28 15:44 [PATCH 0/3] Introduce --first-parent flag for git bisect Aaron Lipman via GitGitGadget
  2020-07-28 15:44 ` [PATCH 1/3] rev-list: allow bisect and first-parent flags Aaron Lipman via GitGitGadget
@ 2020-07-28 15:44 ` Aaron Lipman via GitGitGadget
  2020-07-28 15:44 ` [PATCH 3/3] bisect: combine args passed to find_bisection() Aaron Lipman via GitGitGadget
  2020-07-30  0:27 ` [PATCH v2 0/3] Introduce --first-parent flag for git bisect Aaron Lipman
  3 siblings, 0 replies; 32+ messages in thread
From: Aaron Lipman via GitGitGadget @ 2020-07-28 15:44 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman, Aaron Lipman

From: Aaron Lipman <alipman88@gmail.com>

Upon seeing a merge commit when bisecting, this option may be used to
follow only the first parent.

In detecting regressions introduced through the merging of a branch, the
merge commit will be identified as introduction of the bug and its
ancestors will be ignored.

This option is particularly useful in avoiding false positives when a
merged branch contained broken or non-buildable commits, but the merge
itself was OK.

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 Documentation/git-bisect.txt | 13 ++++++++++++-
 bisect.c                     | 10 +++++++++-
 bisect.h                     |  2 ++
 builtin/bisect--helper.c     | 16 ++++++++++++----
 t/t6030-bisect-porcelain.sh  | 17 +++++++++++++++++
 5 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 7586c5a843..0e993e4587 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -17,7 +17,7 @@ The command takes various subcommands, and different options depending
 on the subcommand:
 
  git bisect start [--term-{old,good}=<term> --term-{new,bad}=<term>]
-		  [--no-checkout] [<bad> [<good>...]] [--] [<paths>...]
+		  [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]
  git bisect (bad|new|<term-new>) [<rev>]
  git bisect (good|old|<term-old>) [<rev>...]
  git bisect terms [--term-good | --term-bad]
@@ -365,6 +365,17 @@ does not require a checked out tree.
 +
 If the repository is bare, `--no-checkout` is assumed.
 
+--first-parent::
++
+Follow only the first parent commit upon seeing a merge commit.
++
+In detecting regressions introduced through the merging of a branch, the merge
+commit will be identified as introduction of the bug and its ancestors will be
+ignored.
++
+This option is particularly useful in avoiding false positives when a merged
+branch contained broken or non-buildable commits, but the merge itself was OK.
+
 EXAMPLES
 --------
 
diff --git a/bisect.c b/bisect.c
index 762f68c8e9..eee1f6f138 100644
--- a/bisect.c
+++ b/bisect.c
@@ -463,6 +463,7 @@ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
+static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
 static GIT_PATH_FUNC(git_path_head_name, "head-name")
 
 static void read_bisect_paths(struct argv_array *array)
@@ -984,6 +985,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
 	fclose(fp);
 }
 
+int read_first_parent_option(void)
+{
+	const char *filename = git_path_bisect_first_parent();
+	return !access(filename, F_OK);
+}
+
 /*
  * We use the convention that return BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND (-10) means
  * the bisection process finished successfully.
@@ -1000,7 +1007,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
 	enum bisect_error res = BISECT_OK;
 	struct object_id *bisect_rev;
 	char *steps_msg;
-	int first_parent_only = 0; /* TODO: pass --first-parent flag from git bisect start */
+	int first_parent_only = read_first_parent_option();
 
 	read_bisect_terms(&term_bad, &term_good);
 	if (read_bisect_refs())
@@ -1144,6 +1151,7 @@ int bisect_clean_state(void)
 	unlink_or_warn(git_path_bisect_names());
 	unlink_or_warn(git_path_bisect_run());
 	unlink_or_warn(git_path_bisect_terms());
+	unlink_or_warn(git_path_bisect_first_parent());
 	/* Cleanup head-name if it got left by an old version of git-bisect */
 	unlink_or_warn(git_path_head_name());
 	/*
diff --git a/bisect.h b/bisect.h
index a63af0505f..8ee80f5b48 100644
--- a/bisect.h
+++ b/bisect.h
@@ -66,6 +66,8 @@ int estimate_bisect_steps(int all);
 
 void read_bisect_terms(const char **bad, const char **good);
 
+int read_first_parent_option(void);
+
 int bisect_clean_state(void);
 
 #endif
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index ec4996282e..1236f5df1d 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -17,6 +17,7 @@ static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 static GIT_PATH_FUNC(git_path_head_name, "head-name")
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
+static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
@@ -28,7 +29,7 @@ static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --bisect-next-check <good_term> <bad_term> [<term>]"),
 	N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
 	N_("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]"
-					     "[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
+					    " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"),
 	NULL
 };
 
@@ -422,7 +423,7 @@ static int bisect_append_log_quoted(const char **argv)
 }
 
 static int bisect_start(struct bisect_terms *terms, int no_checkout,
-			const char **argv, int argc)
+			int first_parent_only, const char **argv, int argc)
 {
 	int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
 	int flags, pathspec_pos, res = 0;
@@ -453,6 +454,8 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 			break;
 		} else if (!strcmp(arg, "--no-checkout")) {
 			no_checkout = 1;
+		} else if (!strcmp(arg, "--first-parent")) {
+			first_parent_only = 1;
 		} else if (!strcmp(arg, "--term-good") ||
 			 !strcmp(arg, "--term-old")) {
 			i++;
@@ -577,6 +580,9 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 	 */
 	write_file(git_path_bisect_start(), "%s\n", start_head.buf);
 
+	if (first_parent_only)
+		write_file(git_path_bisect_first_parent(), "\n");
+
 	if (no_checkout) {
 		if (get_oid(start_head.buf, &oid) < 0) {
 			res = error(_("invalid ref: '%s'"), start_head.buf);
@@ -632,7 +638,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_TERMS,
 		BISECT_START
 	} cmdmode = 0;
-	int no_checkout = 0, res = 0, nolog = 0;
+	int no_checkout = 0, first_parent_only = 0, res = 0, nolog = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &cmdmode,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
@@ -656,6 +662,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("start the bisect session"), BISECT_START),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
+		OPT_BOOL(0, "first-parent", &first_parent_only,
+			 N_("only trace the first parent of merge commits")),
 		OPT_BOOL(0, "no-log", &nolog,
 			 N_("no log for BISECT_WRITE")),
 		OPT_END()
@@ -713,7 +721,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		break;
 	case BISECT_START:
 		set_terms(&terms, "bad", "good");
-		res = bisect_start(&terms, no_checkout, argv, argc);
+		res = bisect_start(&terms, no_checkout, first_parent_only, argv, argc);
 		break;
 	default:
 		return error("BUG: unknown subcommand '%d'", cmdmode);
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 36d9b2b2e4..7965698310 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -458,6 +458,23 @@ test_expect_success 'many merge bases creation' '
 	grep "$SIDE_HASH5" merge_bases.txt
 '
 
+# We want to automatically find the merge that
+# introduced "line" into hello.
+test_expect_success \
+    '"git bisect run --first-parent" simple case' \
+    'echo "#"\!"/bin/sh" > test_script.sh &&
+     echo "grep line hello > /dev/null" >> test_script.sh &&
+     echo "test \$? -ne 0" >> test_script.sh &&
+     chmod +x test_script.sh &&
+     git bisect start --first-parent &&
+     test_path_is_file ".git/BISECT_FIRST_PARENT" &&
+     git bisect good $HASH4 &&
+     git bisect bad $B_HASH &&
+     git bisect run ./test_script.sh > my_bisect_log.txt &&
+     grep "$B_HASH is the first bad commit" my_bisect_log.txt &&
+     git bisect reset &&
+     test_path_is_missing ".git/BISECT_FIRST_PARENT"'
+
 test_expect_success 'good merge bases when good and bad are siblings' '
 	git bisect start "$B_HASH" "$A_HASH" > my_bisect_log.txt &&
 	test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
-- 
gitgitgadget


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH 3/3] bisect: combine args passed to find_bisection()
  2020-07-28 15:44 [PATCH 0/3] Introduce --first-parent flag for git bisect Aaron Lipman via GitGitGadget
  2020-07-28 15:44 ` [PATCH 1/3] rev-list: allow bisect and first-parent flags Aaron Lipman via GitGitGadget
  2020-07-28 15:44 ` [PATCH 2/3] bisect: introduce first-parent flag Aaron Lipman via GitGitGadget
@ 2020-07-28 15:44 ` Aaron Lipman via GitGitGadget
  2020-07-30  0:27 ` [PATCH v2 0/3] Introduce --first-parent flag for git bisect Aaron Lipman
  3 siblings, 0 replies; 32+ messages in thread
From: Aaron Lipman via GitGitGadget @ 2020-07-28 15:44 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman, Aaron Lipman

From: Aaron Lipman <alipman88@gmail.com>

Now that find_bisection() accepts multiple boolean arguments, these may
be combined into a single unsigned integer in order to declutter some of
the code in bisect.c

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
Based-on-patch-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 bisect.c           | 43 +++++++++++++++++++++++++------------------
 bisect.h           |  5 ++++-
 builtin/rev-list.c |  9 ++++++++-
 3 files changed, 37 insertions(+), 20 deletions(-)

diff --git a/bisect.c b/bisect.c
index eee1f6f138..9599c58fd8 100644
--- a/bisect.c
+++ b/bisect.c
@@ -37,7 +37,7 @@ static const char *term_good;
  * We care just barely enough to avoid recursing for
  * non-merge entries.
  */
-static int count_distance(struct commit_list *entry, int first_parent_only)
+static int count_distance(struct commit_list *entry, unsigned bisect_flags)
 {
 	int nr = 0;
 
@@ -52,10 +52,10 @@ static int count_distance(struct commit_list *entry, int first_parent_only)
 		commit->object.flags |= COUNTED;
 		p = commit->parents;
 		entry = p;
-		if (p && !first_parent_only) {
+		if (p && !(bisect_flags & BISECT_FIRST_PARENT)) {
 			p = p->next;
 			while (p) {
-				nr += count_distance(p, first_parent_only);
+				nr += count_distance(p, bisect_flags);
 				p = p->next;
 			}
 		}
@@ -88,7 +88,7 @@ static inline void weight_set(struct commit_list *elem, int weight)
 	**commit_weight_at(&commit_weight, elem->item) = weight;
 }
 
-static int count_interesting_parents(struct commit *commit, int first_parent_only)
+static int count_interesting_parents(struct commit *commit, unsigned bisect_flags)
 {
 	struct commit_list *p;
 	int count;
@@ -96,7 +96,7 @@ static int count_interesting_parents(struct commit *commit, int first_parent_onl
 	for (count = 0, p = commit->parents; p; p = p->next) {
 		if (!(p->item->object.flags & UNINTERESTING))
 			count++;
-		if (first_parent_only)
+		if (bisect_flags & BISECT_FIRST_PARENT)
 			break;
 	}
 	return count;
@@ -260,7 +260,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 first_parent_only)
+					     unsigned bisect_flags)
 {
 	int n, counted;
 	struct commit_list *p;
@@ -272,7 +272,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 		unsigned flags = commit->object.flags;
 
 		*commit_weight_at(&commit_weight, p->item) = &weights[n++];
-		switch (count_interesting_parents(commit, first_parent_only)) {
+		switch (count_interesting_parents(commit, bisect_flags)) {
 		case 0:
 			if (!(flags & TREESAME)) {
 				weight_set(p, 1);
@@ -315,11 +315,11 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 			continue;
 		if (weight(p) != -2)
 			continue;
-		weight_set(p, count_distance(p, first_parent_only));
+		weight_set(p, count_distance(p, bisect_flags));
 		clear_distance(list);
 
 		/* Does it happen to be at exactly half-way? */
-		if (!find_all && halfway(p, nr))
+		if (!(bisect_flags & BISECT_FIND_ALL) && halfway(p, nr))
 			return p;
 		counted++;
 	}
@@ -342,7 +342,7 @@ 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) && 0 <= weight(q)) {
 					break;
-				} else if (first_parent_only) {
+				} else if (bisect_flags & BISECT_FIRST_PARENT) {
 					q = NULL;
 					break;
 				}
@@ -365,21 +365,21 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 				weight_set(p, weight(q));
 
 			/* Does it happen to be at exactly half-way? */
-			if (!find_all && halfway(p, nr))
+			if (!(bisect_flags & BISECT_FIND_ALL) && halfway(p, nr))
 				return p;
 		}
 	}
 
 	show_list("bisection 2 counted all", counted, nr, list);
 
-	if (!find_all)
+	if (!(bisect_flags & BISECT_FIND_ALL))
 		return best_bisection(list, nr);
 	else
 		return best_bisection_sorted(list, nr);
 }
 
 void find_bisection(struct commit_list **commit_list, int *reaches,
-		    int *all, int find_all, int first_parent_only)
+		    int *all, unsigned bisect_flags)
 {
 	int nr, on_list;
 	struct commit_list *list, *p, *best, *next, *last;
@@ -415,9 +415,9 @@ 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, first_parent_only);
+	best = do_find_bisection(list, nr, weights, bisect_flags);
 	if (best) {
-		if (!find_all) {
+		if (!(bisect_flags & BISECT_FIND_ALL)) {
 			list->item = best->item;
 			free_commit_list(list->next);
 			best = list;
@@ -1007,23 +1007,30 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
 	enum bisect_error res = BISECT_OK;
 	struct object_id *bisect_rev;
 	char *steps_msg;
-	int first_parent_only = read_first_parent_option();
+	unsigned bisect_flags = 0;
 
 	read_bisect_terms(&term_bad, &term_good);
 	if (read_bisect_refs())
 		die(_("reading bisect refs failed"));
 
+	if (read_first_parent_option())
+		bisect_flags |= BISECT_FIRST_PARENT;
+
+	if (!!skipped_revs.nr)
+		bisect_flags |= BISECT_FIND_ALL;
+
 	res = check_good_are_ancestors_of_bad(r, prefix, no_checkout);
 	if (res)
 		return res;
 
 	bisect_rev_setup(r, &revs, prefix, "%s", "^%s", 1);
-	revs.first_parent_only = first_parent_only;
+
+	revs.first_parent_only = !!(bisect_flags & BISECT_FIRST_PARENT);
 	revs.limited = 1;
 
 	bisect_common(&revs);
 
-	find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only);
+	find_bisection(&revs.commits, &reaches, &all, bisect_flags);
 	revs.commits = managed_skipped(revs.commits, &tried);
 
 	if (!revs.commits) {
diff --git a/bisect.h b/bisect.h
index 8ee80f5b48..402ddcfb5e 100644
--- a/bisect.h
+++ b/bisect.h
@@ -12,7 +12,7 @@ struct repository;
  * best commit, as chosen by `find_all`.
  */
 void find_bisection(struct commit_list **list, int *reaches, int *all,
-		    int find_all, int first_parent_only);
+		    unsigned bisect_flags);
 
 struct commit_list *filter_skipped(struct commit_list *list,
 				   struct commit_list **tried,
@@ -23,6 +23,9 @@ struct commit_list *filter_skipped(struct commit_list *list,
 #define BISECT_SHOW_ALL		(1<<0)
 #define REV_LIST_QUIET		(1<<1)
 
+#define BISECT_FIND_ALL		(1u<<0)
+#define BISECT_FIRST_PARENT	(1u<<1)
+
 struct rev_list_info {
 	struct rev_info *revs;
 	int flags;
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index d1a14596b2..1536ea6f28 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -637,8 +637,15 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 
 	if (bisect_list) {
 		int reaches, all;
+		unsigned bisect_flags = 0;
 
-		find_bisection(&revs.commits, &reaches, &all, bisect_find_all, revs.first_parent_only);
+		if (bisect_find_all)
+			bisect_flags |= BISECT_FIND_ALL;
+
+		if (revs.first_parent_only)
+			bisect_flags |= BISECT_FIRST_PARENT;
+
+		find_bisection(&revs.commits, &reaches, &all, bisect_flags);
 
 		if (bisect_show_vars)
 			return show_bisect_vars(&info, reaches, all);
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 1/3] rev-list: allow bisect and first-parent flags
  2020-07-28 15:44 ` [PATCH 1/3] rev-list: allow bisect and first-parent flags Aaron Lipman via GitGitGadget
@ 2020-07-28 20:23   ` Junio C Hamano
  2020-07-28 21:53     ` Junio C Hamano
  0 siblings, 1 reply; 32+ messages in thread
From: Junio C Hamano @ 2020-07-28 20:23 UTC (permalink / raw)
  To: Aaron Lipman via GitGitGadget; +Cc: git, Aaron Lipman

"Aaron Lipman via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Aaron Lipman <alipman88@gmail.com>
>
> Add first_parent_only parameter to find_bisection(), removing the
> barrier that prevented combining the --bisect and --first-parent flags
> when using git rev-list
>
> Signed-off-by: Aaron Lipman <alipman88@gmail.com>
> Based-on-patch-by: Tiago Botelho <tiagonbotelho@hotmail.com>

In chronological order, please.

> diff --git a/bisect.c b/bisect.c
> index d5e830410f..762f68c8e9 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -37,7 +37,7 @@ static const char *term_good;
>   * We care just barely enough to avoid recursing for
>   * non-merge entries.
>   */
> -static int count_distance(struct commit_list *entry)
> +static int count_distance(struct commit_list *entry, int first_parent_only)
>  {
>  	int nr = 0;
>  
> @@ -52,10 +52,10 @@ static int count_distance(struct commit_list *entry)
>  		commit->object.flags |= COUNTED;
>  		p = commit->parents;
>  		entry = p;
> -		if (p) {
> +		if (p && !first_parent_only) {
>  			p = p->next;
>  			while (p) {
> -				nr += count_distance(p);
> +				nr += count_distance(p, first_parent_only);
>  				p = p->next;
>  			}
>  		}

If you are running a first-parent-only bisection, by default, each
commit in the "goodness not yet known" region is treated as a single
parent commit for the purpose of bisection.  As do_find_bisection()
avoids the cost of calling stupid-and-expensive count_distance() by
treating such a single-strand-of-pearls specially, wouldn't it be a
BUG() if this funtion is called under first_parent_only mode in the
first place?

> @@ -88,15 +88,16 @@ static inline void weight_set(struct commit_list *elem, int weight)
>  	**commit_weight_at(&commit_weight, elem->item) = weight;
>  }


> -static int count_interesting_parents(struct commit *commit)
> +static int count_interesting_parents(struct commit *commit, int first_parent_only)
>  {
>  	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 (first_parent_only)
> +			break;
>  	}
>  	return count;
>  }

Any merge will be checked for interesting-ness of its first parent;
there is either zero or one interesting parent and no other case
under first-parent-only.  OK.

> @@ -259,7 +260,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 first_parent_only)
>  {
>  	int n, counted;
>  	struct commit_list *p;
> @@ -271,7 +272,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  		unsigned flags = commit->object.flags;
>  
>  		*commit_weight_at(&commit_weight, p->item) = &weights[n++];
> -		switch (count_interesting_parents(commit)) {
> +		switch (count_interesting_parents(commit, first_parent_only)) {
>  		case 0:
>  			if (!(flags & TREESAME)) {
>  				weight_set(p, 1);
> @@ -314,7 +315,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  			continue;
>  		if (weight(p) != -2)
>  			continue;

If count-interesting-parents can never be 2 or more, you will never
see weight(p) == -2 (see the switch statement in the previous hunk).

So it your first-parent-only mode allowed this continue not to
trigger, you have a BUG(), I think.

> -		weight_set(p, count_distance(p));
> +		weight_set(p, count_distance(p, first_parent_only));

Hence, you do not need to touch count_distance() at all, no?

> @@ -330,13 +331,21 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
>  			struct commit_list *q;
>  			unsigned flags = p->item->object.flags;
>  
> +			/* if commit p has already been weighted, continue. */
>  			if (0 <= weight(p))
>  				continue;
> +
> +			/*
> +			 * otherwise, find the first interesting
> +			 * already-weighted parent of p and store as q.
> +			 */

Unnecessary changes above.

>  			for (q = p->item->parents; q; q = q->next) {
> -				if (q->item->object.flags & UNINTERESTING)
> -					continue;
> -				if (0 <= weight(q))
> +				if (!(q->item->object.flags & UNINTERESTING) && 0 <= weight(q)) {
> +					break;

Overlong line.  Fold it after "&&", like this:

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

What is this change about?

Ah, if the first parent is uninteresting, we do not want to
continue, but with the original loop, we may go on and check
q->next.  We just want to break out of the loop instead.

I strongly suspect that

			for (q = p->item->parents;
			     q;
			     q = first_parent_only ? NULL : q->next) {

without any change inside the loop is much easier to reason about.
We follow exactly the same logic as the usual case.  We just pretend
that all commits have only one single parent which is the first one.

Thanks.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH 1/3] rev-list: allow bisect and first-parent flags
  2020-07-28 20:23   ` Junio C Hamano
@ 2020-07-28 21:53     ` Junio C Hamano
  0 siblings, 0 replies; 32+ messages in thread
From: Junio C Hamano @ 2020-07-28 21:53 UTC (permalink / raw)
  To: Aaron Lipman via GitGitGadget; +Cc: git, Aaron Lipman

Taking all the suggestions and speculations I made in the message I
am responding to, I think squashing the following changes into the
[PATCH 1/3] would make the code simpler and much easier to follow.

The first two hunks revert the changes made to count_distance(); the
third hunk makes sure that count_distance() is not called under the
first-parent mode, since the "single-strand-of-pearls" optimization
is the only counting method needed in the first-parent mode.  The
last hunk is to revert the changes to the single-strand-of-pearls
optimized counting and tweak the loop control to take only the first
parent into account.

Your new tests in 6000 and 6002 of course still pass with this
clean-up.

 bisect.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/bisect.c b/bisect.c
index 762f68c8e9..a11fdb1473 100644
--- a/bisect.c
+++ b/bisect.c
@@ -37,7 +37,7 @@ static const char *term_good;
  * We care just barely enough to avoid recursing for
  * non-merge entries.
  */
-static int count_distance(struct commit_list *entry, int first_parent_only)
+static int count_distance(struct commit_list *entry)
 {
 	int nr = 0;
 
@@ -52,10 +52,10 @@ static int count_distance(struct commit_list *entry, int first_parent_only)
 		commit->object.flags |= COUNTED;
 		p = commit->parents;
 		entry = p;
-		if (p && !first_parent_only) {
+		if (p) {
 			p = p->next;
 			while (p) {
-				nr += count_distance(p, first_parent_only);
+				nr += count_distance(p);
 				p = p->next;
 			}
 		}
@@ -315,7 +315,9 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 			continue;
 		if (weight(p) != -2)
 			continue;
-		weight_set(p, count_distance(p, first_parent_only));
+		if (first_parent_only)
+			BUG("shouldn't be calling count-distance in fp mode");
+		weight_set(p, count_distance(p));
 		clear_distance(list);
 
 		/* Does it happen to be at exactly half-way? */
@@ -331,21 +333,16 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 			struct commit_list *q;
 			unsigned flags = p->item->object.flags;
 
-			/* if commit p has already been weighted, continue. */
 			if (0 <= weight(p))
 				continue;
 
-			/*
-			 * otherwise, find the first interesting
-			 * already-weighted parent of p and store as q.
-			 */
-			for (q = p->item->parents; q; q = q->next) {
-				if (!(q->item->object.flags & UNINTERESTING) && 0 <= weight(q)) {
-					break;
-				} else if (first_parent_only) {
-					q = NULL;
+			for (q = p->item->parents;
+			     q;
+			     q = first_parent_only ? NULL : q->next) {
+				if (q->item->object.flags & UNINTERESTING)
+					continue;
+				if (0 <= weight(q))
 					break;
-				}
 			}
 			if (!q)
 				continue;



^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 0/3] Introduce --first-parent flag for git bisect
  2020-07-28 15:44 [PATCH 0/3] Introduce --first-parent flag for git bisect Aaron Lipman via GitGitGadget
                   ` (2 preceding siblings ...)
  2020-07-28 15:44 ` [PATCH 3/3] bisect: combine args passed to find_bisection() Aaron Lipman via GitGitGadget
@ 2020-07-30  0:27 ` Aaron Lipman
  2020-07-30  0:27   ` [PATCH v2 1/3] rev-list: allow bisect and first-parent flags Aaron Lipman
                     ` (4 more replies)
  3 siblings, 5 replies; 32+ messages in thread
From: Aaron Lipman @ 2020-07-30  0:27 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman

Junio, thank you for the feedback and the explanation regarding
count_distance(). I've gone ahead and implemented all your suggestions.

> In chronological order, please.
I submitted my initial patch through GitGitGadget. I don't see a way to
control the order GitGitGadget sends emails, so I'm switching to
git send-email for this iteration. Please continue to let me know if I'm
missing a convention.

Aaron Lipman (3):
  rev-list: allow bisect and first-parent flags
  bisect: introduce first-parent flag
  bisect: combine args passed to find_bisection()

 Documentation/git-bisect.txt       | 13 +++++++-
 Documentation/rev-list-options.txt |  7 ++--
 bisect.c                           | 51 ++++++++++++++++++++++--------
 bisect.h                           |  7 +++-
 builtin/bisect--helper.c           | 16 +++++++---
 builtin/rev-list.c                 |  9 +++++-
 revision.c                         |  3 --
 t/t6000-rev-list-misc.sh           |  4 +--
 t/t6002-rev-list-bisect.sh         | 45 ++++++++++++++++++++++++++
 t/t6030-bisect-porcelain.sh        | 17 ++++++++++
 10 files changed, 142 insertions(+), 30 deletions(-)

-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 1/3] rev-list: allow bisect and first-parent flags
  2020-07-30  0:27 ` [PATCH v2 0/3] Introduce --first-parent flag for git bisect Aaron Lipman
@ 2020-07-30  0:27   ` Aaron Lipman
  2020-07-30  0:27   ` [PATCH v2 2/3] bisect: introduce first-parent flag Aaron Lipman
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 32+ messages in thread
From: Aaron Lipman @ 2020-07-30  0:27 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman

Add first_parent_only parameter to find_bisection(), removing the
barrier that prevented combining the --bisect and --first-parent flags
when using git rev-list

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
Based-on-patch-by: Tiago Botelho <tiagonbotelho@hotmail.com>
---
 Documentation/rev-list-options.txt |  7 ++---
 bisect.c                           | 28 ++++++++++++-------
 bisect.h                           |  2 +-
 builtin/rev-list.c                 |  2 +-
 revision.c                         |  3 --
 t/t6000-rev-list-misc.sh           |  4 +--
 t/t6002-rev-list-bisect.sh         | 45 ++++++++++++++++++++++++++++++
 7 files changed, 70 insertions(+), 21 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index b01b2b6773..d3117ce51b 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -128,8 +128,7 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).
 	because merges into a topic branch tend to be only about
 	adjusting to updated upstream from time to time, and
 	this option allows you to ignore the individual commits
-	brought in to your history by such a merge. Cannot be
-	combined with --bisect.
+	brought in to your history by such a merge.
 
 --not::
 	Reverses the meaning of the '{caret}' prefix (or lack thereof)
@@ -207,7 +206,7 @@ ifndef::git-rev-list[]
 	Pretend as if the bad bisection ref `refs/bisect/bad`
 	was listed and as if it was followed by `--not` and the good
 	bisection refs `refs/bisect/good-*` on the command
-	line. Cannot be combined with --first-parent.
+	line.
 endif::git-rev-list[]
 
 --stdin::
@@ -743,7 +742,7 @@ outputs 'midpoint', the output of the two commands
 would be of roughly the same length.  Finding the change which
 introduces a regression is thus reduced to a binary search: repeatedly
 generate and test new 'midpoint's until the commit chain is of length
-one. Cannot be combined with --first-parent.
+one.
 
 --bisect-vars::
 	This calculates the same as `--bisect`, except that refs in
diff --git a/bisect.c b/bisect.c
index d5e830410f..a11fdb1473 100644
--- a/bisect.c
+++ b/bisect.c
@@ -88,15 +88,16 @@ static inline void weight_set(struct commit_list *elem, int weight)
 	**commit_weight_at(&commit_weight, elem->item) = weight;
 }
 
-static int count_interesting_parents(struct commit *commit)
+static int count_interesting_parents(struct commit *commit, int first_parent_only)
 {
 	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 (first_parent_only)
+			break;
 	}
 	return count;
 }
@@ -259,7 +260,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 first_parent_only)
 {
 	int n, counted;
 	struct commit_list *p;
@@ -271,7 +272,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 		unsigned flags = commit->object.flags;
 
 		*commit_weight_at(&commit_weight, p->item) = &weights[n++];
-		switch (count_interesting_parents(commit)) {
+		switch (count_interesting_parents(commit, first_parent_only)) {
 		case 0:
 			if (!(flags & TREESAME)) {
 				weight_set(p, 1);
@@ -314,6 +315,8 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 			continue;
 		if (weight(p) != -2)
 			continue;
+		if (first_parent_only)
+			BUG("shouldn't be calling count-distance in fp mode");
 		weight_set(p, count_distance(p));
 		clear_distance(list);
 
@@ -332,7 +335,10 @@ 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) {
+
+			for (q = p->item->parents;
+			     q;
+			     q = first_parent_only ? NULL : q->next) {
 				if (q->item->object.flags & UNINTERESTING)
 					continue;
 				if (0 <= weight(q))
@@ -370,7 +376,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 }
 
 void find_bisection(struct commit_list **commit_list, int *reaches,
-		    int *all, int find_all)
+		    int *all, int find_all, int first_parent_only)
 {
 	int nr, on_list;
 	struct commit_list *list, *p, *best, *next, *last;
@@ -406,7 +412,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, first_parent_only);
 	if (best) {
 		if (!find_all) {
 			list->item = best->item;
@@ -991,6 +997,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
 	enum bisect_error res = BISECT_OK;
 	struct object_id *bisect_rev;
 	char *steps_msg;
+	int first_parent_only = 0; /* TODO: pass --first-parent flag from git bisect start */
 
 	read_bisect_terms(&term_bad, &term_good);
 	if (read_bisect_refs())
@@ -1001,11 +1008,12 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
 		return res;
 
 	bisect_rev_setup(r, &revs, prefix, "%s", "^%s", 1);
+	revs.first_parent_only = first_parent_only;
 	revs.limited = 1;
 
 	bisect_common(&revs);
 
-	find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr);
+	find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only);
 	revs.commits = managed_skipped(revs.commits, &tried);
 
 	if (!revs.commits) {
diff --git a/bisect.h b/bisect.h
index 8bad8d8391..a63af0505f 100644
--- a/bisect.h
+++ b/bisect.h
@@ -12,7 +12,7 @@ struct repository;
  * best commit, as chosen by `find_all`.
  */
 void find_bisection(struct commit_list **list, int *reaches, int *all,
-		    int find_all);
+		    int find_all, int first_parent_only);
 
 struct commit_list *filter_skipped(struct commit_list *list,
 				   struct commit_list **tried,
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index f520111eda..d1a14596b2 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -638,7 +638,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, revs.first_parent_only);
 
 		if (bisect_show_vars)
 			return show_bisect_vars(&info, reaches, all);
diff --git a/revision.c b/revision.c
index 6aa7f4f567..f0ad2603c1 100644
--- a/revision.c
+++ b/revision.c
@@ -2869,9 +2869,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->line_level_traverse &&
 	    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
 		die(_("-L does not yet support diff formats besides -p and -s"));
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 3dc1ad8f71..26e3533562 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -125,8 +125,8 @@ test_expect_success 'rev-list can negate index objects' '
 	test_cmp expect actual
 '
 
-test_expect_success '--bisect and --first-parent can not be combined' '
-	test_must_fail git rev-list --bisect --first-parent HEAD
+test_expect_success '--bisect and --first-parent can be combined' '
+	git rev-list --bisect --first-parent HEAD
 '
 
 test_expect_success '--header shows a NUL after each commit' '
diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
index a661408038..7fc0f75ca5 100755
--- a/t/t6002-rev-list-bisect.sh
+++ b/t/t6002-rev-list-bisect.sh
@@ -263,4 +263,49 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' '
 	test_cmp expect.sorted actual.sorted
 '
 
+test_output_expect_success "--bisect --first-parent" 'git rev-list --bisect --first-parent E ^F' <<EOF
+e4
+EOF
+
+test_output_expect_success "--first-parent" 'git rev-list --first-parent E ^F' <<EOF
+E
+e1
+e2
+e3
+e4
+e5
+e6
+e7
+e8
+EOF
+
+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.unsorted <<EOF &&
+$(git rev-parse E) (tag: E, dist=0)
+$(git rev-parse e1) (tag: e1, dist=1)
+$(git rev-parse e2) (tag: e2, dist=2)
+$(git rev-parse e3) (tag: e3, dist=3)
+$(git rev-parse e4) (tag: e4, dist=4)
+$(git rev-parse e5) (tag: e5, dist=4)
+$(git rev-parse e6) (tag: e6, dist=3)
+$(git rev-parse e7) (tag: e7, dist=2)
+$(git rev-parse e8) (tag: e8, dist=1)
+EOF
+
+# expect results to be ordered by distance (descending),
+# commit hash (ascending)
+sort -k4,4r -k1,1 expect.unsorted > expect &&
+git rev-list --bisect-all --first-parent E ^F > actual &&
+test_cmp actual expect
+'
+
 test_done
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 2/3] bisect: introduce first-parent flag
  2020-07-30  0:27 ` [PATCH v2 0/3] Introduce --first-parent flag for git bisect Aaron Lipman
  2020-07-30  0:27   ` [PATCH v2 1/3] rev-list: allow bisect and first-parent flags Aaron Lipman
@ 2020-07-30  0:27   ` Aaron Lipman
  2020-07-30  4:17     ` Junio C Hamano
  2020-07-30  0:27   ` [PATCH v2 3/3] bisect: combine args passed to find_bisection() Aaron Lipman
                     ` (2 subsequent siblings)
  4 siblings, 1 reply; 32+ messages in thread
From: Aaron Lipman @ 2020-07-30  0:27 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman

Upon seeing a merge commit when bisecting, this option may be used to
follow only the first parent.

In detecting regressions introduced through the merging of a branch, the
merge commit will be identified as introduction of the bug and its
ancestors will be ignored.

This option is particularly useful in avoiding false positives when a
merged branch contained broken or non-buildable commits, but the merge
itself was OK.

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 Documentation/git-bisect.txt | 13 ++++++++++++-
 bisect.c                     | 10 +++++++++-
 bisect.h                     |  2 ++
 builtin/bisect--helper.c     | 16 ++++++++++++----
 t/t6030-bisect-porcelain.sh  | 17 +++++++++++++++++
 5 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 7586c5a843..0e993e4587 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -17,7 +17,7 @@ The command takes various subcommands, and different options depending
 on the subcommand:
 
  git bisect start [--term-{old,good}=<term> --term-{new,bad}=<term>]
-		  [--no-checkout] [<bad> [<good>...]] [--] [<paths>...]
+		  [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]
  git bisect (bad|new|<term-new>) [<rev>]
  git bisect (good|old|<term-old>) [<rev>...]
  git bisect terms [--term-good | --term-bad]
@@ -365,6 +365,17 @@ does not require a checked out tree.
 +
 If the repository is bare, `--no-checkout` is assumed.
 
+--first-parent::
++
+Follow only the first parent commit upon seeing a merge commit.
++
+In detecting regressions introduced through the merging of a branch, the merge
+commit will be identified as introduction of the bug and its ancestors will be
+ignored.
++
+This option is particularly useful in avoiding false positives when a merged
+branch contained broken or non-buildable commits, but the merge itself was OK.
+
 EXAMPLES
 --------
 
diff --git a/bisect.c b/bisect.c
index a11fdb1473..b495a19192 100644
--- a/bisect.c
+++ b/bisect.c
@@ -460,6 +460,7 @@ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
+static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
 static GIT_PATH_FUNC(git_path_head_name, "head-name")
 
 static void read_bisect_paths(struct argv_array *array)
@@ -981,6 +982,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
 	fclose(fp);
 }
 
+int read_first_parent_option(void)
+{
+	const char *filename = git_path_bisect_first_parent();
+	return !access(filename, F_OK);
+}
+
 /*
  * We use the convention that return BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND (-10) means
  * the bisection process finished successfully.
@@ -997,7 +1004,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
 	enum bisect_error res = BISECT_OK;
 	struct object_id *bisect_rev;
 	char *steps_msg;
-	int first_parent_only = 0; /* TODO: pass --first-parent flag from git bisect start */
+	int first_parent_only = read_first_parent_option();
 
 	read_bisect_terms(&term_bad, &term_good);
 	if (read_bisect_refs())
@@ -1141,6 +1148,7 @@ int bisect_clean_state(void)
 	unlink_or_warn(git_path_bisect_names());
 	unlink_or_warn(git_path_bisect_run());
 	unlink_or_warn(git_path_bisect_terms());
+	unlink_or_warn(git_path_bisect_first_parent());
 	/* Cleanup head-name if it got left by an old version of git-bisect */
 	unlink_or_warn(git_path_head_name());
 	/*
diff --git a/bisect.h b/bisect.h
index a63af0505f..8ee80f5b48 100644
--- a/bisect.h
+++ b/bisect.h
@@ -66,6 +66,8 @@ int estimate_bisect_steps(int all);
 
 void read_bisect_terms(const char **bad, const char **good);
 
+int read_first_parent_option(void);
+
 int bisect_clean_state(void);
 
 #endif
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index ec4996282e..1236f5df1d 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -17,6 +17,7 @@ static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 static GIT_PATH_FUNC(git_path_head_name, "head-name")
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
+static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
@@ -28,7 +29,7 @@ static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --bisect-next-check <good_term> <bad_term> [<term>]"),
 	N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
 	N_("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]"
-					     "[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
+					    " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"),
 	NULL
 };
 
@@ -422,7 +423,7 @@ static int bisect_append_log_quoted(const char **argv)
 }
 
 static int bisect_start(struct bisect_terms *terms, int no_checkout,
-			const char **argv, int argc)
+			int first_parent_only, const char **argv, int argc)
 {
 	int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
 	int flags, pathspec_pos, res = 0;
@@ -453,6 +454,8 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 			break;
 		} else if (!strcmp(arg, "--no-checkout")) {
 			no_checkout = 1;
+		} else if (!strcmp(arg, "--first-parent")) {
+			first_parent_only = 1;
 		} else if (!strcmp(arg, "--term-good") ||
 			 !strcmp(arg, "--term-old")) {
 			i++;
@@ -577,6 +580,9 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 	 */
 	write_file(git_path_bisect_start(), "%s\n", start_head.buf);
 
+	if (first_parent_only)
+		write_file(git_path_bisect_first_parent(), "\n");
+
 	if (no_checkout) {
 		if (get_oid(start_head.buf, &oid) < 0) {
 			res = error(_("invalid ref: '%s'"), start_head.buf);
@@ -632,7 +638,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_TERMS,
 		BISECT_START
 	} cmdmode = 0;
-	int no_checkout = 0, res = 0, nolog = 0;
+	int no_checkout = 0, first_parent_only = 0, res = 0, nolog = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &cmdmode,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
@@ -656,6 +662,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("start the bisect session"), BISECT_START),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
+		OPT_BOOL(0, "first-parent", &first_parent_only,
+			 N_("only trace the first parent of merge commits")),
 		OPT_BOOL(0, "no-log", &nolog,
 			 N_("no log for BISECT_WRITE")),
 		OPT_END()
@@ -713,7 +721,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		break;
 	case BISECT_START:
 		set_terms(&terms, "bad", "good");
-		res = bisect_start(&terms, no_checkout, argv, argc);
+		res = bisect_start(&terms, no_checkout, first_parent_only, argv, argc);
 		break;
 	default:
 		return error("BUG: unknown subcommand '%d'", cmdmode);
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 36d9b2b2e4..7965698310 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -458,6 +458,23 @@ test_expect_success 'many merge bases creation' '
 	grep "$SIDE_HASH5" merge_bases.txt
 '
 
+# We want to automatically find the merge that
+# introduced "line" into hello.
+test_expect_success \
+    '"git bisect run --first-parent" simple case' \
+    'echo "#"\!"/bin/sh" > test_script.sh &&
+     echo "grep line hello > /dev/null" >> test_script.sh &&
+     echo "test \$? -ne 0" >> test_script.sh &&
+     chmod +x test_script.sh &&
+     git bisect start --first-parent &&
+     test_path_is_file ".git/BISECT_FIRST_PARENT" &&
+     git bisect good $HASH4 &&
+     git bisect bad $B_HASH &&
+     git bisect run ./test_script.sh > my_bisect_log.txt &&
+     grep "$B_HASH is the first bad commit" my_bisect_log.txt &&
+     git bisect reset &&
+     test_path_is_missing ".git/BISECT_FIRST_PARENT"'
+
 test_expect_success 'good merge bases when good and bad are siblings' '
 	git bisect start "$B_HASH" "$A_HASH" > my_bisect_log.txt &&
 	test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v2 3/3] bisect: combine args passed to find_bisection()
  2020-07-30  0:27 ` [PATCH v2 0/3] Introduce --first-parent flag for git bisect Aaron Lipman
  2020-07-30  0:27   ` [PATCH v2 1/3] rev-list: allow bisect and first-parent flags Aaron Lipman
  2020-07-30  0:27   ` [PATCH v2 2/3] bisect: introduce first-parent flag Aaron Lipman
@ 2020-07-30  0:27   ` Aaron Lipman
  2020-07-30  4:32     ` Junio C Hamano
  2020-07-30  0:48   ` [PATCH v2 0/3] Introduce --first-parent flag for git bisect Junio C Hamano
  2020-08-01 17:58   ` [PATCH v3 0/4] " Aaron Lipman
  4 siblings, 1 reply; 32+ messages in thread
From: Aaron Lipman @ 2020-07-30  0:27 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman

Now that find_bisection() accepts multiple boolean arguments, these may
be combined into a single unsigned integer in order to declutter some of
the code in bisect.c

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
Based-on-patch-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 bisect.c           | 37 ++++++++++++++++++++++---------------
 bisect.h           |  5 ++++-
 builtin/rev-list.c |  9 ++++++++-
 3 files changed, 34 insertions(+), 17 deletions(-)

diff --git a/bisect.c b/bisect.c
index b495a19192..07a7760a72 100644
--- a/bisect.c
+++ b/bisect.c
@@ -88,7 +88,7 @@ static inline void weight_set(struct commit_list *elem, int weight)
 	**commit_weight_at(&commit_weight, elem->item) = weight;
 }
 
-static int count_interesting_parents(struct commit *commit, int first_parent_only)
+static int count_interesting_parents(struct commit *commit, unsigned bisect_flags)
 {
 	struct commit_list *p;
 	int count;
@@ -96,7 +96,7 @@ static int count_interesting_parents(struct commit *commit, int first_parent_onl
 	for (count = 0, p = commit->parents; p; p = p->next) {
 		if (!(p->item->object.flags & UNINTERESTING))
 			count++;
-		if (first_parent_only)
+		if (bisect_flags & BISECT_FIRST_PARENT)
 			break;
 	}
 	return count;
@@ -260,7 +260,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 first_parent_only)
+					     unsigned bisect_flags)
 {
 	int n, counted;
 	struct commit_list *p;
@@ -272,7 +272,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 		unsigned flags = commit->object.flags;
 
 		*commit_weight_at(&commit_weight, p->item) = &weights[n++];
-		switch (count_interesting_parents(commit, first_parent_only)) {
+		switch (count_interesting_parents(commit, bisect_flags)) {
 		case 0:
 			if (!(flags & TREESAME)) {
 				weight_set(p, 1);
@@ -315,13 +315,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 			continue;
 		if (weight(p) != -2)
 			continue;
-		if (first_parent_only)
+		if (bisect_flags & BISECT_FIRST_PARENT)
 			BUG("shouldn't be calling count-distance in fp mode");
 		weight_set(p, count_distance(p));
 		clear_distance(list);
 
 		/* Does it happen to be at exactly half-way? */
-		if (!find_all && halfway(p, nr))
+		if (!(bisect_flags & BISECT_FIND_ALL) && halfway(p, nr))
 			return p;
 		counted++;
 	}
@@ -338,7 +338,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 
 			for (q = p->item->parents;
 			     q;
-			     q = first_parent_only ? NULL : q->next) {
+			     q = bisect_flags & BISECT_FIRST_PARENT ? NULL : q->next) {
 				if (q->item->object.flags & UNINTERESTING)
 					continue;
 				if (0 <= weight(q))
@@ -362,21 +362,21 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 				weight_set(p, weight(q));
 
 			/* Does it happen to be at exactly half-way? */
-			if (!find_all && halfway(p, nr))
+			if (!(bisect_flags & BISECT_FIND_ALL) && halfway(p, nr))
 				return p;
 		}
 	}
 
 	show_list("bisection 2 counted all", counted, nr, list);
 
-	if (!find_all)
+	if (!(bisect_flags & BISECT_FIND_ALL))
 		return best_bisection(list, nr);
 	else
 		return best_bisection_sorted(list, nr);
 }
 
 void find_bisection(struct commit_list **commit_list, int *reaches,
-		    int *all, int find_all, int first_parent_only)
+		    int *all, unsigned bisect_flags)
 {
 	int nr, on_list;
 	struct commit_list *list, *p, *best, *next, *last;
@@ -412,9 +412,9 @@ 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, first_parent_only);
+	best = do_find_bisection(list, nr, weights, bisect_flags);
 	if (best) {
-		if (!find_all) {
+		if (!(bisect_flags & BISECT_FIND_ALL)) {
 			list->item = best->item;
 			free_commit_list(list->next);
 			best = list;
@@ -1004,23 +1004,30 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
 	enum bisect_error res = BISECT_OK;
 	struct object_id *bisect_rev;
 	char *steps_msg;
-	int first_parent_only = read_first_parent_option();
+	unsigned bisect_flags = 0;
 
 	read_bisect_terms(&term_bad, &term_good);
 	if (read_bisect_refs())
 		die(_("reading bisect refs failed"));
 
+	if (read_first_parent_option())
+		bisect_flags |= BISECT_FIRST_PARENT;
+
+	if (!!skipped_revs.nr)
+		bisect_flags |= BISECT_FIND_ALL;
+
 	res = check_good_are_ancestors_of_bad(r, prefix, no_checkout);
 	if (res)
 		return res;
 
 	bisect_rev_setup(r, &revs, prefix, "%s", "^%s", 1);
-	revs.first_parent_only = first_parent_only;
+
+	revs.first_parent_only = !!(bisect_flags & BISECT_FIRST_PARENT);
 	revs.limited = 1;
 
 	bisect_common(&revs);
 
-	find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only);
+	find_bisection(&revs.commits, &reaches, &all, bisect_flags);
 	revs.commits = managed_skipped(revs.commits, &tried);
 
 	if (!revs.commits) {
diff --git a/bisect.h b/bisect.h
index 8ee80f5b48..402ddcfb5e 100644
--- a/bisect.h
+++ b/bisect.h
@@ -12,7 +12,7 @@ struct repository;
  * best commit, as chosen by `find_all`.
  */
 void find_bisection(struct commit_list **list, int *reaches, int *all,
-		    int find_all, int first_parent_only);
+		    unsigned bisect_flags);
 
 struct commit_list *filter_skipped(struct commit_list *list,
 				   struct commit_list **tried,
@@ -23,6 +23,9 @@ struct commit_list *filter_skipped(struct commit_list *list,
 #define BISECT_SHOW_ALL		(1<<0)
 #define REV_LIST_QUIET		(1<<1)
 
+#define BISECT_FIND_ALL		(1u<<0)
+#define BISECT_FIRST_PARENT	(1u<<1)
+
 struct rev_list_info {
 	struct rev_info *revs;
 	int flags;
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index d1a14596b2..1536ea6f28 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -637,8 +637,15 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 
 	if (bisect_list) {
 		int reaches, all;
+		unsigned bisect_flags = 0;
 
-		find_bisection(&revs.commits, &reaches, &all, bisect_find_all, revs.first_parent_only);
+		if (bisect_find_all)
+			bisect_flags |= BISECT_FIND_ALL;
+
+		if (revs.first_parent_only)
+			bisect_flags |= BISECT_FIRST_PARENT;
+
+		find_bisection(&revs.commits, &reaches, &all, bisect_flags);
 
 		if (bisect_show_vars)
 			return show_bisect_vars(&info, reaches, all);
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 0/3] Introduce --first-parent flag for git bisect
  2020-07-30  0:27 ` [PATCH v2 0/3] Introduce --first-parent flag for git bisect Aaron Lipman
                     ` (2 preceding siblings ...)
  2020-07-30  0:27   ` [PATCH v2 3/3] bisect: combine args passed to find_bisection() Aaron Lipman
@ 2020-07-30  0:48   ` Junio C Hamano
  2020-08-01 17:58   ` [PATCH v3 0/4] " Aaron Lipman
  4 siblings, 0 replies; 32+ messages in thread
From: Junio C Hamano @ 2020-07-30  0:48 UTC (permalink / raw)
  To: Aaron Lipman; +Cc: git

Aaron Lipman <alipman88@gmail.com> writes:

> Junio, thank you for the feedback and the explanation regarding
> count_distance(). I've gone ahead and implemented all your suggestions.
>
>> In chronological order, please.
> I submitted my initial patch through GitGitGadget. I don't see a way to
> control the order GitGitGadget sends emails,...

Ah, I was referring to the order of sign-off, helped-by, etc.
Somebody writes a patch that is left on the list, you pick it up and
polish it for perfection, and send the result to the list.  

So based-on would come first because it happened first, and then you
worked further until the result deserves to be signed-off and
finally you sent it out.  So your sign-off comes the last.


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 2/3] bisect: introduce first-parent flag
  2020-07-30  0:27   ` [PATCH v2 2/3] bisect: introduce first-parent flag Aaron Lipman
@ 2020-07-30  4:17     ` Junio C Hamano
  0 siblings, 0 replies; 32+ messages in thread
From: Junio C Hamano @ 2020-07-30  4:17 UTC (permalink / raw)
  To: Aaron Lipman; +Cc: git

Aaron Lipman <alipman88@gmail.com> writes:

> diff --git a/bisect.c b/bisect.c
> index a11fdb1473..b495a19192 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -460,6 +460,7 @@ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
>  static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
>  static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
>  static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
> +static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
>  static GIT_PATH_FUNC(git_path_head_name, "head-name")

This is not a new issue, but duplication of the above and the same
set of PATH_FUNC in builtin/bisect-helper.c looks ugly.  We may want
to do something about it after this topic is done.

> @@ -981,6 +982,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
>  	fclose(fp);
>  }
>  
> +int read_first_parent_option(void)

"static int", no?  I do not think we need to be able to call this
from anywhere outside this file.

> +{
> +	const char *filename = git_path_bisect_first_parent();
> +	return !access(filename, F_OK);
> +}
> +
>  /*
>   * We use the convention that return BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND (-10) means
>   * the bisection process finished successfully.
> @@ -997,7 +1004,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
>  	enum bisect_error res = BISECT_OK;
>  	struct object_id *bisect_rev;
>  	char *steps_msg;
> -	int first_parent_only = 0; /* TODO: pass --first-parent flag from git bisect start */
> +	int first_parent_only = read_first_parent_option();
>  
>  	read_bisect_terms(&term_bad, &term_good);
>  	if (read_bisect_refs())
> @@ -1141,6 +1148,7 @@ int bisect_clean_state(void)
>  	unlink_or_warn(git_path_bisect_names());
>  	unlink_or_warn(git_path_bisect_run());
>  	unlink_or_warn(git_path_bisect_terms());
> +	unlink_or_warn(git_path_bisect_first_parent());
>  	/* Cleanup head-name if it got left by an old version of git-bisect */
>  	unlink_or_warn(git_path_head_name());
>  	/*
> diff --git a/bisect.h b/bisect.h
> index a63af0505f..8ee80f5b48 100644
> --- a/bisect.h
> +++ b/bisect.h
> @@ -66,6 +66,8 @@ int estimate_bisect_steps(int all);
>  
>  void read_bisect_terms(const char **bad, const char **good);
>  
> +int read_first_parent_option(void);
> +

So this won't be needed, right?

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index ec4996282e..1236f5df1d 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -422,7 +423,7 @@ static int bisect_append_log_quoted(const char **argv)
>  }
>  
>  static int bisect_start(struct bisect_terms *terms, int no_checkout,
> -			const char **argv, int argc)
> +			int first_parent_only, const char **argv, int argc)

Why do we need to pass this new parameter from cmd_bisect__helper(),
when we are going to parse it out of argv/argc outselves anyway?

I suspect that you would ask the same question to whoever added
no_checkout to this thing, and I wouldn't be surprised if we end up
removing both of these parameters (and parsing of the options inside
cmd_bisect__helper()) after thinking about them, but anyway, let's
read on.

> +# We want to automatically find the merge that
> +# introduced "line" into hello.

'introduced'?  

> +test_expect_success \
> +    '"git bisect run --first-parent" simple case' \

Let's not revert back to ancient line-folding style.  The next test
you can see in the postimage, i.e.

	test_expect_success 'title in single quote' '
		the first command &&
		the second command &&
		...
		the last command
	'

is what we want to mimic.

> +    'echo "#"\!"/bin/sh" > test_script.sh &&
> +     echo "grep line hello > /dev/null" >> test_script.sh &&
> +     echo "test \$? -ne 0" >> test_script.sh &&
> +     chmod +x test_script.sh &&

So, we want to say "bad" if $? is 0, i.e. the file 'hello' has a
string "line" in it and "good" if $? is not 0, i.e. the file 'hello'
does not have such a line?

A few things:

 - Use "write_script" to abstract away platform-specific details
   such as which $SHELL_PATH to use on "#!" line, and "chmod +x".

 - There is no SP between a redirection operator and its target
   file.

i.e. Ssomething like this instead of the above four lines:

	write_script test_script.sh <<-\EOF
	! grep line hello >/dev/null
	EOF

> +     git bisect start --first-parent &&
> +     test_path_is_file ".git/BISECT_FIRST_PARENT" &&
> +     git bisect good $HASH4 &&
> +     git bisect bad $B_HASH &&
> +     git bisect run ./test_script.sh > my_bisect_log.txt &&

Style: no SP between '>' and its target file.

> +     grep "$B_HASH is the first bad commit" my_bisect_log.txt &&
> +     git bisect reset &&
> +     test_path_is_missing ".git/BISECT_FIRST_PARENT"'

The final blame must lie on a commit on the first-parent chain,
which this test tries to ensure, but I wonder if it is also required
that all commits offered to be tested by "git bisect" are on the
first-parent chain, and if so, shouldn't we be make sure each and
every time "test_script" is run, the commit that is at HEAD is on
the first parent chain between the initial good..bad range?

>  test_expect_success 'good merge bases when good and bad are siblings' '
>  	git bisect start "$B_HASH" "$A_HASH" > my_bisect_log.txt &&
>  	test_i18ngrep "merge base must be tested" my_bisect_log.txt &&

Other than that, overall this seems to be going in the right
direction.

Thanks.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v2 3/3] bisect: combine args passed to find_bisection()
  2020-07-30  0:27   ` [PATCH v2 3/3] bisect: combine args passed to find_bisection() Aaron Lipman
@ 2020-07-30  4:32     ` Junio C Hamano
  0 siblings, 0 replies; 32+ messages in thread
From: Junio C Hamano @ 2020-07-30  4:32 UTC (permalink / raw)
  To: Aaron Lipman; +Cc: git

Aaron Lipman <alipman88@gmail.com> writes:

> Now that find_bisection() accepts multiple boolean arguments, these may
> be combined into a single unsigned integer in order to declutter some of
> the code in bisect.c

I'd rather call them "flags" without "bisect_".  If we are passing
two sets of flags, one set about "bisect" and the other set about
something else, it would make quite a lot of sense to call the first
set "bisect_flags", but what we are seeing here is not like that.

> +	if (read_first_parent_option())
> +		bisect_flags |= BISECT_FIRST_PARENT;
> +
> +	if (!!skipped_revs.nr)
> +		bisect_flags |= BISECT_FIND_ALL;
> +

You do not care what kind of "true" you are feeding "if()", so I do
not think you would want to keep !! prefix here.  Older API into
find_bisection() may wanted to say "pass 1 to find_all", in which
case, normalizing with !! prefix may have made perfect sense, but 
that no longer holds here.

> +
> +	revs.first_parent_only = !!(bisect_flags & BISECT_FIRST_PARENT);

On the other hand, this !! may make sense; especially since .first_parent_only
could just be a one-bit unsigned bitfield.

>  	revs.limited = 1;
>  
>  	bisect_common(&revs);
>  
> -	find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only);
> +	find_bisection(&revs.commits, &reaches, &all, bisect_flags);

> @@ -23,6 +23,9 @@ struct commit_list *filter_skipped(struct commit_list *list,
>  #define BISECT_SHOW_ALL		(1<<0)
>  #define REV_LIST_QUIET		(1<<1)
>  
> +#define BISECT_FIND_ALL		(1u<<0)
> +#define BISECT_FIRST_PARENT	(1u<<1)

The set of bits to go to your "bisect_flags" are only these two new
ones, and the existing BISECT_SHOW_ALL does not belong to it (it is
a bit in rev_list_info.flags), but it is not apparent.  I wonder if
there is a good way to help readers easily tell these two sets apart.
These are flags passed to find_bisection(), so perhaps

    #define FIND_BISECTION_ALL (1U<<0)
    #define FIND_BISECTION_FIRST_PARENT_ONLY (1U<<1)

    unsigned flags = 0;
    if (first_parent)
	flags |= FIND_BISECTION_FIRST_PARENT_ONLY;
    ...
    find_bisection(..., flags);

would be a reasonable compromise?

Thanks.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v3 0/4] Introduce --first-parent flag for git bisect
  2020-07-30  0:27 ` [PATCH v2 0/3] Introduce --first-parent flag for git bisect Aaron Lipman
                     ` (3 preceding siblings ...)
  2020-07-30  0:48   ` [PATCH v2 0/3] Introduce --first-parent flag for git bisect Junio C Hamano
@ 2020-08-01 17:58   ` Aaron Lipman
  2020-08-01 17:58     ` [PATCH v3 1/4] rev-list: allow bisect and first-parent flags Aaron Lipman
                       ` (5 more replies)
  4 siblings, 6 replies; 32+ messages in thread
From: Aaron Lipman @ 2020-08-01 17:58 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman

> Ah, I was referring to the order of sign-off, helped-by, etc.

Whoops, re-ordered correctly.

> This is not a new issue, but duplication of the above and the same
> set of PATH_FUNC in builtin/bisect-helper.c looks ugly.  We may want
> to do something about it after this topic is done.

Happy to pick that up next!

> > +int read_first_parent_option(void)

> "static int", no?  I do not think we need to be able to call this
> from anywhere outside this file.

Added static keyword and removed the redundant line from bisect.h

> >  static int bisect_start(struct bisect_terms *terms, int no_checkout,
> > -     const char **argv, int argc)
> > +     int first_parent_only, const char **argv, int argc)

> Why do we need to pass this new parameter from cmd_bisect__helper(),
> when we are going to parse it out of argv/argc outselves anyway?

> I suspect that you would ask the same question to whoever added
> no_checkout to this thing, and I wouldn't be surprised if we end up
> removing both of these parameters (and parsing of the options inside
> cmd_bisect__helper()) after thinking about them, but anyway, let's
> read on.

Hmm. Is there a preferred way to to add a --first-parent line to the
output of "git bisect start --help" via the parse-options API without
removing the --first-parent option from argv in the process?

As long as we're capturing the --first-parent option in
cmd_bisect__helper(), checking argv for a --first-parent flag in
bisect_start() is pointless. So I've deleted the following lines from my
patch inside bisect_start() (for the time being, at least):

git diff v2 v3 -- builtin/bisect--helper.c
-   } else if (!strcmp(arg, "--first-parent")) {
-     first_parent_only = 1;

> > +# We want to automatically find the merge that
> > +# introduced "line" into hello.

> 'introduced'?

That's the language used by other "git bisect run" tests. If
"introduced" sounds too clinical, perhaps "added" instead?

> Let's not revert back to ancient line-folding style.

Thank you for the template. I've updated my test accordingly.

> So, we want to say "bad" if $? is 0, i.e. the file 'hello' has a
> string "line" in it and "good" if $? is not 0, i.e. the file 'hello'
> does not have such a line?

I think the other way around - an exit code of 0 from the script passed
to "git bisect run" marks a commit as good, 1 as bad.

>  - Use "write_script" to abstract away platform-specific details
>    such as which $SHELL_PATH to use on "#!" line, and "chmod +x".

>  - There is no SP between a redirection operator and its target
>    file.

Noted. Can we rewrite the other "git bisect run" tests in
t6030-bisect-porcelain.sh so that they're all consisent? That way, when
someone adds another test for "git bisect run", they'll have a few more
proper examples. (I've added a fourth commit that does this, if that's
OK.)

> The final blame must lie on a commit on the first-parent chain,
> which this test tries to ensure, but I wonder if it is also required
> that all commits offered to be tested by "git bisect" are on the
> first-parent chain, and if so, shouldn't we be make sure each and
> every time "test_script" is run, the commit that is at HEAD is on
> the first parent chain between the initial good..bad range?

That is prudent. I've altered test_script.sh to use the special -1 exit
code to abort the bisection process if it encounters a commit outside
the first parent chain.

Althernatively, if we prefer not to depend on the special -1 exit code,
we can append any tested commits outside the first parent chain to a
file and ensure that it's empty after "git bisect run" finishes.

> I'd rather call them "flags" without "bisect_".  If we are passing
> two sets of flags, one set about "bisect" and the other set about
> something else, it would make quite a lot of sense to call the first
> set "bisect_flags", but what we are seeing here is not like that.

bisect.c already uses a "flags" variable in several locations that would
collide with this. Perhaps rename the existing "flags" variable to
"commit_flags" to explicitly differentiate?

> > + if (!!skipped_revs.nr)
> > +   bisect_flags |= BISECT_FIND_ALL;
> > +

> You do not care what kind of "true" you are feeding "if()", so I do
> not think you would want to keep !! prefix here.

OK, removed.

> The set of bits to go to your "bisect_flags" are only these two new
> ones, and the existing BISECT_SHOW_ALL does not belong to it (it is
> a bit in rev_list_info.flags), but it is not apparent.  I wonder if
> there is a good way to help readers easily tell these two sets apart.
> These are flags passed to find_bisection(), so perhaps
> 
>     #define FIND_BISECTION_ALL (1U<<0)
>     #define FIND_BISECTION_FIRST_PARENT_ONLY (1U<<1)
>     // ...
>
> would be a reasonable compromise?

Sounds good, renamed.

Aaron Lipman (4):
  rev-list: allow bisect and first-parent flags
  bisect: introduce first-parent flag
  bisect: combine args passed to find_bisection()
  bisect: consistent style for git bisect run tests

 Documentation/git-bisect.txt       | 13 ++++-
 Documentation/rev-list-options.txt |  7 ++-
 bisect.c                           | 81 +++++++++++++++++++-----------
 bisect.h                           |  5 +-
 builtin/bisect--helper.c           | 14 ++++--
 builtin/rev-list.c                 |  9 +++-
 revision.c                         |  3 --
 t/t6000-rev-list-misc.sh           |  4 +-
 t/t6002-rev-list-bisect.sh         | 45 +++++++++++++++++
 t/t6030-bisect-porcelain.sh        | 64 ++++++++++++++---------
 10 files changed, 176 insertions(+), 69 deletions(-)

-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v3 1/4] rev-list: allow bisect and first-parent flags
  2020-08-01 17:58   ` [PATCH v3 0/4] " Aaron Lipman
@ 2020-08-01 17:58     ` Aaron Lipman
  2020-08-01 17:58     ` [PATCH v3 2/4] bisect: introduce first-parent flag Aaron Lipman
                       ` (4 subsequent siblings)
  5 siblings, 0 replies; 32+ messages in thread
From: Aaron Lipman @ 2020-08-01 17:58 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman

Add first_parent_only parameter to find_bisection(), removing the
barrier that prevented combining the --bisect and --first-parent flags
when using git rev-list

Based-on-patch-by: Tiago Botelho <tiagonbotelho@hotmail.com>
Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 Documentation/rev-list-options.txt |  7 ++---
 bisect.c                           | 28 ++++++++++++-------
 bisect.h                           |  2 +-
 builtin/rev-list.c                 |  2 +-
 revision.c                         |  3 --
 t/t6000-rev-list-misc.sh           |  4 +--
 t/t6002-rev-list-bisect.sh         | 45 ++++++++++++++++++++++++++++++
 7 files changed, 70 insertions(+), 21 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index b01b2b6773..d3117ce51b 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -128,8 +128,7 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).
 	because merges into a topic branch tend to be only about
 	adjusting to updated upstream from time to time, and
 	this option allows you to ignore the individual commits
-	brought in to your history by such a merge. Cannot be
-	combined with --bisect.
+	brought in to your history by such a merge.
 
 --not::
 	Reverses the meaning of the '{caret}' prefix (or lack thereof)
@@ -207,7 +206,7 @@ ifndef::git-rev-list[]
 	Pretend as if the bad bisection ref `refs/bisect/bad`
 	was listed and as if it was followed by `--not` and the good
 	bisection refs `refs/bisect/good-*` on the command
-	line. Cannot be combined with --first-parent.
+	line.
 endif::git-rev-list[]
 
 --stdin::
@@ -743,7 +742,7 @@ outputs 'midpoint', the output of the two commands
 would be of roughly the same length.  Finding the change which
 introduces a regression is thus reduced to a binary search: repeatedly
 generate and test new 'midpoint's until the commit chain is of length
-one. Cannot be combined with --first-parent.
+one.
 
 --bisect-vars::
 	This calculates the same as `--bisect`, except that refs in
diff --git a/bisect.c b/bisect.c
index d5e830410f..a11fdb1473 100644
--- a/bisect.c
+++ b/bisect.c
@@ -88,15 +88,16 @@ static inline void weight_set(struct commit_list *elem, int weight)
 	**commit_weight_at(&commit_weight, elem->item) = weight;
 }
 
-static int count_interesting_parents(struct commit *commit)
+static int count_interesting_parents(struct commit *commit, int first_parent_only)
 {
 	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 (first_parent_only)
+			break;
 	}
 	return count;
 }
@@ -259,7 +260,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 first_parent_only)
 {
 	int n, counted;
 	struct commit_list *p;
@@ -271,7 +272,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 		unsigned flags = commit->object.flags;
 
 		*commit_weight_at(&commit_weight, p->item) = &weights[n++];
-		switch (count_interesting_parents(commit)) {
+		switch (count_interesting_parents(commit, first_parent_only)) {
 		case 0:
 			if (!(flags & TREESAME)) {
 				weight_set(p, 1);
@@ -314,6 +315,8 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 			continue;
 		if (weight(p) != -2)
 			continue;
+		if (first_parent_only)
+			BUG("shouldn't be calling count-distance in fp mode");
 		weight_set(p, count_distance(p));
 		clear_distance(list);
 
@@ -332,7 +335,10 @@ 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) {
+
+			for (q = p->item->parents;
+			     q;
+			     q = first_parent_only ? NULL : q->next) {
 				if (q->item->object.flags & UNINTERESTING)
 					continue;
 				if (0 <= weight(q))
@@ -370,7 +376,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 }
 
 void find_bisection(struct commit_list **commit_list, int *reaches,
-		    int *all, int find_all)
+		    int *all, int find_all, int first_parent_only)
 {
 	int nr, on_list;
 	struct commit_list *list, *p, *best, *next, *last;
@@ -406,7 +412,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, first_parent_only);
 	if (best) {
 		if (!find_all) {
 			list->item = best->item;
@@ -991,6 +997,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
 	enum bisect_error res = BISECT_OK;
 	struct object_id *bisect_rev;
 	char *steps_msg;
+	int first_parent_only = 0; /* TODO: pass --first-parent flag from git bisect start */
 
 	read_bisect_terms(&term_bad, &term_good);
 	if (read_bisect_refs())
@@ -1001,11 +1008,12 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
 		return res;
 
 	bisect_rev_setup(r, &revs, prefix, "%s", "^%s", 1);
+	revs.first_parent_only = first_parent_only;
 	revs.limited = 1;
 
 	bisect_common(&revs);
 
-	find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr);
+	find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only);
 	revs.commits = managed_skipped(revs.commits, &tried);
 
 	if (!revs.commits) {
diff --git a/bisect.h b/bisect.h
index 8bad8d8391..a63af0505f 100644
--- a/bisect.h
+++ b/bisect.h
@@ -12,7 +12,7 @@ struct repository;
  * best commit, as chosen by `find_all`.
  */
 void find_bisection(struct commit_list **list, int *reaches, int *all,
-		    int find_all);
+		    int find_all, int first_parent_only);
 
 struct commit_list *filter_skipped(struct commit_list *list,
 				   struct commit_list **tried,
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index f520111eda..d1a14596b2 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -638,7 +638,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, revs.first_parent_only);
 
 		if (bisect_show_vars)
 			return show_bisect_vars(&info, reaches, all);
diff --git a/revision.c b/revision.c
index 6aa7f4f567..f0ad2603c1 100644
--- a/revision.c
+++ b/revision.c
@@ -2869,9 +2869,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->line_level_traverse &&
 	    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
 		die(_("-L does not yet support diff formats besides -p and -s"));
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 3dc1ad8f71..26e3533562 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -125,8 +125,8 @@ test_expect_success 'rev-list can negate index objects' '
 	test_cmp expect actual
 '
 
-test_expect_success '--bisect and --first-parent can not be combined' '
-	test_must_fail git rev-list --bisect --first-parent HEAD
+test_expect_success '--bisect and --first-parent can be combined' '
+	git rev-list --bisect --first-parent HEAD
 '
 
 test_expect_success '--header shows a NUL after each commit' '
diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
index a661408038..7fc0f75ca5 100755
--- a/t/t6002-rev-list-bisect.sh
+++ b/t/t6002-rev-list-bisect.sh
@@ -263,4 +263,49 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' '
 	test_cmp expect.sorted actual.sorted
 '
 
+test_output_expect_success "--bisect --first-parent" 'git rev-list --bisect --first-parent E ^F' <<EOF
+e4
+EOF
+
+test_output_expect_success "--first-parent" 'git rev-list --first-parent E ^F' <<EOF
+E
+e1
+e2
+e3
+e4
+e5
+e6
+e7
+e8
+EOF
+
+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.unsorted <<EOF &&
+$(git rev-parse E) (tag: E, dist=0)
+$(git rev-parse e1) (tag: e1, dist=1)
+$(git rev-parse e2) (tag: e2, dist=2)
+$(git rev-parse e3) (tag: e3, dist=3)
+$(git rev-parse e4) (tag: e4, dist=4)
+$(git rev-parse e5) (tag: e5, dist=4)
+$(git rev-parse e6) (tag: e6, dist=3)
+$(git rev-parse e7) (tag: e7, dist=2)
+$(git rev-parse e8) (tag: e8, dist=1)
+EOF
+
+# expect results to be ordered by distance (descending),
+# commit hash (ascending)
+sort -k4,4r -k1,1 expect.unsorted > expect &&
+git rev-list --bisect-all --first-parent E ^F > actual &&
+test_cmp actual expect
+'
+
 test_done
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v3 2/4] bisect: introduce first-parent flag
  2020-08-01 17:58   ` [PATCH v3 0/4] " Aaron Lipman
  2020-08-01 17:58     ` [PATCH v3 1/4] rev-list: allow bisect and first-parent flags Aaron Lipman
@ 2020-08-01 17:58     ` Aaron Lipman
  2020-08-01 17:58     ` [PATCH v3 3/4] bisect: combine args passed to find_bisection() Aaron Lipman
                       ` (3 subsequent siblings)
  5 siblings, 0 replies; 32+ messages in thread
From: Aaron Lipman @ 2020-08-01 17:58 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman

Upon seeing a merge commit when bisecting, this option may be used to
follow only the first parent.

In detecting regressions introduced through the merging of a branch, the
merge commit will be identified as introduction of the bug and its
ancestors will be ignored.

This option is particularly useful in avoiding false positives when a
merged branch contained broken or non-buildable commits, but the merge
itself was OK.

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 Documentation/git-bisect.txt | 13 ++++++++++++-
 bisect.c                     | 10 +++++++++-
 builtin/bisect--helper.c     | 14 ++++++++++----
 t/t6030-bisect-porcelain.sh  | 18 ++++++++++++++++++
 4 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 7586c5a843..0e993e4587 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -17,7 +17,7 @@ The command takes various subcommands, and different options depending
 on the subcommand:
 
  git bisect start [--term-{old,good}=<term> --term-{new,bad}=<term>]
-		  [--no-checkout] [<bad> [<good>...]] [--] [<paths>...]
+		  [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]
  git bisect (bad|new|<term-new>) [<rev>]
  git bisect (good|old|<term-old>) [<rev>...]
  git bisect terms [--term-good | --term-bad]
@@ -365,6 +365,17 @@ does not require a checked out tree.
 +
 If the repository is bare, `--no-checkout` is assumed.
 
+--first-parent::
++
+Follow only the first parent commit upon seeing a merge commit.
++
+In detecting regressions introduced through the merging of a branch, the merge
+commit will be identified as introduction of the bug and its ancestors will be
+ignored.
++
+This option is particularly useful in avoiding false positives when a merged
+branch contained broken or non-buildable commits, but the merge itself was OK.
+
 EXAMPLES
 --------
 
diff --git a/bisect.c b/bisect.c
index a11fdb1473..afbb5aea32 100644
--- a/bisect.c
+++ b/bisect.c
@@ -460,6 +460,7 @@ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
+static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
 static GIT_PATH_FUNC(git_path_head_name, "head-name")
 
 static void read_bisect_paths(struct argv_array *array)
@@ -981,6 +982,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
 	fclose(fp);
 }
 
+static int read_first_parent_option(void)
+{
+	const char *filename = git_path_bisect_first_parent();
+	return !access(filename, F_OK);
+}
+
 /*
  * We use the convention that return BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND (-10) means
  * the bisection process finished successfully.
@@ -997,7 +1004,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
 	enum bisect_error res = BISECT_OK;
 	struct object_id *bisect_rev;
 	char *steps_msg;
-	int first_parent_only = 0; /* TODO: pass --first-parent flag from git bisect start */
+	int first_parent_only = read_first_parent_option();
 
 	read_bisect_terms(&term_bad, &term_good);
 	if (read_bisect_refs())
@@ -1141,6 +1148,7 @@ int bisect_clean_state(void)
 	unlink_or_warn(git_path_bisect_names());
 	unlink_or_warn(git_path_bisect_run());
 	unlink_or_warn(git_path_bisect_terms());
+	unlink_or_warn(git_path_bisect_first_parent());
 	/* Cleanup head-name if it got left by an old version of git-bisect */
 	unlink_or_warn(git_path_head_name());
 	/*
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index ec4996282e..9273140f0b 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -17,6 +17,7 @@ static GIT_PATH_FUNC(git_path_bisect_head, "BISECT_HEAD")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 static GIT_PATH_FUNC(git_path_head_name, "head-name")
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
+static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
@@ -28,7 +29,7 @@ static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --bisect-next-check <good_term> <bad_term> [<term>]"),
 	N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
 	N_("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]"
-					     "[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
+					    " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"),
 	NULL
 };
 
@@ -422,7 +423,7 @@ static int bisect_append_log_quoted(const char **argv)
 }
 
 static int bisect_start(struct bisect_terms *terms, int no_checkout,
-			const char **argv, int argc)
+			int first_parent_only, const char **argv, int argc)
 {
 	int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
 	int flags, pathspec_pos, res = 0;
@@ -577,6 +578,9 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 	 */
 	write_file(git_path_bisect_start(), "%s\n", start_head.buf);
 
+	if (first_parent_only)
+		write_file(git_path_bisect_first_parent(), "\n");
+
 	if (no_checkout) {
 		if (get_oid(start_head.buf, &oid) < 0) {
 			res = error(_("invalid ref: '%s'"), start_head.buf);
@@ -632,7 +636,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_TERMS,
 		BISECT_START
 	} cmdmode = 0;
-	int no_checkout = 0, res = 0, nolog = 0;
+	int no_checkout = 0, first_parent_only = 0, res = 0, nolog = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &cmdmode,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
@@ -656,6 +660,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("start the bisect session"), BISECT_START),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
+		OPT_BOOL(0, "first-parent", &first_parent_only,
+			 N_("only trace the first parent of merge commits")),
 		OPT_BOOL(0, "no-log", &nolog,
 			 N_("no log for BISECT_WRITE")),
 		OPT_END()
@@ -713,7 +719,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		break;
 	case BISECT_START:
 		set_terms(&terms, "bad", "good");
-		res = bisect_start(&terms, no_checkout, argv, argc);
+		res = bisect_start(&terms, no_checkout, first_parent_only, argv, argc);
 		break;
 	default:
 		return error("BUG: unknown subcommand '%d'", cmdmode);
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 36d9b2b2e4..51f5eb6ea3 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -458,6 +458,24 @@ test_expect_success 'many merge bases creation' '
 	grep "$SIDE_HASH5" merge_bases.txt
 '
 
+# We want to automatically find the merge that
+# added "line" into hello.
+test_expect_success '"git bisect run --first-parent" simple case' '
+	git rev-list --first-parent $B_HASH ^$HASH4 >first_parent_chain.txt &&
+	write_script test_script.sh <<-\EOF &&
+	grep $(git rev-parse HEAD) first_parent_chain.txt || exit -1
+	! grep line hello >/dev/null
+	EOF
+  git bisect start --first-parent &&
+  test_path_is_file ".git/BISECT_FIRST_PARENT" &&
+  git bisect good $HASH4 &&
+  git bisect bad $B_HASH &&
+  git bisect run ./test_script.sh >my_bisect_log.txt &&
+  grep "$B_HASH is the first bad commit" my_bisect_log.txt &&
+  git bisect reset &&
+  test_path_is_missing .git/BISECT_FIRST_PARENT
+'
+
 test_expect_success 'good merge bases when good and bad are siblings' '
 	git bisect start "$B_HASH" "$A_HASH" > my_bisect_log.txt &&
 	test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v3 3/4] bisect: combine args passed to find_bisection()
  2020-08-01 17:58   ` [PATCH v3 0/4] " Aaron Lipman
  2020-08-01 17:58     ` [PATCH v3 1/4] rev-list: allow bisect and first-parent flags Aaron Lipman
  2020-08-01 17:58     ` [PATCH v3 2/4] bisect: introduce first-parent flag Aaron Lipman
@ 2020-08-01 17:58     ` Aaron Lipman
  2020-08-01 19:30       ` Martin Ågren
  2020-08-01 17:58     ` [PATCH v3 4/4] bisect: consistent style for git bisect run tests Aaron Lipman
                       ` (2 subsequent siblings)
  5 siblings, 1 reply; 32+ messages in thread
From: Aaron Lipman @ 2020-08-01 17:58 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman

Now that find_bisection() accepts multiple boolean arguments, these may
be combined into a single unsigned integer in order to declutter some of
the code in bisect.c

Also, rename existing the "flags" bitfield to "commit_flags", to
explicitly differentiate between the new "bisect_flags" bitfield.

Based-on-patch-by: Harald Nordgren <haraldnordgren@gmail.com>
Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 bisect.c           | 67 +++++++++++++++++++++++++---------------------
 bisect.h           |  5 +++-
 builtin/rev-list.c |  9 ++++++-
 3 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/bisect.c b/bisect.c
index afbb5aea32..b14a2edc38 100644
--- a/bisect.c
+++ b/bisect.c
@@ -88,7 +88,7 @@ static inline void weight_set(struct commit_list *elem, int weight)
 	**commit_weight_at(&commit_weight, elem->item) = weight;
 }
 
-static int count_interesting_parents(struct commit *commit, int first_parent_only)
+static int count_interesting_parents(struct commit *commit, unsigned bisect_flags)
 {
 	struct commit_list *p;
 	int count;
@@ -96,7 +96,7 @@ static int count_interesting_parents(struct commit *commit, int first_parent_onl
 	for (count = 0, p = commit->parents; p; p = p->next) {
 		if (!(p->item->object.flags & UNINTERESTING))
 			count++;
-		if (first_parent_only)
+		if (bisect_flags & FIND_BISECTION_FIRST_PARENT_ONLY)
 			break;
 	}
 	return count;
@@ -136,7 +136,7 @@ static void show_list(const char *debug, int counted, int nr,
 	for (p = list; p; p = p->next) {
 		struct commit_list *pp;
 		struct commit *commit = p->item;
-		unsigned flags = commit->object.flags;
+		unsigned commit_flags = commit->object.flags;
 		enum object_type type;
 		unsigned long size;
 		char *buf = read_object_file(&commit->object.oid, &type,
@@ -145,9 +145,9 @@ static void show_list(const char *debug, int counted, int nr,
 		int subject_len;
 
 		fprintf(stderr, "%c%c%c ",
-			(flags & TREESAME) ? ' ' : 'T',
-			(flags & UNINTERESTING) ? 'U' : ' ',
-			(flags & COUNTED) ? 'C' : ' ');
+			(commit_flags & TREESAME) ? ' ' : 'T',
+			(commit_flags & UNINTERESTING) ? 'U' : ' ',
+			(commit_flags & COUNTED) ? 'C' : ' ');
 		if (*commit_weight_at(&commit_weight, p->item))
 			fprintf(stderr, "%3d", weight(p));
 		else
@@ -172,9 +172,9 @@ static struct commit_list *best_bisection(struct commit_list *list, int nr)
 	best = list;
 	for (p = list; p; p = p->next) {
 		int distance;
-		unsigned flags = p->item->object.flags;
+		unsigned commit_flags = p->item->object.flags;
 
-		if (flags & TREESAME)
+		if (commit_flags & TREESAME)
 			continue;
 		distance = weight(p);
 		if (nr - distance < distance)
@@ -213,9 +213,9 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n
 
 	for (p = list, cnt = 0; p; p = p->next) {
 		int distance;
-		unsigned flags = p->item->object.flags;
+		unsigned commit_flags = p->item->object.flags;
 
-		if (flags & TREESAME)
+		if (commit_flags & TREESAME)
 			continue;
 		distance = weight(p);
 		if (nr - distance < distance)
@@ -260,7 +260,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 first_parent_only)
+					     unsigned bisect_flags)
 {
 	int n, counted;
 	struct commit_list *p;
@@ -269,12 +269,12 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 
 	for (n = 0, p = list; p; p = p->next) {
 		struct commit *commit = p->item;
-		unsigned flags = commit->object.flags;
+		unsigned commit_flags = commit->object.flags;
 
 		*commit_weight_at(&commit_weight, p->item) = &weights[n++];
-		switch (count_interesting_parents(commit, first_parent_only)) {
+		switch (count_interesting_parents(commit, bisect_flags)) {
 		case 0:
-			if (!(flags & TREESAME)) {
+			if (!(commit_flags & TREESAME)) {
 				weight_set(p, 1);
 				counted++;
 				show_list("bisection 2 count one",
@@ -315,13 +315,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 			continue;
 		if (weight(p) != -2)
 			continue;
-		if (first_parent_only)
+		if (bisect_flags & FIND_BISECTION_FIRST_PARENT_ONLY)
 			BUG("shouldn't be calling count-distance in fp mode");
 		weight_set(p, count_distance(p));
 		clear_distance(list);
 
 		/* Does it happen to be at exactly half-way? */
-		if (!find_all && halfway(p, nr))
+		if (!(bisect_flags & FIND_BISECTION_ALL) && halfway(p, nr))
 			return p;
 		counted++;
 	}
@@ -331,14 +331,14 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 	while (counted < nr) {
 		for (p = list; p; p = p->next) {
 			struct commit_list *q;
-			unsigned flags = p->item->object.flags;
+			unsigned commit_flags = p->item->object.flags;
 
 			if (0 <= weight(p))
 				continue;
 
 			for (q = p->item->parents;
 			     q;
-			     q = first_parent_only ? NULL : q->next) {
+			     q = bisect_flags & FIND_BISECTION_FIRST_PARENT_ONLY ? NULL : q->next) {
 				if (q->item->object.flags & UNINTERESTING)
 					continue;
 				if (0 <= weight(q))
@@ -352,7 +352,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 			 * add one for p itself if p is to be counted,
 			 * otherwise inherit it from q directly.
 			 */
-			if (!(flags & TREESAME)) {
+			if (!(commit_flags & TREESAME)) {
 				weight_set(p, weight(q)+1);
 				counted++;
 				show_list("bisection 2 count one",
@@ -362,21 +362,21 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 				weight_set(p, weight(q));
 
 			/* Does it happen to be at exactly half-way? */
-			if (!find_all && halfway(p, nr))
+			if (!(bisect_flags & FIND_BISECTION_ALL) && halfway(p, nr))
 				return p;
 		}
 	}
 
 	show_list("bisection 2 counted all", counted, nr, list);
 
-	if (!find_all)
+	if (!(bisect_flags & FIND_BISECTION_ALL))
 		return best_bisection(list, nr);
 	else
 		return best_bisection_sorted(list, nr);
 }
 
 void find_bisection(struct commit_list **commit_list, int *reaches,
-		    int *all, int find_all, int first_parent_only)
+		    int *all, unsigned bisect_flags)
 {
 	int nr, on_list;
 	struct commit_list *list, *p, *best, *next, *last;
@@ -392,16 +392,16 @@ void find_bisection(struct commit_list **commit_list, int *reaches,
 	for (nr = on_list = 0, last = NULL, p = *commit_list;
 	     p;
 	     p = next) {
-		unsigned flags = p->item->object.flags;
+		unsigned commit_flags = p->item->object.flags;
 
 		next = p->next;
-		if (flags & UNINTERESTING) {
+		if (commit_flags & UNINTERESTING) {
 			free(p);
 			continue;
 		}
 		p->next = last;
 		last = p;
-		if (!(flags & TREESAME))
+		if (!(commit_flags & TREESAME))
 			nr++;
 		on_list++;
 	}
@@ -412,9 +412,9 @@ 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, first_parent_only);
+	best = do_find_bisection(list, nr, weights, bisect_flags);
 	if (best) {
-		if (!find_all) {
+		if (!(bisect_flags & FIND_BISECTION_ALL)) {
 			list->item = best->item;
 			free_commit_list(list->next);
 			best = list;
@@ -1004,23 +1004,30 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
 	enum bisect_error res = BISECT_OK;
 	struct object_id *bisect_rev;
 	char *steps_msg;
-	int first_parent_only = read_first_parent_option();
+	unsigned bisect_flags = 0;
 
 	read_bisect_terms(&term_bad, &term_good);
 	if (read_bisect_refs())
 		die(_("reading bisect refs failed"));
 
+	if (read_first_parent_option())
+		bisect_flags |= FIND_BISECTION_FIRST_PARENT_ONLY;
+
+	if (skipped_revs.nr)
+		bisect_flags |= FIND_BISECTION_ALL;
+
 	res = check_good_are_ancestors_of_bad(r, prefix, no_checkout);
 	if (res)
 		return res;
 
 	bisect_rev_setup(r, &revs, prefix, "%s", "^%s", 1);
-	revs.first_parent_only = first_parent_only;
+
+	revs.first_parent_only = !!(bisect_flags & FIND_BISECTION_FIRST_PARENT_ONLY);
 	revs.limited = 1;
 
 	bisect_common(&revs);
 
-	find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only);
+	find_bisection(&revs.commits, &reaches, &all, bisect_flags);
 	revs.commits = managed_skipped(revs.commits, &tried);
 
 	if (!revs.commits) {
diff --git a/bisect.h b/bisect.h
index a63af0505f..b673fd9ed2 100644
--- a/bisect.h
+++ b/bisect.h
@@ -12,7 +12,7 @@ struct repository;
  * best commit, as chosen by `find_all`.
  */
 void find_bisection(struct commit_list **list, int *reaches, int *all,
-		    int find_all, int first_parent_only);
+		    unsigned bisect_flags);
 
 struct commit_list *filter_skipped(struct commit_list *list,
 				   struct commit_list **tried,
@@ -23,6 +23,9 @@ struct commit_list *filter_skipped(struct commit_list *list,
 #define BISECT_SHOW_ALL		(1<<0)
 #define REV_LIST_QUIET		(1<<1)
 
+#define FIND_BISECTION_ALL			(1u<<0)
+#define FIND_BISECTION_FIRST_PARENT_ONLY	(1u<<1)
+
 struct rev_list_info {
 	struct rev_info *revs;
 	int flags;
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index d1a14596b2..25c6c3b38d 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -637,8 +637,15 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 
 	if (bisect_list) {
 		int reaches, all;
+		unsigned bisect_flags = 0;
 
-		find_bisection(&revs.commits, &reaches, &all, bisect_find_all, revs.first_parent_only);
+		if (bisect_find_all)
+			bisect_flags |= FIND_BISECTION_ALL;
+
+		if (revs.first_parent_only)
+			bisect_flags |= FIND_BISECTION_FIRST_PARENT_ONLY;
+
+		find_bisection(&revs.commits, &reaches, &all, bisect_flags);
 
 		if (bisect_show_vars)
 			return show_bisect_vars(&info, reaches, all);
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v3 4/4] bisect: consistent style for git bisect run tests
  2020-08-01 17:58   ` [PATCH v3 0/4] " Aaron Lipman
                       ` (2 preceding siblings ...)
  2020-08-01 17:58     ` [PATCH v3 3/4] bisect: combine args passed to find_bisection() Aaron Lipman
@ 2020-08-01 17:58     ` Aaron Lipman
  2020-08-01 19:27       ` Martin Ågren
  2020-08-01 19:06     ` [PATCH v3 0/4] Introduce --first-parent flag for git bisect Junio C Hamano
  2020-08-04 22:01     ` [PATCH v4 0/5] " Aaron Lipman
  5 siblings, 1 reply; 32+ messages in thread
From: Aaron Lipman @ 2020-08-01 17:58 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman

Enforce consistent styling for tests on "git bisect run":
- Use "write_script" to abstract away platform-specific details.
- Favor current whitespace conventions.
---
 t/t6030-bisect-porcelain.sh | 46 ++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 24 deletions(-)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 51f5eb6ea3..63661c5e3d 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -243,32 +243,30 @@ test_expect_success 'bisect skip: with commit both bad and skipped' '
 '
 
 # We want to automatically find the commit that
-# introduced "Another" into hello.
-test_expect_success \
-    '"git bisect run" simple case' \
-    'echo "#"\!"/bin/sh" > test_script.sh &&
-     echo "grep Another hello > /dev/null" >> test_script.sh &&
-     echo "test \$? -ne 0" >> test_script.sh &&
-     chmod +x test_script.sh &&
-     git bisect start &&
-     git bisect good $HASH1 &&
-     git bisect bad $HASH4 &&
-     git bisect run ./test_script.sh > my_bisect_log.txt &&
-     grep "$HASH3 is the first bad commit" my_bisect_log.txt &&
-     git bisect reset'
+# added "Another" into hello.
+test_expect_success '"git bisect run" simple case' '
+	write_script test_script.sh <<-\EOF &&
+	! grep Another hello >/dev/null
+	EOF
+	git bisect start &&
+	git bisect good $HASH1 &&
+	git bisect bad $HASH4 &&
+	git bisect run ./test_script.sh >my_bisect_log.txt &&
+	grep "$HASH3 is the first bad commit" my_bisect_log.txt &&
+	git bisect reset
+'
 
 # We want to automatically find the commit that
-# introduced "Ciao" into hello.
-test_expect_success \
-    '"git bisect run" with more complex "git bisect start"' \
-    'echo "#"\!"/bin/sh" > test_script.sh &&
-     echo "grep Ciao hello > /dev/null" >> test_script.sh &&
-     echo "test \$? -ne 0" >> test_script.sh &&
-     chmod +x test_script.sh &&
-     git bisect start $HASH4 $HASH1 &&
-     git bisect run ./test_script.sh > my_bisect_log.txt &&
-     grep "$HASH4 is the first bad commit" my_bisect_log.txt &&
-     git bisect reset'
+# added "Ciao" into hello.
+test_expect_success '"git bisect run" with more complex "git bisect start"' '
+	write_script test_script.sh <<-\EOF &&
+	! grep Ciao hello >/dev/null
+	EOF
+	git bisect start $HASH4 $HASH1 &&
+	git bisect run ./test_script.sh >my_bisect_log.txt &&
+	grep "$HASH4 is the first bad commit" my_bisect_log.txt &&
+	git bisect reset
+'
 
 # $HASH1 is good, $HASH5 is bad, we skip $HASH3
 # but $HASH4 is good,
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 0/4] Introduce --first-parent flag for git bisect
  2020-08-01 17:58   ` [PATCH v3 0/4] " Aaron Lipman
                       ` (3 preceding siblings ...)
  2020-08-01 17:58     ` [PATCH v3 4/4] bisect: consistent style for git bisect run tests Aaron Lipman
@ 2020-08-01 19:06     ` Junio C Hamano
  2020-08-04 22:01     ` [PATCH v4 0/5] " Aaron Lipman
  5 siblings, 0 replies; 32+ messages in thread
From: Junio C Hamano @ 2020-08-01 19:06 UTC (permalink / raw)
  To: Aaron Lipman; +Cc: git

Aaron Lipman <alipman88@gmail.com> writes:

>> I suspect that you would ask the same question to whoever added
>> no_checkout to this thing, and I wouldn't be surprised if we end up
>> removing both of these parameters (and parsing of the options inside
>> cmd_bisect__helper()) after thinking about them, but anyway, let's
>> read on.
>
> Hmm. Is there a preferred way to to add a --first-parent line to the
> output of "git bisect start --help" via the parse-options API without
> removing the --first-parent option from argv in the process?

I'd rather not to see the code made ugly if the only reason why you
have duplicated command line parsing is to support "git bisect start
-h" while cmd_bisect__helper() still exists.  

That function is supposed to be a thin shim layer whose only reason
of its existence is to serve as an interface with the scripted
version of "git bisect".  When everything is migrated from the shell
script, cmd_bisect__helper() should disappear.

In its place, cmd_bisect() written in C would become a dispatcher
that only looks at the first token on the command line that comes
after ["git", "bisect"] and calls "bisect_start()", "bisect_next()",
etc. with the remainder of the command line with options such as
"--first-parent" and "--no-checkout", which will be parsed by
bisect_start() etc. parsing its argc/argv[] passed by cmd_bisect().
It is likely that cmd_bisect() would not have any call to
parse_options(); instead cmd_start() etc. would call parse_options()
with their own option[] table.

So I am not sure if the change between v2 and v3 is going in the
right direction.

> As long as we're capturing the --first-parent option in
> cmd_bisect__helper(), checking argv for a --first-parent flag in
> bisect_start() is pointless.

This is going backwards, as far as I can tell.  If anything, I'd
rather see cmd_bisect__helper() get fixed so that it does not parse
"--first-parent" (and similarly, "--no-checkout" that you imitated)
into first_parent_only (and no_checkout) variables and passed as
parameters to bisect_start().  cmd_bisect__helper() should instead
just let these flags (and there may be others) sit in argc/argv[]
and have bisect_start() parse them, together with all other options
bisect_start() already has its parser for.

Thanks.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 4/4] bisect: consistent style for git bisect run tests
  2020-08-01 17:58     ` [PATCH v3 4/4] bisect: consistent style for git bisect run tests Aaron Lipman
@ 2020-08-01 19:27       ` Martin Ågren
  0 siblings, 0 replies; 32+ messages in thread
From: Martin Ågren @ 2020-08-01 19:27 UTC (permalink / raw)
  To: Aaron Lipman; +Cc: git

On Sat, 1 Aug 2020 at 20:03, Aaron Lipman <alipman88@gmail.com> wrote:
> Subject: bisect: consistent style for git bisect run tests

Nit: this could be 't6030: modernize "git bisect run" tests' to tighten
the area a bit and to begin with a verb.

> Enforce consistent styling for tests on "git bisect run":
> - Use "write_script" to abstract away platform-specific details.
> - Favor current whitespace conventions.
> ---

Missing sign-off.

I think this should come earlier in the series, probably already as
patch 1/4. Some reasons for that:

 * If this effort doesn't go any further (let's hope it does!), at
   least the initial cleanup can be merged, so that later endeavours can
   find a slightly cleaner table to start with.

 * Revisiting your other patches in `git log` after they've been merged,
   it will be obvious that they follow other tests, so one can focus on
   what they do and not on how they differ.

 * Let's first tidy up, then build shiny new things. ;-)

>  # We want to automatically find the commit that
> -# introduced "Another" into hello.
> -test_expect_success \
> -    '"git bisect run" simple case' \
> -    'echo "#"\!"/bin/sh" > test_script.sh &&
[...]
> +# added "Another" into hello.
> +test_expect_success '"git bisect run" simple case' '
> +       write_script test_script.sh <<-\EOF &&
[...]

All of these look like correct, worthwhile modifications. The
s/introduced/added/ changes aren't mentioned in the proposed log
message. They have been discussed up-thread, but it might still be
good to add something like

  While at it, change "introduced" to "added" in the comments to make
  them read better.

FWIW, I guess there's some minor added value in doing that change and
since you're doing other changes anyway, it might actually be worth the
churn.

You change two tests like this, but there are other "git bisect run"
tests that look like they're in need of some modernizing. Looking for
"chmod" I spotted the ones I'm tweaking below. Looking for "git bisect
run" you might find more that may contain some funny indentation. I'll
leave it to you where to stop going down that rabbit-hole but if you do
re-roll, it might make sense to squash in the diff below, maybe with
some modifications to the commit message. (Signed-off-by: Martin Ågren
<martin.agren@gmail.com>, FWIW.)

Martin

-- >8 --

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 63661c5e3d..66c12bc665 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -293,24 +293,17 @@ HASH6=
 test_expect_success 'bisect run & skip: cannot tell between 2' '
 	add_line_into_file "6: Yet a line." hello &&
 	HASH6=$(git rev-parse --verify HEAD) &&
-	echo "#"\!"/bin/sh" > test_script.sh &&
-	echo "sed -ne \\\$p hello | grep Ciao > /dev/null && exit 125" >> test_script.sh &&
-	echo "grep line hello > /dev/null" >> test_script.sh &&
-	echo "test \$? -ne 0" >> test_script.sh &&
-	chmod +x test_script.sh &&
+	write_script test_script.sh <<-\EOF &&
+	sed -ne \$p hello | grep Ciao >/dev/null && exit 125
+	! grep line hello >/dev/null
+	EOF
 	git bisect start $HASH6 $HASH1 &&
-	if git bisect run ./test_script.sh > my_bisect_log.txt
-	then
-		echo Oops, should have failed.
-		false
-	else
-		test $? -eq 2 &&
-		grep "first bad commit could be any of" my_bisect_log.txt &&
-		! grep $HASH3 my_bisect_log.txt &&
-		! grep $HASH6 my_bisect_log.txt &&
-		grep $HASH4 my_bisect_log.txt &&
-		grep $HASH5 my_bisect_log.txt
-	fi
+	test_expect_code 2 git bisect run ./test_script.sh >my_bisect_log.txt &&
+	grep "first bad commit could be any of" my_bisect_log.txt &&
+	! grep $HASH3 my_bisect_log.txt &&
+	! grep $HASH6 my_bisect_log.txt &&
+	grep $HASH4 my_bisect_log.txt &&
+	grep $HASH5 my_bisect_log.txt
 '
 
 HASH7=
@@ -318,14 +311,13 @@ test_expect_success 'bisect run & skip: find first bad' '
 	git bisect reset &&
 	add_line_into_file "7: Should be the last line." hello &&
 	HASH7=$(git rev-parse --verify HEAD) &&
-	echo "#"\!"/bin/sh" > test_script.sh &&
-	echo "sed -ne \\\$p hello | grep Ciao > /dev/null && exit 125" >> test_script.sh &&
-	echo "sed -ne \\\$p hello | grep day > /dev/null && exit 125" >> test_script.sh &&
-	echo "grep Yet hello > /dev/null" >> test_script.sh &&
-	echo "test \$? -ne 0" >> test_script.sh &&
-	chmod +x test_script.sh &&
+	write_script test_script.sh <<-\EOF &&
+	sed -ne \$p hello | grep Ciao >/dev/null && exit 125
+	sed -ne \$p hello | grep day >/dev/null && exit 125
+	! grep Yet hello >/dev/null
+	EOF
 	git bisect start $HASH7 $HASH1 &&
-	git bisect run ./test_script.sh > my_bisect_log.txt &&
+	git bisect run ./test_script.sh >my_bisect_log.txt &&
 	grep "$HASH6 is the first bad commit" my_bisect_log.txt
 '
 
-- 
2.28.0.81.ge8ab941b67


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v3 3/4] bisect: combine args passed to find_bisection()
  2020-08-01 17:58     ` [PATCH v3 3/4] bisect: combine args passed to find_bisection() Aaron Lipman
@ 2020-08-01 19:30       ` Martin Ågren
  0 siblings, 0 replies; 32+ messages in thread
From: Martin Ågren @ 2020-08-01 19:30 UTC (permalink / raw)
  To: Aaron Lipman; +Cc: Git Mailing List

On Sat, 1 Aug 2020 at 20:01, Aaron Lipman <alipman88@gmail.com> wrote:
> Also, rename existing the "flags" bitfield to "commit_flags", to
> explicitly differentiate between the new "bisect_flags" bitfield.

s/existing the/the existing/

Also, does "separate between <one thing>" make grammatical sense? Should
it be "separate between <two things>" or "separate [it] from <one
thing>"? Dunno..

Martin

^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v4 0/5] Introduce --first-parent flag for git bisect
  2020-08-01 17:58   ` [PATCH v3 0/4] " Aaron Lipman
                       ` (4 preceding siblings ...)
  2020-08-01 19:06     ` [PATCH v3 0/4] Introduce --first-parent flag for git bisect Junio C Hamano
@ 2020-08-04 22:01     ` Aaron Lipman
  2020-08-04 22:01       ` [PATCH v4 1/5] t6030: modernize "git bisect run" tests Aaron Lipman
                         ` (7 more replies)
  5 siblings, 8 replies; 32+ messages in thread
From: Aaron Lipman @ 2020-08-04 22:01 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman

OK, here's take 4! Responding to Junio's feedback, first:

> That function [cmd_bisect__helper()] is supposed to be a thin shim
> layer whose only reason of its existence is to serve as an interface
> with the scripted version of "git bisect".  When everything is
> migrated from the shell script, cmd_bisect__helper() should disappear.
> ...
> This is going backwards, as far as I can tell.  If anything, I'd
> rather see cmd_bisect__helper() get fixed so that it does not parse
> "--first-parent" (and similarly, "--no-checkout" that you imitated)
> into first_parent_only (and no_checkout) variables and passed as
> parameters to bisect_start().

Thanks for the explanation, Junio. (And for bearing with me while I
gain familiarity with git's codebase.) Now that I've taken some time
to examine how git-bisect.sh and cmd_bisect__helper() fit together,
the correct approach is much more clear.

I've added a commit (3/5 in this iteration) that updates
cmd_bisect__helper() to defer parsing the --no-checkout flag.

As no_checkout was also passed to bisect_next_all() [via implicitly
checking for the existence of .git/BISECT_HEAD when calling
bisect_next() in git-bisect.sh], I've removed that parameter and
instead check for .git/BISECT_HEAD in bisect_next_all() to determine
whether no-checkout mode is on.

This means no-checkout mode can no longer be enabled by "git
bisect--helper --next-all --no-checkout" - it can now only be turned
on when running "git bisect start". But this doesn't change git
bisect's public API, so this should be OK.

Other than the changes suggested by Martin (summarized below), the
only new change is removing the read_first_parent_option() wrapper
function in favor of file_exists() from dir.h.

---

Martin, thanks for your suggestions - I've moved the commit updating
"git bisect run" tests to 1/5, updated the commit message, and
included the changes you provided. (I held off on additional
indentation corrections, as it kinda felt like unnecessary code
churn/outside the scope of abstracting platform-specific details.)

> Also, does "separate between <one thing>" make grammatical sense?
> Should it be "separate between <two things>" or "separate [it] from
> <one thing>"? Dunno..

I think "separate [it] from <one thing>" is correct, good catch!

> (Signed-off-by: Martin Ågren <martin.agren@gmail.com>, FWIW.)

I'm still getting used to the conventions - should I add your name as
a signed-off-by tag, a thanks-to tag, or both?

Aaron Lipman (5):
  t6030: modernize "git bisect run" tests
  rev-list: allow bisect and first-parent flags
  cmd_bisect__helper: defer parsing no-checkout flag
  bisect: introduce first-parent flag
  bisect: combine args passed to find_bisection()

 Documentation/git-bisect.txt       |  13 +++-
 Documentation/rev-list-options.txt |   7 +-
 bisect.c                           |  79 +++++++++++++---------
 bisect.h                           |   9 +--
 builtin/bisect--helper.c           |  23 ++++---
 builtin/rev-list.c                 |   9 ++-
 git-bisect.sh                      |   2 +-
 revision.c                         |   3 -
 t/t6000-rev-list-misc.sh           |   4 +-
 t/t6002-rev-list-bisect.sh         |  45 +++++++++++++
 t/t6030-bisect-porcelain.sh        | 104 ++++++++++++++++-------------
 11 files changed, 195 insertions(+), 103 deletions(-)

-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v4 1/5] t6030: modernize "git bisect run" tests
  2020-08-04 22:01     ` [PATCH v4 0/5] " Aaron Lipman
@ 2020-08-04 22:01       ` Aaron Lipman
  2020-08-05  6:11         ` Christian Couder
  2020-08-04 22:01       ` [PATCH v4 2/5] rev-list: allow bisect and first-parent flags Aaron Lipman
                         ` (6 subsequent siblings)
  7 siblings, 1 reply; 32+ messages in thread
From: Aaron Lipman @ 2020-08-04 22:01 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman

Enforce consistent styling for tests on "git bisect run":
- Use "write_script" to abstract away platform-specific details.
- Favor current whitespace conventions.
- While at it, change "introduced" to "added" in the comments to make
  them read better.

Thanks-to: Martin Ågren <martin.agren@gmail.com>
Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 t/t6030-bisect-porcelain.sh | 86 ++++++++++++++++---------------------
 1 file changed, 38 insertions(+), 48 deletions(-)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 36d9b2b2e4..a66c4b89bc 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -243,32 +243,30 @@ test_expect_success 'bisect skip: with commit both bad and skipped' '
 '
 
 # We want to automatically find the commit that
-# introduced "Another" into hello.
-test_expect_success \
-    '"git bisect run" simple case' \
-    'echo "#"\!"/bin/sh" > test_script.sh &&
-     echo "grep Another hello > /dev/null" >> test_script.sh &&
-     echo "test \$? -ne 0" >> test_script.sh &&
-     chmod +x test_script.sh &&
-     git bisect start &&
-     git bisect good $HASH1 &&
-     git bisect bad $HASH4 &&
-     git bisect run ./test_script.sh > my_bisect_log.txt &&
-     grep "$HASH3 is the first bad commit" my_bisect_log.txt &&
-     git bisect reset'
+# added "Another" into hello.
+test_expect_success '"git bisect run" simple case' '
+	write_script test_script.sh <<-\EOF &&
+	! grep Another hello >/dev/null
+	EOF
+	git bisect start &&
+	git bisect good $HASH1 &&
+	git bisect bad $HASH4 &&
+	git bisect run ./test_script.sh >my_bisect_log.txt &&
+	grep "$HASH3 is the first bad commit" my_bisect_log.txt &&
+	git bisect reset
+'
 
 # We want to automatically find the commit that
-# introduced "Ciao" into hello.
-test_expect_success \
-    '"git bisect run" with more complex "git bisect start"' \
-    'echo "#"\!"/bin/sh" > test_script.sh &&
-     echo "grep Ciao hello > /dev/null" >> test_script.sh &&
-     echo "test \$? -ne 0" >> test_script.sh &&
-     chmod +x test_script.sh &&
-     git bisect start $HASH4 $HASH1 &&
-     git bisect run ./test_script.sh > my_bisect_log.txt &&
-     grep "$HASH4 is the first bad commit" my_bisect_log.txt &&
-     git bisect reset'
+# added "Ciao" into hello.
+test_expect_success '"git bisect run" with more complex "git bisect start"' '
+	write_script test_script.sh <<-\EOF &&
+	! grep Ciao hello >/dev/null
+	EOF
+	git bisect start $HASH4 $HASH1 &&
+	git bisect run ./test_script.sh >my_bisect_log.txt &&
+	grep "$HASH4 is the first bad commit" my_bisect_log.txt &&
+	git bisect reset
+'
 
 # $HASH1 is good, $HASH5 is bad, we skip $HASH3
 # but $HASH4 is good,
@@ -295,24 +293,17 @@ HASH6=
 test_expect_success 'bisect run & skip: cannot tell between 2' '
 	add_line_into_file "6: Yet a line." hello &&
 	HASH6=$(git rev-parse --verify HEAD) &&
-	echo "#"\!"/bin/sh" > test_script.sh &&
-	echo "sed -ne \\\$p hello | grep Ciao > /dev/null && exit 125" >> test_script.sh &&
-	echo "grep line hello > /dev/null" >> test_script.sh &&
-	echo "test \$? -ne 0" >> test_script.sh &&
-	chmod +x test_script.sh &&
+	write_script test_script.sh <<-\EOF &&
+	sed -ne \$p hello | grep Ciao >/dev/null && exit 125
+	! grep line hello >/dev/null
+	EOF
 	git bisect start $HASH6 $HASH1 &&
-	if git bisect run ./test_script.sh > my_bisect_log.txt
-	then
-		echo Oops, should have failed.
-		false
-	else
-		test $? -eq 2 &&
-		grep "first bad commit could be any of" my_bisect_log.txt &&
-		! grep $HASH3 my_bisect_log.txt &&
-		! grep $HASH6 my_bisect_log.txt &&
-		grep $HASH4 my_bisect_log.txt &&
-		grep $HASH5 my_bisect_log.txt
-	fi
+	test_expect_code 2 git bisect run ./test_script.sh >my_bisect_log.txt &&
+	grep "first bad commit could be any of" my_bisect_log.txt &&
+	! grep $HASH3 my_bisect_log.txt &&
+	! grep $HASH6 my_bisect_log.txt &&
+	grep $HASH4 my_bisect_log.txt &&
+	grep $HASH5 my_bisect_log.txt
 '
 
 HASH7=
@@ -320,14 +311,13 @@ test_expect_success 'bisect run & skip: find first bad' '
 	git bisect reset &&
 	add_line_into_file "7: Should be the last line." hello &&
 	HASH7=$(git rev-parse --verify HEAD) &&
-	echo "#"\!"/bin/sh" > test_script.sh &&
-	echo "sed -ne \\\$p hello | grep Ciao > /dev/null && exit 125" >> test_script.sh &&
-	echo "sed -ne \\\$p hello | grep day > /dev/null && exit 125" >> test_script.sh &&
-	echo "grep Yet hello > /dev/null" >> test_script.sh &&
-	echo "test \$? -ne 0" >> test_script.sh &&
-	chmod +x test_script.sh &&
+	write_script test_script.sh <<-\EOF &&
+	sed -ne \$p hello | grep Ciao >/dev/null && exit 125
+	sed -ne \$p hello | grep day >/dev/null && exit 125
+	! grep Yet hello >/dev/null
+	EOF
 	git bisect start $HASH7 $HASH1 &&
-	git bisect run ./test_script.sh > my_bisect_log.txt &&
+	git bisect run ./test_script.sh >my_bisect_log.txt &&
 	grep "$HASH6 is the first bad commit" my_bisect_log.txt
 '
 
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v4 2/5] rev-list: allow bisect and first-parent flags
  2020-08-04 22:01     ` [PATCH v4 0/5] " Aaron Lipman
  2020-08-04 22:01       ` [PATCH v4 1/5] t6030: modernize "git bisect run" tests Aaron Lipman
@ 2020-08-04 22:01       ` Aaron Lipman
  2020-08-05  0:38         ` Eric Sunshine
  2020-08-04 22:01       ` [PATCH v4 3/5] cmd_bisect__helper: defer parsing no-checkout flag Aaron Lipman
                         ` (5 subsequent siblings)
  7 siblings, 1 reply; 32+ messages in thread
From: Aaron Lipman @ 2020-08-04 22:01 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman

Add first_parent_only parameter to find_bisection(), removing the
barrier that prevented combining the --bisect and --first-parent flags
when using git rev-list

Based-on-patch-by: Tiago Botelho <tiagonbotelho@hotmail.com>
Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 Documentation/rev-list-options.txt |  7 ++---
 bisect.c                           | 28 ++++++++++++-------
 bisect.h                           |  2 +-
 builtin/rev-list.c                 |  2 +-
 revision.c                         |  3 --
 t/t6000-rev-list-misc.sh           |  4 +--
 t/t6002-rev-list-bisect.sh         | 45 ++++++++++++++++++++++++++++++
 7 files changed, 70 insertions(+), 21 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index b01b2b6773..d3117ce51b 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -128,8 +128,7 @@ parents) and `--max-parents=-1` (negative numbers denote no upper limit).
 	because merges into a topic branch tend to be only about
 	adjusting to updated upstream from time to time, and
 	this option allows you to ignore the individual commits
-	brought in to your history by such a merge. Cannot be
-	combined with --bisect.
+	brought in to your history by such a merge.
 
 --not::
 	Reverses the meaning of the '{caret}' prefix (or lack thereof)
@@ -207,7 +206,7 @@ ifndef::git-rev-list[]
 	Pretend as if the bad bisection ref `refs/bisect/bad`
 	was listed and as if it was followed by `--not` and the good
 	bisection refs `refs/bisect/good-*` on the command
-	line. Cannot be combined with --first-parent.
+	line.
 endif::git-rev-list[]
 
 --stdin::
@@ -743,7 +742,7 @@ outputs 'midpoint', the output of the two commands
 would be of roughly the same length.  Finding the change which
 introduces a regression is thus reduced to a binary search: repeatedly
 generate and test new 'midpoint's until the commit chain is of length
-one. Cannot be combined with --first-parent.
+one.
 
 --bisect-vars::
 	This calculates the same as `--bisect`, except that refs in
diff --git a/bisect.c b/bisect.c
index d5e830410f..a11fdb1473 100644
--- a/bisect.c
+++ b/bisect.c
@@ -88,15 +88,16 @@ static inline void weight_set(struct commit_list *elem, int weight)
 	**commit_weight_at(&commit_weight, elem->item) = weight;
 }
 
-static int count_interesting_parents(struct commit *commit)
+static int count_interesting_parents(struct commit *commit, int first_parent_only)
 {
 	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 (first_parent_only)
+			break;
 	}
 	return count;
 }
@@ -259,7 +260,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 first_parent_only)
 {
 	int n, counted;
 	struct commit_list *p;
@@ -271,7 +272,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 		unsigned flags = commit->object.flags;
 
 		*commit_weight_at(&commit_weight, p->item) = &weights[n++];
-		switch (count_interesting_parents(commit)) {
+		switch (count_interesting_parents(commit, first_parent_only)) {
 		case 0:
 			if (!(flags & TREESAME)) {
 				weight_set(p, 1);
@@ -314,6 +315,8 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 			continue;
 		if (weight(p) != -2)
 			continue;
+		if (first_parent_only)
+			BUG("shouldn't be calling count-distance in fp mode");
 		weight_set(p, count_distance(p));
 		clear_distance(list);
 
@@ -332,7 +335,10 @@ 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) {
+
+			for (q = p->item->parents;
+			     q;
+			     q = first_parent_only ? NULL : q->next) {
 				if (q->item->object.flags & UNINTERESTING)
 					continue;
 				if (0 <= weight(q))
@@ -370,7 +376,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 }
 
 void find_bisection(struct commit_list **commit_list, int *reaches,
-		    int *all, int find_all)
+		    int *all, int find_all, int first_parent_only)
 {
 	int nr, on_list;
 	struct commit_list *list, *p, *best, *next, *last;
@@ -406,7 +412,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, first_parent_only);
 	if (best) {
 		if (!find_all) {
 			list->item = best->item;
@@ -991,6 +997,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
 	enum bisect_error res = BISECT_OK;
 	struct object_id *bisect_rev;
 	char *steps_msg;
+	int first_parent_only = 0; /* TODO: pass --first-parent flag from git bisect start */
 
 	read_bisect_terms(&term_bad, &term_good);
 	if (read_bisect_refs())
@@ -1001,11 +1008,12 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
 		return res;
 
 	bisect_rev_setup(r, &revs, prefix, "%s", "^%s", 1);
+	revs.first_parent_only = first_parent_only;
 	revs.limited = 1;
 
 	bisect_common(&revs);
 
-	find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr);
+	find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only);
 	revs.commits = managed_skipped(revs.commits, &tried);
 
 	if (!revs.commits) {
diff --git a/bisect.h b/bisect.h
index 8bad8d8391..a63af0505f 100644
--- a/bisect.h
+++ b/bisect.h
@@ -12,7 +12,7 @@ struct repository;
  * best commit, as chosen by `find_all`.
  */
 void find_bisection(struct commit_list **list, int *reaches, int *all,
-		    int find_all);
+		    int find_all, int first_parent_only);
 
 struct commit_list *filter_skipped(struct commit_list *list,
 				   struct commit_list **tried,
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index f520111eda..d1a14596b2 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -638,7 +638,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, revs.first_parent_only);
 
 		if (bisect_show_vars)
 			return show_bisect_vars(&info, reaches, all);
diff --git a/revision.c b/revision.c
index 6de29cdf7a..e628d5ea56 100644
--- a/revision.c
+++ b/revision.c
@@ -2891,9 +2891,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->line_level_traverse &&
 	    (revs->diffopt.output_format & ~(DIFF_FORMAT_PATCH | DIFF_FORMAT_NO_OUTPUT)))
 		die(_("-L does not yet support diff formats besides -p and -s"));
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 3bb0e4ff8f..fc4d55dcb2 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -128,8 +128,8 @@ test_expect_success 'rev-list can negate index objects' '
 	test_cmp expect actual
 '
 
-test_expect_success '--bisect and --first-parent can not be combined' '
-	test_must_fail git rev-list --bisect --first-parent HEAD
+test_expect_success '--bisect and --first-parent can be combined' '
+	git rev-list --bisect --first-parent HEAD
 '
 
 test_expect_success '--header shows a NUL after each commit' '
diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
index a661408038..7fc0f75ca5 100755
--- a/t/t6002-rev-list-bisect.sh
+++ b/t/t6002-rev-list-bisect.sh
@@ -263,4 +263,49 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' '
 	test_cmp expect.sorted actual.sorted
 '
 
+test_output_expect_success "--bisect --first-parent" 'git rev-list --bisect --first-parent E ^F' <<EOF
+e4
+EOF
+
+test_output_expect_success "--first-parent" 'git rev-list --first-parent E ^F' <<EOF
+E
+e1
+e2
+e3
+e4
+e5
+e6
+e7
+e8
+EOF
+
+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.unsorted <<EOF &&
+$(git rev-parse E) (tag: E, dist=0)
+$(git rev-parse e1) (tag: e1, dist=1)
+$(git rev-parse e2) (tag: e2, dist=2)
+$(git rev-parse e3) (tag: e3, dist=3)
+$(git rev-parse e4) (tag: e4, dist=4)
+$(git rev-parse e5) (tag: e5, dist=4)
+$(git rev-parse e6) (tag: e6, dist=3)
+$(git rev-parse e7) (tag: e7, dist=2)
+$(git rev-parse e8) (tag: e8, dist=1)
+EOF
+
+# expect results to be ordered by distance (descending),
+# commit hash (ascending)
+sort -k4,4r -k1,1 expect.unsorted > expect &&
+git rev-list --bisect-all --first-parent E ^F > actual &&
+test_cmp actual expect
+'
+
 test_done
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v4 3/5] cmd_bisect__helper: defer parsing no-checkout flag
  2020-08-04 22:01     ` [PATCH v4 0/5] " Aaron Lipman
  2020-08-04 22:01       ` [PATCH v4 1/5] t6030: modernize "git bisect run" tests Aaron Lipman
  2020-08-04 22:01       ` [PATCH v4 2/5] rev-list: allow bisect and first-parent flags Aaron Lipman
@ 2020-08-04 22:01       ` Aaron Lipman
  2020-08-04 22:01       ` [PATCH v4 4/5] bisect: introduce first-parent flag Aaron Lipman
                         ` (4 subsequent siblings)
  7 siblings, 0 replies; 32+ messages in thread
From: Aaron Lipman @ 2020-08-04 22:01 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman

cmd_bisect__helper() is intended as a temporary shim layer serving as an
interface for git-bisect.sh. This function and git-bisect.sh should
eventually be replaced by a C implementation, cmd_bisect(), serving as
an entrypoint for all "git bisect ..." shell commands: cmd_bisect() will
only parse the first token following "git bisect", and dispatch the
remaining args to the appropriate function ["bisect_start()",
"bisect_next()", etc.].

Thus, cmd_bisect__helper() should not be responsible for parsing flags
like --no-checkout. Instead, let the --no-checkout flag remain in the
argv array, so it may be evaluated alongside the other options already
parsed by bisect_start().

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 bisect.c                 |  3 ++-
 bisect.h                 |  4 +---
 builtin/bisect--helper.c | 14 ++++++--------
 git-bisect.sh            |  2 +-
 4 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/bisect.c b/bisect.c
index a11fdb1473..950ff6f533 100644
--- a/bisect.c
+++ b/bisect.c
@@ -989,7 +989,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.
  */
-enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
+enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
 {
 	struct rev_info revs;
 	struct commit_list *tried;
@@ -997,6 +997,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
 	enum bisect_error res = BISECT_OK;
 	struct object_id *bisect_rev;
 	char *steps_msg;
+	int no_checkout = ref_exists("BISECT_HEAD");
 	int first_parent_only = 0; /* TODO: pass --first-parent flag from git bisect start */
 
 	read_bisect_terms(&term_bad, &term_good);
diff --git a/bisect.h b/bisect.h
index a63af0505f..7f30b94db3 100644
--- a/bisect.h
+++ b/bisect.h
@@ -58,9 +58,7 @@ enum bisect_error {
 	BISECT_INTERNAL_SUCCESS_MERGE_BASE = -11
 };
 
-enum bisect_error bisect_next_all(struct repository *r,
-		    const char *prefix,
-		    int no_checkout);
+enum bisect_error bisect_next_all(struct repository *r, const char *prefix);
 
 int estimate_bisect_steps(int all);
 
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 73f9324ad7..95a15b918f 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -18,7 +18,7 @@ static GIT_PATH_FUNC(git_path_head_name, "head-name")
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 
 static const char * const git_bisect_helper_usage[] = {
-	N_("git bisect--helper --next-all [--no-checkout]"),
+	N_("git bisect--helper --next-all"),
 	N_("git bisect--helper --write-terms <bad_term> <good_term>"),
 	N_("git bisect--helper --bisect-clean-state"),
 	N_("git bisect--helper --bisect-reset [<commit>]"),
@@ -420,9 +420,9 @@ static int bisect_append_log_quoted(const char **argv)
 	return res;
 }
 
-static int bisect_start(struct bisect_terms *terms, int no_checkout,
-			const char **argv, int argc)
+static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
 {
+	int no_checkout = 0;
 	int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
 	int flags, pathspec_pos, res = 0;
 	struct string_list revs = STRING_LIST_INIT_DUP;
@@ -631,7 +631,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_TERMS,
 		BISECT_START
 	} cmdmode = 0;
-	int no_checkout = 0, res = 0, nolog = 0;
+	int res = 0, nolog = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &cmdmode,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
@@ -653,8 +653,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("print out the bisect terms"), BISECT_TERMS),
 		OPT_CMDMODE(0, "bisect-start", &cmdmode,
 			 N_("start the bisect session"), BISECT_START),
-		OPT_BOOL(0, "no-checkout", &no_checkout,
-			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_BOOL(0, "no-log", &nolog,
 			 N_("no log for BISECT_WRITE")),
 		OPT_END()
@@ -670,7 +668,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 
 	switch (cmdmode) {
 	case NEXT_ALL:
-		res = bisect_next_all(the_repository, prefix, no_checkout);
+		res = bisect_next_all(the_repository, prefix);
 		break;
 	case WRITE_TERMS:
 		if (argc != 2)
@@ -712,7 +710,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		break;
 	case BISECT_START:
 		set_terms(&terms, "bad", "good");
-		res = bisect_start(&terms, no_checkout, argv, argc);
+		res = bisect_start(&terms, argv, argc);
 		break;
 	default:
 		return error("BUG: unknown subcommand '%d'", cmdmode);
diff --git a/git-bisect.sh b/git-bisect.sh
index f03fbb18f0..c7580e51a0 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -153,7 +153,7 @@ bisect_next() {
 	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit
 
 	# Perform all bisection computation, display and checkout
-	git bisect--helper --next-all $(git rev-parse --verify -q BISECT_HEAD > /dev/null && echo --no-checkout)
+	git bisect--helper --next-all
 	res=$?
 
 	# Check if we should exit because bisection is finished
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v4 4/5] bisect: introduce first-parent flag
  2020-08-04 22:01     ` [PATCH v4 0/5] " Aaron Lipman
                         ` (2 preceding siblings ...)
  2020-08-04 22:01       ` [PATCH v4 3/5] cmd_bisect__helper: defer parsing no-checkout flag Aaron Lipman
@ 2020-08-04 22:01       ` Aaron Lipman
  2020-08-04 22:01       ` [PATCH v4 5/5] bisect: combine args passed to find_bisection() Aaron Lipman
                         ` (3 subsequent siblings)
  7 siblings, 0 replies; 32+ messages in thread
From: Aaron Lipman @ 2020-08-04 22:01 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman

Upon seeing a merge commit when bisecting, this option may be used to
follow only the first parent.

In detecting regressions introduced through the merging of a branch, the
merge commit will be identified as introduction of the bug and its
ancestors will be ignored.

This option is particularly useful in avoiding false positives when a
merged branch contained broken or non-buildable commits, but the merge
itself was OK.

Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 Documentation/git-bisect.txt | 13 ++++++++++++-
 bisect.c                     |  5 ++++-
 builtin/bisect--helper.c     |  9 ++++++++-
 t/t6030-bisect-porcelain.sh  | 18 ++++++++++++++++++
 4 files changed, 42 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index 7586c5a843..0e993e4587 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -17,7 +17,7 @@ The command takes various subcommands, and different options depending
 on the subcommand:
 
  git bisect start [--term-{old,good}=<term> --term-{new,bad}=<term>]
-		  [--no-checkout] [<bad> [<good>...]] [--] [<paths>...]
+		  [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]
  git bisect (bad|new|<term-new>) [<rev>]
  git bisect (good|old|<term-old>) [<rev>...]
  git bisect terms [--term-good | --term-bad]
@@ -365,6 +365,17 @@ does not require a checked out tree.
 +
 If the repository is bare, `--no-checkout` is assumed.
 
+--first-parent::
++
+Follow only the first parent commit upon seeing a merge commit.
++
+In detecting regressions introduced through the merging of a branch, the merge
+commit will be identified as introduction of the bug and its ancestors will be
+ignored.
++
+This option is particularly useful in avoiding false positives when a merged
+branch contained broken or non-buildable commits, but the merge itself was OK.
+
 EXAMPLES
 --------
 
diff --git a/bisect.c b/bisect.c
index 950ff6f533..bc4241b51f 100644
--- a/bisect.c
+++ b/bisect.c
@@ -15,6 +15,7 @@
 #include "commit-slab.h"
 #include "commit-reach.h"
 #include "object-store.h"
+#include "dir.h"
 
 static struct oid_array good_revs;
 static struct oid_array skipped_revs;
@@ -460,6 +461,7 @@ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
 static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
+static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
 static GIT_PATH_FUNC(git_path_head_name, "head-name")
 
 static void read_bisect_paths(struct argv_array *array)
@@ -998,7 +1000,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
 	struct object_id *bisect_rev;
 	char *steps_msg;
 	int no_checkout = ref_exists("BISECT_HEAD");
-	int first_parent_only = 0; /* TODO: pass --first-parent flag from git bisect start */
+	int first_parent_only = file_exists(git_path_bisect_first_parent());
 
 	read_bisect_terms(&term_bad, &term_good);
 	if (read_bisect_refs())
@@ -1142,6 +1144,7 @@ int bisect_clean_state(void)
 	unlink_or_warn(git_path_bisect_names());
 	unlink_or_warn(git_path_bisect_run());
 	unlink_or_warn(git_path_bisect_terms());
+	unlink_or_warn(git_path_bisect_first_parent());
 	/* Cleanup head-name if it got left by an old version of git-bisect */
 	unlink_or_warn(git_path_head_name());
 	/*
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 95a15b918f..a055147f19 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -16,6 +16,7 @@ static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
 static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 static GIT_PATH_FUNC(git_path_head_name, "head-name")
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
+static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all"),
@@ -27,7 +28,7 @@ static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --bisect-next-check <good_term> <bad_term> [<term>]"),
 	N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
 	N_("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]"
-					     "[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
+					    " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"),
 	NULL
 };
 
@@ -423,6 +424,7 @@ static int bisect_append_log_quoted(const char **argv)
 static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
 {
 	int no_checkout = 0;
+	int first_parent_only = 0;
 	int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
 	int flags, pathspec_pos, res = 0;
 	struct string_list revs = STRING_LIST_INIT_DUP;
@@ -452,6 +454,8 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
 			break;
 		} else if (!strcmp(arg, "--no-checkout")) {
 			no_checkout = 1;
+		} else if (!strcmp(arg, "--first-parent")) {
+			first_parent_only = 1;
 		} else if (!strcmp(arg, "--term-good") ||
 			 !strcmp(arg, "--term-old")) {
 			i++;
@@ -576,6 +580,9 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
 	 */
 	write_file(git_path_bisect_start(), "%s\n", start_head.buf);
 
+	if (first_parent_only)
+		write_file(git_path_bisect_first_parent(), "\n");
+
 	if (no_checkout) {
 		if (get_oid(start_head.buf, &oid) < 0) {
 			res = error(_("invalid ref: '%s'"), start_head.buf);
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index a66c4b89bc..b886529e59 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -448,6 +448,24 @@ test_expect_success 'many merge bases creation' '
 	grep "$SIDE_HASH5" merge_bases.txt
 '
 
+# We want to automatically find the merge that
+# added "line" into hello.
+test_expect_success '"git bisect run --first-parent" simple case' '
+	git rev-list --first-parent $B_HASH ^$HASH4 >first_parent_chain.txt &&
+	write_script test_script.sh <<-\EOF &&
+	grep $(git rev-parse HEAD) first_parent_chain.txt || exit -1
+	! grep line hello >/dev/null
+	EOF
+	git bisect start --first-parent &&
+	test_path_is_file ".git/BISECT_FIRST_PARENT" &&
+	git bisect good $HASH4 &&
+	git bisect bad $B_HASH &&
+	git bisect run ./test_script.sh >my_bisect_log.txt &&
+	grep "$B_HASH is the first bad commit" my_bisect_log.txt &&
+	git bisect reset &&
+	test_path_is_missing .git/BISECT_FIRST_PARENT
+'
+
 test_expect_success 'good merge bases when good and bad are siblings' '
 	git bisect start "$B_HASH" "$A_HASH" > my_bisect_log.txt &&
 	test_i18ngrep "merge base must be tested" my_bisect_log.txt &&
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 32+ messages in thread

* [PATCH v4 5/5] bisect: combine args passed to find_bisection()
  2020-08-04 22:01     ` [PATCH v4 0/5] " Aaron Lipman
                         ` (3 preceding siblings ...)
  2020-08-04 22:01       ` [PATCH v4 4/5] bisect: introduce first-parent flag Aaron Lipman
@ 2020-08-04 22:01       ` Aaron Lipman
  2020-08-04 22:19       ` [PATCH v4 0/5] Introduce --first-parent flag for git bisect Junio C Hamano
                         ` (2 subsequent siblings)
  7 siblings, 0 replies; 32+ messages in thread
From: Aaron Lipman @ 2020-08-04 22:01 UTC (permalink / raw)
  To: git; +Cc: Aaron Lipman

Now that find_bisection() accepts multiple boolean arguments, these may
be combined into a single unsigned integer in order to declutter some of
the code in bisect.c

Also, rename the existing "flags" bitfield to "commit_flags", to
explicitly differentiate it from the new "bisect_flags" bitfield.

Based-on-patch-by: Harald Nordgren <haraldnordgren@gmail.com>
Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
 bisect.c           | 67 +++++++++++++++++++++++++---------------------
 bisect.h           |  5 +++-
 builtin/rev-list.c |  9 ++++++-
 3 files changed, 49 insertions(+), 32 deletions(-)

diff --git a/bisect.c b/bisect.c
index bc4241b51f..1585fcc6ad 100644
--- a/bisect.c
+++ b/bisect.c
@@ -89,7 +89,7 @@ static inline void weight_set(struct commit_list *elem, int weight)
 	**commit_weight_at(&commit_weight, elem->item) = weight;
 }
 
-static int count_interesting_parents(struct commit *commit, int first_parent_only)
+static int count_interesting_parents(struct commit *commit, unsigned bisect_flags)
 {
 	struct commit_list *p;
 	int count;
@@ -97,7 +97,7 @@ static int count_interesting_parents(struct commit *commit, int first_parent_onl
 	for (count = 0, p = commit->parents; p; p = p->next) {
 		if (!(p->item->object.flags & UNINTERESTING))
 			count++;
-		if (first_parent_only)
+		if (bisect_flags & FIND_BISECTION_FIRST_PARENT_ONLY)
 			break;
 	}
 	return count;
@@ -137,7 +137,7 @@ static void show_list(const char *debug, int counted, int nr,
 	for (p = list; p; p = p->next) {
 		struct commit_list *pp;
 		struct commit *commit = p->item;
-		unsigned flags = commit->object.flags;
+		unsigned commit_flags = commit->object.flags;
 		enum object_type type;
 		unsigned long size;
 		char *buf = read_object_file(&commit->object.oid, &type,
@@ -146,9 +146,9 @@ static void show_list(const char *debug, int counted, int nr,
 		int subject_len;
 
 		fprintf(stderr, "%c%c%c ",
-			(flags & TREESAME) ? ' ' : 'T',
-			(flags & UNINTERESTING) ? 'U' : ' ',
-			(flags & COUNTED) ? 'C' : ' ');
+			(commit_flags & TREESAME) ? ' ' : 'T',
+			(commit_flags & UNINTERESTING) ? 'U' : ' ',
+			(commit_flags & COUNTED) ? 'C' : ' ');
 		if (*commit_weight_at(&commit_weight, p->item))
 			fprintf(stderr, "%3d", weight(p));
 		else
@@ -173,9 +173,9 @@ static struct commit_list *best_bisection(struct commit_list *list, int nr)
 	best = list;
 	for (p = list; p; p = p->next) {
 		int distance;
-		unsigned flags = p->item->object.flags;
+		unsigned commit_flags = p->item->object.flags;
 
-		if (flags & TREESAME)
+		if (commit_flags & TREESAME)
 			continue;
 		distance = weight(p);
 		if (nr - distance < distance)
@@ -214,9 +214,9 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n
 
 	for (p = list, cnt = 0; p; p = p->next) {
 		int distance;
-		unsigned flags = p->item->object.flags;
+		unsigned commit_flags = p->item->object.flags;
 
-		if (flags & TREESAME)
+		if (commit_flags & TREESAME)
 			continue;
 		distance = weight(p);
 		if (nr - distance < distance)
@@ -261,7 +261,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 first_parent_only)
+					     unsigned bisect_flags)
 {
 	int n, counted;
 	struct commit_list *p;
@@ -270,12 +270,12 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 
 	for (n = 0, p = list; p; p = p->next) {
 		struct commit *commit = p->item;
-		unsigned flags = commit->object.flags;
+		unsigned commit_flags = commit->object.flags;
 
 		*commit_weight_at(&commit_weight, p->item) = &weights[n++];
-		switch (count_interesting_parents(commit, first_parent_only)) {
+		switch (count_interesting_parents(commit, bisect_flags)) {
 		case 0:
-			if (!(flags & TREESAME)) {
+			if (!(commit_flags & TREESAME)) {
 				weight_set(p, 1);
 				counted++;
 				show_list("bisection 2 count one",
@@ -316,13 +316,13 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 			continue;
 		if (weight(p) != -2)
 			continue;
-		if (first_parent_only)
+		if (bisect_flags & FIND_BISECTION_FIRST_PARENT_ONLY)
 			BUG("shouldn't be calling count-distance in fp mode");
 		weight_set(p, count_distance(p));
 		clear_distance(list);
 
 		/* Does it happen to be at exactly half-way? */
-		if (!find_all && halfway(p, nr))
+		if (!(bisect_flags & FIND_BISECTION_ALL) && halfway(p, nr))
 			return p;
 		counted++;
 	}
@@ -332,14 +332,14 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 	while (counted < nr) {
 		for (p = list; p; p = p->next) {
 			struct commit_list *q;
-			unsigned flags = p->item->object.flags;
+			unsigned commit_flags = p->item->object.flags;
 
 			if (0 <= weight(p))
 				continue;
 
 			for (q = p->item->parents;
 			     q;
-			     q = first_parent_only ? NULL : q->next) {
+			     q = bisect_flags & FIND_BISECTION_FIRST_PARENT_ONLY ? NULL : q->next) {
 				if (q->item->object.flags & UNINTERESTING)
 					continue;
 				if (0 <= weight(q))
@@ -353,7 +353,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 			 * add one for p itself if p is to be counted,
 			 * otherwise inherit it from q directly.
 			 */
-			if (!(flags & TREESAME)) {
+			if (!(commit_flags & TREESAME)) {
 				weight_set(p, weight(q)+1);
 				counted++;
 				show_list("bisection 2 count one",
@@ -363,21 +363,21 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
 				weight_set(p, weight(q));
 
 			/* Does it happen to be at exactly half-way? */
-			if (!find_all && halfway(p, nr))
+			if (!(bisect_flags & FIND_BISECTION_ALL) && halfway(p, nr))
 				return p;
 		}
 	}
 
 	show_list("bisection 2 counted all", counted, nr, list);
 
-	if (!find_all)
+	if (!(bisect_flags & FIND_BISECTION_ALL))
 		return best_bisection(list, nr);
 	else
 		return best_bisection_sorted(list, nr);
 }
 
 void find_bisection(struct commit_list **commit_list, int *reaches,
-		    int *all, int find_all, int first_parent_only)
+		    int *all, unsigned bisect_flags)
 {
 	int nr, on_list;
 	struct commit_list *list, *p, *best, *next, *last;
@@ -393,16 +393,16 @@ void find_bisection(struct commit_list **commit_list, int *reaches,
 	for (nr = on_list = 0, last = NULL, p = *commit_list;
 	     p;
 	     p = next) {
-		unsigned flags = p->item->object.flags;
+		unsigned commit_flags = p->item->object.flags;
 
 		next = p->next;
-		if (flags & UNINTERESTING) {
+		if (commit_flags & UNINTERESTING) {
 			free(p);
 			continue;
 		}
 		p->next = last;
 		last = p;
-		if (!(flags & TREESAME))
+		if (!(commit_flags & TREESAME))
 			nr++;
 		on_list++;
 	}
@@ -413,9 +413,9 @@ 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, first_parent_only);
+	best = do_find_bisection(list, nr, weights, bisect_flags);
 	if (best) {
-		if (!find_all) {
+		if (!(bisect_flags & FIND_BISECTION_ALL)) {
 			list->item = best->item;
 			free_commit_list(list->next);
 			best = list;
@@ -1000,23 +1000,30 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
 	struct object_id *bisect_rev;
 	char *steps_msg;
 	int no_checkout = ref_exists("BISECT_HEAD");
-	int first_parent_only = file_exists(git_path_bisect_first_parent());
+	unsigned bisect_flags = 0;
 
 	read_bisect_terms(&term_bad, &term_good);
 	if (read_bisect_refs())
 		die(_("reading bisect refs failed"));
 
+	if (file_exists(git_path_bisect_first_parent()))
+		bisect_flags |= FIND_BISECTION_FIRST_PARENT_ONLY;
+
+	if (skipped_revs.nr)
+		bisect_flags |= FIND_BISECTION_ALL;
+
 	res = check_good_are_ancestors_of_bad(r, prefix, no_checkout);
 	if (res)
 		return res;
 
 	bisect_rev_setup(r, &revs, prefix, "%s", "^%s", 1);
-	revs.first_parent_only = first_parent_only;
+
+	revs.first_parent_only = !!(bisect_flags & FIND_BISECTION_FIRST_PARENT_ONLY);
 	revs.limited = 1;
 
 	bisect_common(&revs);
 
-	find_bisection(&revs.commits, &reaches, &all, !!skipped_revs.nr, first_parent_only);
+	find_bisection(&revs.commits, &reaches, &all, bisect_flags);
 	revs.commits = managed_skipped(revs.commits, &tried);
 
 	if (!revs.commits) {
diff --git a/bisect.h b/bisect.h
index 7f30b94db3..ec24ac2d7e 100644
--- a/bisect.h
+++ b/bisect.h
@@ -12,7 +12,7 @@ struct repository;
  * best commit, as chosen by `find_all`.
  */
 void find_bisection(struct commit_list **list, int *reaches, int *all,
-		    int find_all, int first_parent_only);
+		    unsigned bisect_flags);
 
 struct commit_list *filter_skipped(struct commit_list *list,
 				   struct commit_list **tried,
@@ -23,6 +23,9 @@ struct commit_list *filter_skipped(struct commit_list *list,
 #define BISECT_SHOW_ALL		(1<<0)
 #define REV_LIST_QUIET		(1<<1)
 
+#define FIND_BISECTION_ALL			(1u<<0)
+#define FIND_BISECTION_FIRST_PARENT_ONLY	(1u<<1)
+
 struct rev_list_info {
 	struct rev_info *revs;
 	int flags;
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index d1a14596b2..25c6c3b38d 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -637,8 +637,15 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
 
 	if (bisect_list) {
 		int reaches, all;
+		unsigned bisect_flags = 0;
 
-		find_bisection(&revs.commits, &reaches, &all, bisect_find_all, revs.first_parent_only);
+		if (bisect_find_all)
+			bisect_flags |= FIND_BISECTION_ALL;
+
+		if (revs.first_parent_only)
+			bisect_flags |= FIND_BISECTION_FIRST_PARENT_ONLY;
+
+		find_bisection(&revs.commits, &reaches, &all, bisect_flags);
 
 		if (bisect_show_vars)
 			return show_bisect_vars(&info, reaches, all);
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 0/5] Introduce --first-parent flag for git bisect
  2020-08-04 22:01     ` [PATCH v4 0/5] " Aaron Lipman
                         ` (4 preceding siblings ...)
  2020-08-04 22:01       ` [PATCH v4 5/5] bisect: combine args passed to find_bisection() Aaron Lipman
@ 2020-08-04 22:19       ` Junio C Hamano
  2020-08-05  5:55       ` Christian Couder
  2020-08-05  6:18       ` Martin Ågren
  7 siblings, 0 replies; 32+ messages in thread
From: Junio C Hamano @ 2020-08-04 22:19 UTC (permalink / raw)
  To: Aaron Lipman; +Cc: git

Aaron Lipman <alipman88@gmail.com> writes:

> OK, here's take 4! Responding to Junio's feedback, first:
>
>> That function [cmd_bisect__helper()] is supposed to be a thin shim
>> layer whose only reason of its existence is to serve as an interface
>> with the scripted version of "git bisect".  When everything is
>> migrated from the shell script, cmd_bisect__helper() should disappear.
>> ...
>> This is going backwards, as far as I can tell.  If anything, I'd
>> rather see cmd_bisect__helper() get fixed so that it does not parse
>> "--first-parent" (and similarly, "--no-checkout" that you imitated)
>> into first_parent_only (and no_checkout) variables and passed as
>> parameters to bisect_start().
>
> Thanks for the explanation, Junio. (And for bearing with me while I
> gain familiarity with git's codebase.) Now that I've taken some time
> to examine how git-bisect.sh and cmd_bisect__helper() fit together,
> the correct approach is much more clear.

I did notice that no_checkout variable in the top-level
cmd_bisect__helper() was convenient to have in the current code
structure, and I didn't think how it should be fixed deeply enough,
so for the purpose of this series, I do not mind if it is left
untouched.  It's just that it was disturbing to see that addition of
"--first-parent" wanted to add yet another variable to the
top-level, and that the new variable was totally unneeded.

> As no_checkout was also passed to bisect_next_all() [via implicitly
> checking for the existence of .git/BISECT_HEAD when calling
> bisect_next() in git-bisect.sh], I've removed that parameter and
> instead check for .git/BISECT_HEAD in bisect_next_all() to determine
> whether no-checkout mode is on.
>
> This means no-checkout mode can no longer be enabled by "git
> bisect--helper --next-all --no-checkout"

Unlike "start", "next-all" is not really a command in the end-user's
mind ("git bisect next" is, though).  It merely is just an interim
"convenience" interface for the remaining part of the scripted "git
bisect" to call into.  As long as the caller can do the same thing
as before, it should be OK, I would think.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 2/5] rev-list: allow bisect and first-parent flags
  2020-08-04 22:01       ` [PATCH v4 2/5] rev-list: allow bisect and first-parent flags Aaron Lipman
@ 2020-08-05  0:38         ` Eric Sunshine
  0 siblings, 0 replies; 32+ messages in thread
From: Eric Sunshine @ 2020-08-05  0:38 UTC (permalink / raw)
  To: Aaron Lipman; +Cc: Git List

On Tue, Aug 4, 2020 at 6:01 PM Aaron Lipman <alipman88@gmail.com> wrote:
> Add first_parent_only parameter to find_bisection(), removing the
> barrier that prevented combining the --bisect and --first-parent flags
> when using git rev-list
>
> Signed-off-by: Aaron Lipman <alipman88@gmail.com>
> ---
> diff --git a/t/t6002-rev-list-bisect.sh b/t/t6002-rev-list-bisect.sh
> @@ -263,4 +263,49 @@ test_expect_success 'rev-parse --bisect can default to good/bad refs' '
> +test_output_expect_success "--bisect --first-parent" 'git rev-list --bisect --first-parent E ^F' <<EOF

Custom is to use single quotes around the test title, not double
quotes (unless you need to interpolate a variable). Same comment
applies to other tests introduced by this patch.

> +test_expect_success "--bisect-all --first-parent" '
> +cat > expect.unsorted <<EOF &&
> +$(git rev-parse E) (tag: E, dist=0)
> +[...]
> +$(git rev-parse e8) (tag: e8, dist=1)
> +EOF
> +
> +# expect results to be ordered by distance (descending),
> +# commit hash (ascending)
> +sort -k4,4r -k1,1 expect.unsorted > expect &&
> +git rev-list --bisect-all --first-parent E ^F > actual &&
> +test_cmp actual expect
> +'

Indent this test body with TAB rather than having it sit at the left
margin. You can use <<-EOF for the here-doc to allow its content to be
indented, as well.

Swap the arguments to test_cmp.

Also, for style consistency, no whitespace following redirect:

    cat >expect.unsorted <<-EOF
    ...
    EOF
    ...
    sort ... >expect &&
    git rev-list ... >actual &&
    test_cmp expect actual

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 0/5] Introduce --first-parent flag for git bisect
  2020-08-04 22:01     ` [PATCH v4 0/5] " Aaron Lipman
                         ` (5 preceding siblings ...)
  2020-08-04 22:19       ` [PATCH v4 0/5] Introduce --first-parent flag for git bisect Junio C Hamano
@ 2020-08-05  5:55       ` Christian Couder
  2020-08-05  6:18       ` Martin Ågren
  7 siblings, 0 replies; 32+ messages in thread
From: Christian Couder @ 2020-08-05  5:55 UTC (permalink / raw)
  To: Aaron Lipman; +Cc: git, Junio C Hamano, Martin Ågren

On Wed, Aug 5, 2020 at 12:04 AM Aaron Lipman <alipman88@gmail.com> wrote:
>
> OK, here's take 4! Responding to Junio's feedback, first:

[...]

> Martin, thanks for your suggestions

[...]

It's better to have the people you are replying to as recipients of
your emails (in the "To:" field). I have added them into "Cc:".

> > (Signed-off-by: Martin Ågren <martin.agren@gmail.com>, FWIW.)
>
> I'm still getting used to the conventions - should I add your name as
> a signed-off-by tag, a thanks-to tag, or both?

We often use the following trailers:

- "Helped-by:" when someone helped you
- "Suggested-by:" when someone suggested the main idea in the patch
- "Reported-by:" when someone reported an issue fixed by the patch
- "Acked-by:" when someone explicitly acked the patch
- "Reviewed-by:" when someone explicitly gave their "Reviewed-by:"

If your patch is based on a patch from someone else, you can also keep
the "Signed-off-by:" and other trailers that the person already put in
the commit message. If you haven't made a lot of changes to a patch
initially from someone else you can also keep them as the author.

Thanks,
Christian.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 1/5] t6030: modernize "git bisect run" tests
  2020-08-04 22:01       ` [PATCH v4 1/5] t6030: modernize "git bisect run" tests Aaron Lipman
@ 2020-08-05  6:11         ` Christian Couder
  0 siblings, 0 replies; 32+ messages in thread
From: Christian Couder @ 2020-08-05  6:11 UTC (permalink / raw)
  To: Aaron Lipman; +Cc: git

On Wed, Aug 5, 2020 at 12:03 AM Aaron Lipman <alipman88@gmail.com> wrote:
>
> Enforce consistent styling for tests on "git bisect run":
> - Use "write_script" to abstract away platform-specific details.
> - Favor current whitespace conventions.
> - While at it, change "introduced" to "added" in the comments to make
>   them read better.
>
> Thanks-to: Martin Ågren <martin.agren@gmail.com>

'Helped-by:' and 'Suggested-by:' are much more often used than
'Thanks-to:'. There are approximately 1182, 247 and 69 occurrences of
them respectively.

> Signed-off-by: Aaron Lipman <alipman88@gmail.com>

Anyway the patch looks good to me.

Thanks,
Christian.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH v4 0/5] Introduce --first-parent flag for git bisect
  2020-08-04 22:01     ` [PATCH v4 0/5] " Aaron Lipman
                         ` (6 preceding siblings ...)
  2020-08-05  5:55       ` Christian Couder
@ 2020-08-05  6:18       ` Martin Ågren
  7 siblings, 0 replies; 32+ messages in thread
From: Martin Ågren @ 2020-08-05  6:18 UTC (permalink / raw)
  To: Aaron Lipman; +Cc: Git Mailing List, Christian Couder

Hi Aaron,

By the way, welcome to the list!

On Wed, 5 Aug 2020 at 00:02, Aaron Lipman <alipman88@gmail.com> wrote:
>
> Martin, thanks for your suggestions - I've moved the commit updating
> "git bisect run" tests to 1/5, updated the commit message, and
> included the changes you provided. (I held off on additional
> indentation corrections, as it kinda felt like unnecessary code
> churn/outside the scope of abstracting platform-specific details.)

Ok, great, I think it's fine to stop there. Those spots really are in a
much better shape now, thanks.

> > (Signed-off-by: Martin Ågren <martin.agren@gmail.com>, FWIW.)
>
> I'm still getting used to the conventions - should I add your name as
> a signed-off-by tag, a thanks-to tag, or both?

What I meant was "oh, and here's my Signed-off-by for the record" so
that we wouldn't need a round-trip of "cool, can I have your S-o-b on
that?".  Of course, I expressed that in such a lousy, compact way that
we needed a round-trip *anyway*. :-/

And I'm not even sure we need a "Signed-off-by". I just posted the patch
form of "If you look for chmod, you'll find a few more spots where you
can do the exact same transformation, and you might want to look up
test_expect_code."

I see you added "Thanks-to" and I would have been fine with no mention
at all. I think that patch looks good now, thanks. No need to think
about the trailers for that patch now, I think you can safely focus on
the later patches from now on. ;-)

Martin

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, back to index

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-28 15:44 [PATCH 0/3] Introduce --first-parent flag for git bisect Aaron Lipman via GitGitGadget
2020-07-28 15:44 ` [PATCH 1/3] rev-list: allow bisect and first-parent flags Aaron Lipman via GitGitGadget
2020-07-28 20:23   ` Junio C Hamano
2020-07-28 21:53     ` Junio C Hamano
2020-07-28 15:44 ` [PATCH 2/3] bisect: introduce first-parent flag Aaron Lipman via GitGitGadget
2020-07-28 15:44 ` [PATCH 3/3] bisect: combine args passed to find_bisection() Aaron Lipman via GitGitGadget
2020-07-30  0:27 ` [PATCH v2 0/3] Introduce --first-parent flag for git bisect Aaron Lipman
2020-07-30  0:27   ` [PATCH v2 1/3] rev-list: allow bisect and first-parent flags Aaron Lipman
2020-07-30  0:27   ` [PATCH v2 2/3] bisect: introduce first-parent flag Aaron Lipman
2020-07-30  4:17     ` Junio C Hamano
2020-07-30  0:27   ` [PATCH v2 3/3] bisect: combine args passed to find_bisection() Aaron Lipman
2020-07-30  4:32     ` Junio C Hamano
2020-07-30  0:48   ` [PATCH v2 0/3] Introduce --first-parent flag for git bisect Junio C Hamano
2020-08-01 17:58   ` [PATCH v3 0/4] " Aaron Lipman
2020-08-01 17:58     ` [PATCH v3 1/4] rev-list: allow bisect and first-parent flags Aaron Lipman
2020-08-01 17:58     ` [PATCH v3 2/4] bisect: introduce first-parent flag Aaron Lipman
2020-08-01 17:58     ` [PATCH v3 3/4] bisect: combine args passed to find_bisection() Aaron Lipman
2020-08-01 19:30       ` Martin Ågren
2020-08-01 17:58     ` [PATCH v3 4/4] bisect: consistent style for git bisect run tests Aaron Lipman
2020-08-01 19:27       ` Martin Ågren
2020-08-01 19:06     ` [PATCH v3 0/4] Introduce --first-parent flag for git bisect Junio C Hamano
2020-08-04 22:01     ` [PATCH v4 0/5] " Aaron Lipman
2020-08-04 22:01       ` [PATCH v4 1/5] t6030: modernize "git bisect run" tests Aaron Lipman
2020-08-05  6:11         ` Christian Couder
2020-08-04 22:01       ` [PATCH v4 2/5] rev-list: allow bisect and first-parent flags Aaron Lipman
2020-08-05  0:38         ` Eric Sunshine
2020-08-04 22:01       ` [PATCH v4 3/5] cmd_bisect__helper: defer parsing no-checkout flag Aaron Lipman
2020-08-04 22:01       ` [PATCH v4 4/5] bisect: introduce first-parent flag Aaron Lipman
2020-08-04 22:01       ` [PATCH v4 5/5] bisect: combine args passed to find_bisection() Aaron Lipman
2020-08-04 22:19       ` [PATCH v4 0/5] Introduce --first-parent flag for git bisect Junio C Hamano
2020-08-05  5:55       ` Christian Couder
2020-08-05  6:18       ` Martin Ågren

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git