# [PATCH v16 2/6] bisect--helper: rewrite `check_term_format` shell function in C

18 messages from 2017-09-29 to 2017-11-20. Participants: Pranit Bauva, Stephan Beyer, Ramsay Jones, Junio C Hamano, Christian Couder.
Thread: https://gitlist.dev/t/46857

## Pranit Bauva, 2017-09-29 06:49

Subject: [PATCH v16 2/6] bisect--helper: rewrite `check_term_format` shell function in C
Message-ID: <0102015ecc65d728-bea21919-4d18-48a1-b5c1-1c2ba4a136dc-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102015ecc65d728-bea21919-4d18-48a1-b5c1-1c2ba4a136dc-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102015ecc65d695-22151d3b-752b-4c10-a3a3-b8ef52491664-000000@eu-west-1.amazonses.com>

```
Reimplement the `check_term_format` shell function in C and add
a `--check-term-format` subcommand to `git bisect--helper` to call it
from git-bisect.sh

Using `--check-term-format` subcommand is a temporary measure to port
shell function to C so as to use the existing test suite. As more
functions are ported, this subcommand will be retired and its
implementation will be called by some other method/subcommand. For
eg. In conversion of write_terms() of git-bisect.sh, the subcommand will
be removed and instead check_term_format() will be called in its C
implementation while a new subcommand will be introduced for write_terms().

Helped-by: Johannes Schindelein <Johannes.Schindelein@gmx.de>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 31 ++-----------------------
 2 files changed, 61 insertions(+), 30 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index b6ee0acb82765..db9091e249454 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -2,19 +2,73 @@
 #include "cache.h"
 #include "parse-options.h"
 #include "bisect.h"
+#include "refs.h"
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
+	N_("git bisect--helper --check-term-format <term> <orig_term>"),
 	NULL
 };
 
+/*
+ * Check whether the string `term` belongs to the set of strings
+ * included in the variable arguments.
+ */
+LAST_ARG_MUST_BE_NULL
+static int one_of(const char *term, ...)
+{
+	int res = 0;
+	va_list matches;
+	const char *match;
+
+	va_start(matches, term);
+	while (!res && (match = va_arg(matches, const char *)))
+		res = !strcmp(term, match);
+	va_end(matches);
+
+	return res;
+}
+
+static int check_term_format(const char *term, const char *orig_term)
+{
+	int res;
+	char *new_term = xstrfmt("refs/bisect/%s", term);
+
+	res = check_refname_format(new_term, 0);
+	free(new_term);
+
+	if (res)
+		return error(_("'%s' is not a valid term"), term);
+
+	if (one_of(term, "help", "start", "skip", "next", "reset",
+			"visualize", "replay", "log", "run", "terms", NULL))
+		return error(_("can't use the builtin command '%s' as a term"), term);
+
+	/*
+	 * In theory, nothing prevents swapping completely good and bad,
+	 * but this situation could be confusing and hasn't been tested
+	 * enough. Forbid it for now.
+	 */
+
+	if ((strcmp(orig_term, "bad") && one_of(term, "bad", "new", NULL)) ||
+		 (strcmp(orig_term, "good") && one_of(term, "good", "old", NULL)))
+		return error(_("can't change the meaning of the term '%s'"), term);
+
+	return 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-	enum { NEXT_ALL = 1 } cmdmode = 0;
+	enum {
+		NEXT_ALL = 1,
+		CHECK_TERM_FMT
+	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &cmdmode,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
+		OPT_CMDMODE(0, "check-term-format", &cmdmode,
+			 N_("check format of the term"), CHECK_TERM_FMT),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -29,6 +83,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	switch (cmdmode) {
 	case NEXT_ALL:
 		return bisect_next_all(prefix, no_checkout);
+	case CHECK_TERM_FMT:
+		if (argc != 2)
+			return error(_("--check-term-format requires two arguments"));
+		return check_term_format(argv[0], argv[1]);
 	default:
 		return error("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index ae3cb013e7d56..a727c59250f13 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -564,38 +564,11 @@ write_terms () {
 	then
 		die "$(gettext "please use two different terms")"
 	fi
-	check_term_format "$TERM_BAD" bad
-	check_term_format "$TERM_GOOD" good
+	git bisect--helper --check-term-format "$TERM_BAD" bad || exit
+	git bisect--helper --check-term-format "$TERM_GOOD" good || exit
 	printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
 }
 
-check_term_format () {
-	term=$1
-	git check-ref-format refs/bisect/"$term" ||
-	die "$(eval_gettext "'\$term' is not a valid term")"
-	case "$term" in
-	help|start|terms|skip|next|reset|visualize|replay|log|run)
-		die "$(eval_gettext "can't use the builtin command '\$term' as a term")"
-		;;
-	bad|new)
-		if test "$2" != bad
-		then
-			# In theory, nothing prevents swapping
-			# completely good and bad, but this situation
-			# could be confusing and hasn't been tested
-			# enough. Forbid it for now.
-			die "$(eval_gettext "can't change the meaning of term '\$term'")"
-		fi
-		;;
-	good|old)
-		if test "$2" != good
-		then
-			die "$(eval_gettext "can't change the meaning of term '\$term'")"
-		fi
-		;;
-	esac
-}
-
 check_and_set_terms () {
 	cmd="$1"
 	case "$cmd" in

--
https://github.com/git/git/pull/410

```

## Pranit Bauva, 2017-09-29 06:49

Subject: [PATCH v16 5/6] t6030: explicitly test for bisection cleanup
Message-ID: <0102015ecc65d757-f89b00fc-a69c-46f0-a06e-e173c3bf0614-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102015ecc65d757-f89b00fc-a69c-46f0-a06e-e173c3bf0614-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102015ecc65d695-22151d3b-752b-4c10-a3a3-b8ef52491664-000000@eu-west-1.amazonses.com>

```
Add test to explicitly check that 'git bisect reset' is working as
expected. This is already covered implicitly by the test suite.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

---
I faced this problem while converting `bisect_clean_state` and the tests
where showing breakages but it wasn't clear as to where exactly are they
breaking. This will patch  will help in that. Also I tested the test
coverage of the test suite before this patch and it covers this (I did
this by purposely changing names of files in git-bisect.sh and running
the test suite).
---
 t/t6030-bisect-porcelain.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 8c2c6eaef83fe..f84ff941c3624 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -894,4 +894,21 @@ test_expect_success 'bisect start takes options and revs in any order' '
 	test_cmp expected actual
 '
 
+test_expect_success 'git bisect reset cleans bisection state properly' '
+	git bisect reset &&
+	git bisect start &&
+	git bisect good $HASH1 &&
+	git bisect bad $HASH4 &&
+	git bisect reset &&
+	test -z "$(git for-each-ref "refs/bisect/*")" &&
+	test_path_is_missing "$GIT_DIR/BISECT_EXPECTED_REV" &&
+	test_path_is_missing "$GIT_DIR/BISECT_ANCESTORS_OK" &&
+	test_path_is_missing "$GIT_DIR/BISECT_LOG" &&
+	test_path_is_missing "$GIT_DIR/BISECT_RUN" &&
+	test_path_is_missing "$GIT_DIR/BISECT_TERMS" &&
+	test_path_is_missing "$GIT_DIR/head-name" &&
+	test_path_is_missing "$GIT_DIR/BISECT_HEAD" &&
+	test_path_is_missing "$GIT_DIR/BISECT_START"
+'
+
 test_done

--
https://github.com/git/git/pull/410

```

## Pranit Bauva, 2017-09-29 06:49

Subject: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Message-ID: <0102015ecc65d695-22151d3b-752b-4c10-a3a3-b8ef52491664-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102015ecc65d695-22151d3b-752b-4c10-a3a3-b8ef52491664-000000%40eu-west-1.amazonses.com
In-Reply-To: <CAFZEwPOjK25m84BgTF7AL72DL_K1dHf7OrYoX=_vky9r3GayNw@mail.gmail.com>

```
`--next-all` is meant to be used as a subcommand to support multiple
"operation mode" though the current implementation does not contain any
other subcommand along side with `--next-all` but further commits will
include some more subcommands.

Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>

---
Hey,

It has been a long time since this series appeared on the mailing list.
The previous version v15[1] is now split into many parts and I am
sending the first part right now, will focus on getting this merged and
then send out the next part.

The changes in this part:
 * Stephan pointed out that "terms" was missing in patch 2 ie.
   "bisect--helper: rewrite `check_term_format` shell function in C"

[1]:
https://public-inbox.org/git/CAFZEwPOjK25m84BgTF7AL72DL_K1dHf7OrYoX=_vky9r3GayNw@mail.gmail.com/
---
 builtin/bisect--helper.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 3324229025300..b6ee0acb82765 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -10,11 +10,11 @@ static const char * const git_bisect_helper_usage[] = {
 
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
-	int next_all = 0;
+	enum { NEXT_ALL = 1 } cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
-		OPT_BOOL(0, "next-all", &next_all,
-			 N_("perform 'git bisect next'")),
+		OPT_CMDMODE(0, "next-all", &cmdmode,
+			 N_("perform 'git bisect next'"), NEXT_ALL),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -23,9 +23,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, options,
 			     git_bisect_helper_usage, 0);
 
-	if (!next_all)
+	if (!cmdmode)
 		usage_with_options(git_bisect_helper_usage, options);
 
-	/* next-all */
-	return bisect_next_all(prefix, no_checkout);
+	switch (cmdmode) {
+	case NEXT_ALL:
+		return bisect_next_all(prefix, no_checkout);
+	default:
+		return error("BUG: unknown subcommand '%d'", cmdmode);
+	}
+	return 0;
 }

--
https://github.com/git/git/pull/410

```

## Pranit Bauva, 2017-09-29 06:49

Subject: [PATCH v16 4/6] bisect--helper: `bisect_clean_state` shell function in C
Message-ID: <0102015ecc65d750-d709dbe2-d386-406a-8dc3-326920c92cb1-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102015ecc65d750-d709dbe2-d386-406a-8dc3-326920c92cb1-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102015ecc65d695-22151d3b-752b-4c10-a3a3-b8ef52491664-000000@eu-west-1.amazonses.com>

```
Reimplement `bisect_clean_state` shell function in C and add a
`bisect-clean-state` subcommand to `git bisect--helper` to call it from
git-bisect.sh .

Using `--bisect-clean-state` subcommand is a measure to port shell
function to C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired but its implementation  will
be called by bisect_reset() and bisect_start().

Also introduce a function `mark_for_removal` to store the refs which
need to be removed while iterating through the refs.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 bisect.c                 | 42 ++++++++++++++++++++++++++++++++++++++++++
 bisect.h                 |  2 ++
 builtin/bisect--helper.c | 10 +++++++++-
 git-bisect.sh            | 26 +++-----------------------
 4 files changed, 56 insertions(+), 24 deletions(-)

diff --git a/bisect.c b/bisect.c
index 2549eaf7b1515..d8c4425812eb0 100644
--- a/bisect.c
+++ b/bisect.c
@@ -433,7 +433,12 @@ static int read_bisect_refs(void)
 
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
+static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
+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_head_name, "head-name")
 
 static void read_bisect_paths(struct argv_array *array)
 {
@@ -1043,3 +1048,40 @@ int estimate_bisect_steps(int all)
 
 	return (e < 3 * x) ? n : n - 1;
 }
+
+static int mark_for_removal(const char *refname, const struct object_id *oid,
+			    int flag, void *cb_data)
+{
+	struct string_list *refs = cb_data;
+	char *ref = xstrfmt("refs/bisect%s", refname);
+	string_list_append(refs, ref);
+	return 0;
+}
+
+int bisect_clean_state(void)
+{
+	int result = 0;
+
+	/* There may be some refs packed during bisection */
+	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
+	for_each_ref_in("refs/bisect", mark_for_removal, (void *) &refs_for_removal);
+	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
+	result = delete_refs("bisect: remove", &refs_for_removal, REF_NODEREF);
+	refs_for_removal.strdup_strings = 1;
+	string_list_clear(&refs_for_removal, 0);
+	unlink_or_warn(git_path_bisect_expected_rev());
+	unlink_or_warn(git_path_bisect_ancestors_ok());
+	unlink_or_warn(git_path_bisect_log());
+	unlink_or_warn(git_path_bisect_names());
+	unlink_or_warn(git_path_bisect_run());
+	unlink_or_warn(git_path_bisect_terms());
+	/* Cleanup head-name if it got left by an old version of git-bisect */
+	unlink_or_warn(git_path_head_name());
+	/*
+	 * Cleanup BISECT_START last to support the --no-checkout option
+	 * introduced in the commit 4796e823a.
+	 */
+	unlink_or_warn(git_path_bisect_start());
+
+	return result;
+}
diff --git a/bisect.h b/bisect.h
index acd12ef802c61..0ae63d4616dc6 100644
--- a/bisect.h
+++ b/bisect.h
@@ -28,4 +28,6 @@ extern int estimate_bisect_steps(int all);
 
 extern void read_bisect_terms(const char **bad, const char **good);
 
+extern int bisect_clean_state(void);
+
 #endif
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 2bd2d396e335d..0f4d4e41cf233 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -9,6 +9,7 @@ static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
 	N_("git bisect--helper --write-terms <bad_term> <good_term>"),
+	N_("git bisect--helper --bisect-clean-state"),
 	NULL
 };
 
@@ -83,7 +84,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		NEXT_ALL = 1,
-		WRITE_TERMS
+		WRITE_TERMS,
+		BISECT_CLEAN_STATE
 	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
@@ -91,6 +93,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("perform 'git bisect next'"), NEXT_ALL),
 		OPT_CMDMODE(0, "write-terms", &cmdmode,
 			 N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
+		OPT_CMDMODE(0, "bisect-clean-state", &cmdmode,
+			 N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -109,6 +113,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		if (argc != 2)
 			return error(_("--write-terms requires two arguments"));
 		return write_terms(argv[0], argv[1]);
+	case BISECT_CLEAN_STATE:
+		if (argc != 0)
+			return error(_("--bisect-clean-state requires no arguments"));
+		return bisect_clean_state();
 	default:
 		return error("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index a80fc44e524f2..045830c399873 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -186,7 +186,7 @@ bisect_start() {
 	#
 	# Get rid of any old bisect state.
 	#
-	bisect_clean_state || exit
+	git bisect--helper --bisect-clean-state || exit
 
 	#
 	# Change state.
@@ -195,7 +195,7 @@ bisect_start() {
 	# We have to trap this to be able to clean up using
 	# "bisect_clean_state".
 	#
-	trap 'bisect_clean_state' 0
+	trap 'git bisect--helper --bisect-clean-state' 0
 	trap 'exit 255' 1 2 3 15
 
 	#
@@ -430,27 +430,7 @@ bisect_reset() {
 		die "$(eval_gettext "Could not check out original HEAD '\$branch'.
 Try 'git bisect reset <commit>'.")"
 	fi
-	bisect_clean_state
-}
-
-bisect_clean_state() {
-	# There may be some refs packed during bisection.
-	git for-each-ref --format='%(refname) %(objectname)' refs/bisect/\* |
-	while read ref hash
-	do
-		git update-ref -d $ref $hash || exit
-	done
-	rm -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
-	rm -f "$GIT_DIR/BISECT_ANCESTORS_OK" &&
-	rm -f "$GIT_DIR/BISECT_LOG" &&
-	rm -f "$GIT_DIR/BISECT_NAMES" &&
-	rm -f "$GIT_DIR/BISECT_RUN" &&
-	rm -f "$GIT_DIR/BISECT_TERMS" &&
-	# Cleanup head-name if it got left by an old version of git-bisect
-	rm -f "$GIT_DIR/head-name" &&
-	git update-ref -d --no-deref BISECT_HEAD &&
-	# clean up BISECT_START last
-	rm -f "$GIT_DIR/BISECT_START"
+	git bisect--helper --bisect-clean-state || exit
 }
 
 bisect_replay () {

--
https://github.com/git/git/pull/410

```

## Pranit Bauva, 2017-09-29 06:49

Subject: [PATCH v16 6/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Message-ID: <0102015ecc65d76b-124658b5-5b79-4321-a6cc-01be807c62aa-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102015ecc65d76b-124658b5-5b79-4321-a6cc-01be807c62aa-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102015ecc65d695-22151d3b-752b-4c10-a3a3-b8ef52491664-000000@eu-west-1.amazonses.com>

```
Reimplement `is_expected_rev` & `check_expected_revs` shell function in
C and add a `--check-expected-revs` subcommand to `git bisect--helper` to
call it from git-bisect.sh .

Using `--check-expected-revs` subcommand is a temporary measure to port
shell functions to C so as to use the existing test suite. As more
functions are ported, this subcommand would be retired but its
implementation will be called by some other method.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 34 +++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 20 ++------------------
 2 files changed, 35 insertions(+), 19 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 0f4d4e41cf233..35d2105f941c6 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -5,6 +5,8 @@
 #include "refs.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
+static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
+static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
@@ -80,12 +82,37 @@ static int write_terms(const char *bad, const char *good)
 	return (res < 0) ? -1 : 0;
 }
 
+static int is_expected_rev(const char *expected_hex)
+{
+	struct strbuf actual_hex = STRBUF_INIT;
+	int res = 0;
+	if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 40) {
+		strbuf_trim(&actual_hex);
+		res = !strcmp(actual_hex.buf, expected_hex);
+	}
+	strbuf_release(&actual_hex);
+	return res;
+}
+
+static void check_expected_revs(const char **revs, int rev_nr)
+{
+	int i;
+
+	for (i = 0; i < rev_nr; i++) {
+		if (!is_expected_rev(revs[i])) {
+			unlink_or_warn(git_path_bisect_ancestors_ok());
+			unlink_or_warn(git_path_bisect_expected_rev());
+		}
+	}
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		NEXT_ALL = 1,
 		WRITE_TERMS,
-		BISECT_CLEAN_STATE
+		BISECT_CLEAN_STATE,
+		CHECK_EXPECTED_REVS
 	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
@@ -95,6 +122,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
 		OPT_CMDMODE(0, "bisect-clean-state", &cmdmode,
 			 N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
+		OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
+			 N_("check for expected revs"), CHECK_EXPECTED_REVS),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -117,6 +146,9 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		if (argc != 0)
 			return error(_("--bisect-clean-state requires no arguments"));
 		return bisect_clean_state();
+	case CHECK_EXPECTED_REVS:
+		check_expected_revs(argv, argc);
+		return 0;
 	default:
 		return error("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 045830c399873..0138a8860e377 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -237,22 +237,6 @@ bisect_write() {
 	test -n "$nolog" || echo "git bisect $state $rev" >>"$GIT_DIR/BISECT_LOG"
 }
 
-is_expected_rev() {
-	test -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
-	test "$1" = $(cat "$GIT_DIR/BISECT_EXPECTED_REV")
-}
-
-check_expected_revs() {
-	for _rev in "$@"; do
-		if ! is_expected_rev "$_rev"
-		then
-			rm -f "$GIT_DIR/BISECT_ANCESTORS_OK"
-			rm -f "$GIT_DIR/BISECT_EXPECTED_REV"
-			return
-		fi
-	done
-}
-
 bisect_skip() {
 	all=''
 	for arg in "$@"
@@ -280,7 +264,7 @@ bisect_state() {
 		rev=$(git rev-parse --verify "$bisected_head") ||
 			die "$(eval_gettext "Bad rev input: \$bisected_head")"
 		bisect_write "$state" "$rev"
-		check_expected_revs "$rev" ;;
+		git bisect--helper --check-expected-revs "$rev" ;;
 	2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
 		shift
 		hash_list=''
@@ -294,7 +278,7 @@ bisect_state() {
 		do
 			bisect_write "$state" "$rev"
 		done
-		check_expected_revs $hash_list ;;
+		git bisect--helper --check-expected-revs $hash_list ;;
 	*,"$TERM_BAD")
 		die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;;
 	*)

--
https://github.com/git/git/pull/410

```

## Pranit Bauva, 2017-09-29 06:49

Subject: [PATCH v16 3/6] bisect--helper: `write_terms` shell function in C
Message-ID: <0102015ecc65d74b-cca6093c-3720-46a6-ac99-6e717db2bf8e-000000@eu-west-1.amazonses.com>
URL: https://gitlist.dev/e/0102015ecc65d74b-cca6093c-3720-46a6-ac99-6e717db2bf8e-000000%40eu-west-1.amazonses.com
In-Reply-To: <0102015ecc65d695-22151d3b-752b-4c10-a3a3-b8ef52491664-000000@eu-west-1.amazonses.com>

```
Reimplement the `write_terms` shell function in C and add a `write-terms`
subcommand to `git bisect--helper` to call it from git-bisect.sh . Also
remove the subcommand `--check-term-format` as it can now be called from
inside the function write_terms() C implementation.

Also `|| exit` is added when calling write-terms subcommand from
git-bisect.sh so as to exit whenever there is an error.

Using `--write-terms` subcommand is a temporary measure to port shell
function to C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired and its implementation will
be called by some other method.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
---
 builtin/bisect--helper.c | 36 +++++++++++++++++++++++++++++-------
 git-bisect.sh            | 22 +++++++---------------
 2 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index db9091e249454..2bd2d396e335d 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -4,9 +4,11 @@
 #include "bisect.h"
 #include "refs.h"
 
+static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
+
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
-	N_("git bisect--helper --check-term-format <term> <orig_term>"),
+	N_("git bisect--helper --write-terms <bad_term> <good_term>"),
 	NULL
 };
 
@@ -57,18 +59,38 @@ static int check_term_format(const char *term, const char *orig_term)
 	return 0;
 }
 
+static int write_terms(const char *bad, const char *good)
+{
+	FILE *fp = NULL;
+	int res;
+
+	if (!strcmp(bad, good))
+		return error(_("please use two different terms"));
+
+	if (check_term_format(bad, "bad") || check_term_format(good, "good"))
+		return -1;
+
+	fp = fopen(git_path_bisect_terms(), "w");
+	if (!fp)
+		return error_errno(_("could not open the file BISECT_TERMS"));
+
+	res = fprintf(fp, "%s\n%s\n", bad, good);
+	res |= fclose(fp);
+	return (res < 0) ? -1 : 0;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		NEXT_ALL = 1,
-		CHECK_TERM_FMT
+		WRITE_TERMS
 	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
 		OPT_CMDMODE(0, "next-all", &cmdmode,
 			 N_("perform 'git bisect next'"), NEXT_ALL),
-		OPT_CMDMODE(0, "check-term-format", &cmdmode,
-			 N_("check format of the term"), CHECK_TERM_FMT),
+		OPT_CMDMODE(0, "write-terms", &cmdmode,
+			 N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -83,10 +105,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	switch (cmdmode) {
 	case NEXT_ALL:
 		return bisect_next_all(prefix, no_checkout);
-	case CHECK_TERM_FMT:
+	case WRITE_TERMS:
 		if (argc != 2)
-			return error(_("--check-term-format requires two arguments"));
-		return check_term_format(argv[0], argv[1]);
+			return error(_("--write-terms requires two arguments"));
+		return write_terms(argv[0], argv[1]);
 	default:
 		return error("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index a727c59250f13..a80fc44e524f2 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -209,7 +209,7 @@ bisect_start() {
 	eval "$eval true" &&
 	if test $must_write_terms -eq 1
 	then
-		write_terms "$TERM_BAD" "$TERM_GOOD"
+		git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || exit
 	fi &&
 	echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
 	#
@@ -557,18 +557,6 @@ get_terms () {
 	fi
 }
 
-write_terms () {
-	TERM_BAD=$1
-	TERM_GOOD=$2
-	if test "$TERM_BAD" = "$TERM_GOOD"
-	then
-		die "$(gettext "please use two different terms")"
-	fi
-	git bisect--helper --check-term-format "$TERM_BAD" bad || exit
-	git bisect--helper --check-term-format "$TERM_GOOD" good || exit
-	printf '%s\n%s\n' "$TERM_BAD" "$TERM_GOOD" >"$GIT_DIR/BISECT_TERMS"
-}
-
 check_and_set_terms () {
 	cmd="$1"
 	case "$cmd" in
@@ -582,13 +570,17 @@ check_and_set_terms () {
 		bad|good)
 			if ! test -s "$GIT_DIR/BISECT_TERMS"
 			then
-				write_terms bad good
+				TERM_BAD=bad
+				TERM_GOOD=good
+				git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || exit
 			fi
 			;;
 		new|old)
 			if ! test -s "$GIT_DIR/BISECT_TERMS"
 			then
-				write_terms new old
+				TERM_BAD=new
+				TERM_GOOD=old
+				git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || exit
 			fi
 			;;
 		esac ;;

--
https://github.com/git/git/pull/410

```

## Stephan Beyer, 2017-09-29 18:54

Subject: Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Message-ID: <1ffcb7a7-6ffe-c98e-66a1-c3d392f76077@gmx.net>
URL: https://gitlist.dev/e/1ffcb7a7-6ffe-c98e-66a1-c3d392f76077%40gmx.net
In-Reply-To: <0102015ecc65d695-22151d3b-752b-4c10-a3a3-b8ef52491664-000000@eu-west-1.amazonses.com>

```
Hi Pranit,

On 09/29/2017 08:49 AM, Pranit Bauva wrote:
> It has been a long time since this series appeared on the mailing list.
> The previous version v15[1] is now split into many parts and I am
> sending the first part right now, will focus on getting this merged and
> then send out the next part.

That's a good idea!

I just reviewed your series by assuming I did the v15 review well (it
took quite some time at least)... so I just diff'ed the v15 and the v16
patches. I am totally fine with it!

Thanks,
Stephan

```

## Pranit Bauva, 2017-09-29 21:10

Subject: Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Message-ID: <CAFZEwPPikNjVDmif0izfwYUqywDO5N2RLf0UF7pZ6qSQyT3q6w@mail.gmail.com>
URL: https://gitlist.dev/e/CAFZEwPPikNjVDmif0izfwYUqywDO5N2RLf0UF7pZ6qSQyT3q6w%40mail.gmail.com
In-Reply-To: <1ffcb7a7-6ffe-c98e-66a1-c3d392f76077@gmx.net>

```
Hey Stephan,

On Sat, Sep 30, 2017 at 12:24 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
>
> Hi Pranit,
>
> On 09/29/2017 08:49 AM, Pranit Bauva wrote:
> > It has been a long time since this series appeared on the mailing list.
> > The previous version v15[1] is now split into many parts and I am
> > sending the first part right now, will focus on getting this merged and
> > then send out the next part.
>
> That's a good idea!
>
> I just reviewed your series by assuming I did the v15 review well (it
> took quite some time at least)... so I just diff'ed the v15 and the v16
> patches. I am totally fine with it!

Thanks for having the patience of waiting for it and reviewing it again.

Regards,
Pranit Bauva
www.bauva.com

```

## Ramsay Jones, 2017-09-30 12:59

Subject: Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Message-ID: <5ca7a863-5b85-23b1-4432-cf74dcc7c4e0@ramsayjones.plus.com>
URL: https://gitlist.dev/e/5ca7a863-5b85-23b1-4432-cf74dcc7c4e0%40ramsayjones.plus.com
In-Reply-To: <0102015ecc65d695-22151d3b-752b-4c10-a3a3-b8ef52491664-000000@eu-west-1.amazonses.com>

```


On 29/09/17 07:49, Pranit Bauva wrote:
> `--next-all` is meant to be used as a subcommand to support multiple
> "operation mode" though the current implementation does not contain any
> other subcommand along side with `--next-all` but further commits will
> include some more subcommands.
> 
> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> 
> ---
> Hey,
> 
> It has been a long time since this series appeared on the mailing list.
> The previous version v15[1] is now split into many parts and I am
> sending the first part right now, will focus on getting this merged and
> then send out the next part.
> 
> The changes in this part:
>  * Stephan pointed out that "terms" was missing in patch 2 ie.
>    "bisect--helper: rewrite `check_term_format` shell function in C"
> 
> [1]:
> https://public-inbox.org/git/CAFZEwPOjK25m84BgTF7AL72DL_K1dHf7OrYoX=_vky9r3GayNw@mail.gmail.com/

Hi Pranit,

Just before Junio dropped your 'pb/bisect' branch from his
repository (and What's cooking), I fetched it locally with
the intention of finishing it off. (It would have been silly
to waste all your good work).

Although I have rebased your branch a few times, and added
a few commits while 'reading' the code, I haven't actually
added much to your branch (only 12 commits and I had meant
to squash some of those together)!

However, there were some bug fixes in there, so you may want
to take a look at:

    git://repo.or.cz/git/raj.git branch 'bisect'

[the 'pb-bisect' branch was the original branch from Junio,
including the 'SQUASH' commit that I squashed!]

These patches seem to relate to patches 1-5 & 8 of the original
series. The diff between these patches and the first 6 patches
of my bisect branch is given below. Note that most of the diff
seems to be caused by swapping patch #6 for #8, but not all of
the hunks are caused by this.

Note that I moved some code between patches (e.g. some of the
GIT_PATH_FUNC()s moved out of patch #4, because they were not
used in that patch. Ah, is that why you moved patch #8 up?).
[Also I added the 'bisect clean' message to delet_refs() to
patch #4 as well.]

Look for []-ed comments in the commit messages for a note of
the changes I made to your original patches, in patches #2,
#4, #7-9, #11-12 and #14.

The commits I added, which are just WIP, are as follows:

  $ git log --oneline bisect~12..bisect
  7d7117040 (raj/bisect, bisect) bisect--helper: convert to struct object_id
  188ea5855 bisect--helper: add the get_bad_commit() function
  b75f46fb4 bisect--helper: add a log_commit() helper function
  4afc34403 bisect--helper: reduce the scope of a variable
  62495f6ae bisect--helper: remove useless sub-expression in condition
  964f4e2b0 bisect--helper: set correct term from --term-new= option
  62efc099f bisect--helper: remove redundant assignment to has_double_dash
  d35950b92 bisect--helper: remove redundant goto's
  b33f313ac bisect--helper: remove space just before \n in string
  3eb407156 bisect--helper: remove some unnecessary braces
  c2b89c9b8 bisect--helper: add some vertical whitespace
  8c883701c bisect--helper: fix up some coding style issues
  $ 

Again IIRC, there are a couple of bug fixes in these commits ...

I have to go now, so I will leave it with you. ;-)

Hope that helps.

ATB,
Ramsay Jones


-- >8 --
diff --git a/bisect.c b/bisect.c
index 2838d672d..b19311ca7 100644
--- a/bisect.c
+++ b/bisect.c
@@ -1066,7 +1066,7 @@ int bisect_clean_state(void)
 	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
 	for_each_ref_in("refs/bisect", mark_for_removal, (void *) &refs_for_removal);
 	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
-	result = delete_refs("bisect: remove", &refs_for_removal, REF_NODEREF);
+	result = delete_refs("bisect: clean", &refs_for_removal, REF_NODEREF);
 	refs_for_removal.strdup_strings = 1;
 	string_list_clear(&refs_for_removal, 0);
 	unlink_or_warn(git_path_bisect_expected_rev());
diff --git a/builtin/am.c b/builtin/am.c
index c973bd96d..aa66f9915 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -32,22 +32,6 @@
 #include "apply.h"
 #include "string-list.h"
 
-/**
- * Returns 1 if the file is empty or does not exist, 0 otherwise.
- */
-static int is_empty_file(const char *filename)
-{
-	struct stat st;
-
-	if (stat(filename, &st) < 0) {
-		if (errno == ENOENT)
-			return 1;
-		die_errno(_("could not stat %s"), filename);
-	}
-
-	return !st.st_size;
-}
-
 /**
  * Returns the length of the first line of msg.
  */
@@ -1300,7 +1284,7 @@ static int parse_mail(struct am_state *state, const char *mail)
 		goto finish;
 	}
 
-	if (is_empty_file(am_path(state, "patch"))) {
+	if (is_empty_or_missing_file(am_path(state, "patch"))) {
 		printf_ln(_("Patch is empty."));
 		die_user_resolve(state);
 	}
@@ -1883,7 +1867,7 @@ static void am_run(struct am_state *state, int resume)
 		resume = 0;
 	}
 
-	if (!is_empty_file(am_path(state, "rewritten"))) {
+	if (!is_empty_or_missing_file(am_path(state, "rewritten"))) {
 		assert(state->rebasing);
 		copy_notes_for_rebase(state);
 		run_post_rewrite_hook(state);
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 35d2105f9..2af024f60 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -5,8 +5,6 @@
 #include "refs.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
-static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
-static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
@@ -45,8 +43,8 @@ static int check_term_format(const char *term, const char *orig_term)
 	if (res)
 		return error(_("'%s' is not a valid term"), term);
 
-	if (one_of(term, "help", "start", "skip", "next", "reset",
-			"visualize", "replay", "log", "run", "terms", NULL))
+	if (one_of(term, "help", "start", "terms", "skip", "next", "reset",
+			"visualize", "replay", "log", "run", NULL))
 		return error(_("can't use the builtin command '%s' as a term"), term);
 
 	/*
@@ -82,37 +80,12 @@ static int write_terms(const char *bad, const char *good)
 	return (res < 0) ? -1 : 0;
 }
 
-static int is_expected_rev(const char *expected_hex)
-{
-	struct strbuf actual_hex = STRBUF_INIT;
-	int res = 0;
-	if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 40) {
-		strbuf_trim(&actual_hex);
-		res = !strcmp(actual_hex.buf, expected_hex);
-	}
-	strbuf_release(&actual_hex);
-	return res;
-}
-
-static void check_expected_revs(const char **revs, int rev_nr)
-{
-	int i;
-
-	for (i = 0; i < rev_nr; i++) {
-		if (!is_expected_rev(revs[i])) {
-			unlink_or_warn(git_path_bisect_ancestors_ok());
-			unlink_or_warn(git_path_bisect_expected_rev());
-		}
-	}
-}
-
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		NEXT_ALL = 1,
 		WRITE_TERMS,
-		BISECT_CLEAN_STATE,
-		CHECK_EXPECTED_REVS
+		BISECT_CLEAN_STATE
 	} cmdmode = 0;
 	int no_checkout = 0;
 	struct option options[] = {
@@ -122,8 +95,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
 		OPT_CMDMODE(0, "bisect-clean-state", &cmdmode,
 			 N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
-		OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
-			 N_("check for expected revs"), CHECK_EXPECTED_REVS),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_END()
@@ -140,17 +111,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		return bisect_next_all(prefix, no_checkout);
 	case WRITE_TERMS:
 		if (argc != 2)
-			return error(_("--write-terms requires two arguments"));
+			die(_("--write-terms requires two arguments"));
 		return write_terms(argv[0], argv[1]);
 	case BISECT_CLEAN_STATE:
 		if (argc != 0)
-			return error(_("--bisect-clean-state requires no arguments"));
+			die(_("--bisect-clean-state requires no arguments"));
 		return bisect_clean_state();
-	case CHECK_EXPECTED_REVS:
-		check_expected_revs(argv, argc);
-		return 0;
 	default:
-		return error("BUG: unknown subcommand '%d'", cmdmode);
+		die("BUG: unknown subcommand '%d'", cmdmode);
 	}
 	return 0;
 }
diff --git a/cache.h b/cache.h
index 71fe09264..7a1901893 100644
--- a/cache.h
+++ b/cache.h
@@ -2029,4 +2029,7 @@ void sleep_millisec(int millisec);
  */
 void safe_create_dir(const char *dir, int share);
 
+/* Return 1 if the file is empty or does not exists, 0 otherwise. */
+extern int is_empty_or_missing_file(const char *filename);
+
 #endif /* CACHE_H */
diff --git a/git-bisect.sh b/git-bisect.sh
index 0138a8860..f1202dfb4 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -209,7 +209,7 @@ bisect_start() {
 	eval "$eval true" &&
 	if test $must_write_terms -eq 1
 	then
-		git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || exit
+		git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD"
 	fi &&
 	echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
 	#
@@ -237,6 +237,22 @@ bisect_write() {
 	test -n "$nolog" || echo "git bisect $state $rev" >>"$GIT_DIR/BISECT_LOG"
 }
 
+is_expected_rev() {
+	test -f "$GIT_DIR/BISECT_EXPECTED_REV" &&
+	test "$1" = $(cat "$GIT_DIR/BISECT_EXPECTED_REV")
+}
+
+check_expected_revs() {
+	for _rev in "$@"; do
+		if ! is_expected_rev "$_rev"
+		then
+			rm -f "$GIT_DIR/BISECT_ANCESTORS_OK"
+			rm -f "$GIT_DIR/BISECT_EXPECTED_REV"
+			return
+		fi
+	done
+}
+
 bisect_skip() {
 	all=''
 	for arg in "$@"
@@ -264,7 +280,7 @@ bisect_state() {
 		rev=$(git rev-parse --verify "$bisected_head") ||
 			die "$(eval_gettext "Bad rev input: \$bisected_head")"
 		bisect_write "$state" "$rev"
-		git bisect--helper --check-expected-revs "$rev" ;;
+		check_expected_revs "$rev" ;;
 	2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
 		shift
 		hash_list=''
@@ -278,7 +294,7 @@ bisect_state() {
 		do
 			bisect_write "$state" "$rev"
 		done
-		git bisect--helper --check-expected-revs $hash_list ;;
+		check_expected_revs $hash_list ;;
 	*,"$TERM_BAD")
 		die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;;
 	*)
diff --git a/wrapper.c b/wrapper.c
index 36630e5d1..975ca2f1b 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -690,3 +690,16 @@ int xgethostname(char *buf, size_t len)
 		buf[len - 1] = 0;
 	return ret;
 }
+
+int is_empty_or_missing_file(const char *filename)
+{
+	struct stat st;
+
+	if (stat(filename, &st) < 0) {
+		if (errno == ENOENT)
+			return 1;
+		die_errno(_("could not stat %s"), filename);
+	}
+
+	return !st.st_size;
+}

```

## Pranit Bauva, 2017-10-02 13:44

Subject: Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Message-ID: <CAFZEwPN2BeogeVqXp7xh+GPUF6SDJa5JwQ3knZzOD_H6zcEwvQ@mail.gmail.com>
URL: https://gitlist.dev/e/CAFZEwPN2BeogeVqXp7xh%2BGPUF6SDJa5JwQ3knZzOD_H6zcEwvQ%40mail.gmail.com
In-Reply-To: <5ca7a863-5b85-23b1-4432-cf74dcc7c4e0@ramsayjones.plus.com>

```
Hey Ramsay,

On Sat, Sep 30, 2017 at 6:29 PM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:

> Hi Pranit,
>
> Just before Junio dropped your 'pb/bisect' branch from his
> repository (and What's cooking), I fetched it locally with
> the intention of finishing it off. (It would have been silly
> to waste all your good work).

Thanks!

> Although I have rebased your branch a few times, and added
> a few commits while 'reading' the code, I haven't actually
> added much to your branch (only 12 commits and I had meant
> to squash some of those together)!
>
> However, there were some bug fixes in there, so you may want
> to take a look at:
>
>     git://repo.or.cz/git/raj.git branch 'bisect'
>
> [the 'pb-bisect' branch was the original branch from Junio,
> including the 'SQUASH' commit that I squashed!]

Yes, I have checked it out. I had worked on Stephan's review and
updated a few parts. I think you have included that as well as some of
your modifications. I will squash it in together and send the series
out in parts.

> These patches seem to relate to patches 1-5 & 8 of the original
> series. The diff between these patches and the first 6 patches
> of my bisect branch is given below. Note that most of the diff
> seems to be caused by swapping patch #6 for #8, but not all of
> the hunks are caused by this.
>
> Note that I moved some code between patches (e.g. some of the
> GIT_PATH_FUNC()s moved out of patch #4, because they were not
> used in that patch. Ah, is that why you moved patch #8 up?).
> [Also I added the 'bisect clean' message to delet_refs() to
> patch #4 as well.]

Even I thought keeping GIT_PATH_FUNC()s should be declared whenever
required. Did that change in this patch series.

> Look for []-ed comments in the commit messages for a note of
> the changes I made to your original patches, in patches #2,
> #4, #7-9, #11-12 and #14.
>
> The commits I added, which are just WIP, are as follows:
>
>   $ git log --oneline bisect~12..bisect
>   7d7117040 (raj/bisect, bisect) bisect--helper: convert to struct object_id
>   188ea5855 bisect--helper: add the get_bad_commit() function
>   b75f46fb4 bisect--helper: add a log_commit() helper function
>   4afc34403 bisect--helper: reduce the scope of a variable
>   62495f6ae bisect--helper: remove useless sub-expression in condition
>   964f4e2b0 bisect--helper: set correct term from --term-new= option
>   62efc099f bisect--helper: remove redundant assignment to has_double_dash
>   d35950b92 bisect--helper: remove redundant goto's
>   b33f313ac bisect--helper: remove space just before \n in string
>   3eb407156 bisect--helper: remove some unnecessary braces
>   c2b89c9b8 bisect--helper: add some vertical whitespace
>   8c883701c bisect--helper: fix up some coding style issues
>   $
>
> Again IIRC, there are a couple of bug fixes in these commits ...

There is actually a major bug in the later part of previous series
mostly in the bisect-next which actually caused delays. I think you
have fixed it in your commit 682d0bff0. Although I would need to have
a closer look at it. In original series, I did get a sigserv, and as
you mention it in the commit that you have fixed it.

> I have to go now, so I will leave it with you. ;-)
>
> Hope that helps.
>
> ATB,
> Ramsay Jones

Again Thanks!

>
> -- >8 --
> diff --git a/bisect.c b/bisect.c
> index 2838d672d..b19311ca7 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -1066,7 +1066,7 @@ int bisect_clean_state(void)
>         struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
>         for_each_ref_in("refs/bisect", mark_for_removal, (void *) &refs_for_removal);
>         string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
> -       result = delete_refs("bisect: remove", &refs_for_removal, REF_NODEREF);
> +       result = delete_refs("bisect: clean", &refs_for_removal, REF_NODEREF);
>         refs_for_removal.strdup_strings = 1;
>         string_list_clear(&refs_for_removal, 0);
>         unlink_or_warn(git_path_bisect_expected_rev());
> diff --git a/builtin/am.c b/builtin/am.c
> index c973bd96d..aa66f9915 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -32,22 +32,6 @@
>  #include "apply.h"
>  #include "string-list.h"
>
> -/**
> - * Returns 1 if the file is empty or does not exist, 0 otherwise.
> - */
> -static int is_empty_file(const char *filename)
> -{
> -       struct stat st;
> -
> -       if (stat(filename, &st) < 0) {
> -               if (errno == ENOENT)
> -                       return 1;
> -               die_errno(_("could not stat %s"), filename);
> -       }
> -
> -       return !st.st_size;
> -}
> -
>  /**
>   * Returns the length of the first line of msg.
>   */
> @@ -1300,7 +1284,7 @@ static int parse_mail(struct am_state *state, const char *mail)
>                 goto finish;
>         }
>
> -       if (is_empty_file(am_path(state, "patch"))) {
> +       if (is_empty_or_missing_file(am_path(state, "patch"))) {
>                 printf_ln(_("Patch is empty."));
>                 die_user_resolve(state);
>         }
> @@ -1883,7 +1867,7 @@ static void am_run(struct am_state *state, int resume)
>                 resume = 0;
>         }
>
> -       if (!is_empty_file(am_path(state, "rewritten"))) {
> +       if (!is_empty_or_missing_file(am_path(state, "rewritten"))) {
>                 assert(state->rebasing);
>                 copy_notes_for_rebase(state);
>                 run_post_rewrite_hook(state);
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 35d2105f9..2af024f60 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -5,8 +5,6 @@
>  #include "refs.h"
>
>  static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
> -static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
> -static GIT_PATH_FUNC(git_path_bisect_ancestors_ok, "BISECT_ANCESTORS_OK")
>
>  static const char * const git_bisect_helper_usage[] = {
>         N_("git bisect--helper --next-all [--no-checkout]"),
> @@ -45,8 +43,8 @@ static int check_term_format(const char *term, const char *orig_term)
>         if (res)
>                 return error(_("'%s' is not a valid term"), term);
>
> -       if (one_of(term, "help", "start", "skip", "next", "reset",
> -                       "visualize", "replay", "log", "run", "terms", NULL))
> +       if (one_of(term, "help", "start", "terms", "skip", "next", "reset",
> +                       "visualize", "replay", "log", "run", NULL))
>                 return error(_("can't use the builtin command '%s' as a term"), term);
>
>         /*
> @@ -82,37 +80,12 @@ static int write_terms(const char *bad, const char *good)
>         return (res < 0) ? -1 : 0;
>  }
>
> -static int is_expected_rev(const char *expected_hex)
> -{
> -       struct strbuf actual_hex = STRBUF_INIT;
> -       int res = 0;
> -       if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 40) {
> -               strbuf_trim(&actual_hex);
> -               res = !strcmp(actual_hex.buf, expected_hex);
> -       }
> -       strbuf_release(&actual_hex);
> -       return res;
> -}
> -
> -static void check_expected_revs(const char **revs, int rev_nr)
> -{
> -       int i;
> -
> -       for (i = 0; i < rev_nr; i++) {
> -               if (!is_expected_rev(revs[i])) {
> -                       unlink_or_warn(git_path_bisect_ancestors_ok());
> -                       unlink_or_warn(git_path_bisect_expected_rev());
> -               }
> -       }
> -}
> -
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>         enum {
>                 NEXT_ALL = 1,
>                 WRITE_TERMS,
> -               BISECT_CLEAN_STATE,
> -               CHECK_EXPECTED_REVS
> +               BISECT_CLEAN_STATE
>         } cmdmode = 0;
>         int no_checkout = 0;
>         struct option options[] = {
> @@ -122,8 +95,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>                          N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
>                 OPT_CMDMODE(0, "bisect-clean-state", &cmdmode,
>                          N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
> -               OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
> -                        N_("check for expected revs"), CHECK_EXPECTED_REVS),
>                 OPT_BOOL(0, "no-checkout", &no_checkout,
>                          N_("update BISECT_HEAD instead of checking out the current commit")),
>                 OPT_END()
> @@ -140,17 +111,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>                 return bisect_next_all(prefix, no_checkout);
>         case WRITE_TERMS:
>                 if (argc != 2)
> -                       return error(_("--write-terms requires two arguments"));
> +                       die(_("--write-terms requires two arguments"));
>                 return write_terms(argv[0], argv[1]);
>         case BISECT_CLEAN_STATE:
>                 if (argc != 0)
> -                       return error(_("--bisect-clean-state requires no arguments"));
> +                       die(_("--bisect-clean-state requires no arguments"));
>                 return bisect_clean_state();
> -       case CHECK_EXPECTED_REVS:
> -               check_expected_revs(argv, argc);
> -               return 0;
>         default:
> -               return error("BUG: unknown subcommand '%d'", cmdmode);
> +               die("BUG: unknown subcommand '%d'", cmdmode);

I will keep the return rather than die since Christian and I had a few
conversations long back.

Regards,
Pranit Bauva
www.bauva.com

```

## Ramsay Jones, 2017-10-03 00:48

Subject: Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Message-ID: <72995d24-d337-39c0-0abd-64bce1c22028@ramsayjones.plus.com>
URL: https://gitlist.dev/e/72995d24-d337-39c0-0abd-64bce1c22028%40ramsayjones.plus.com
In-Reply-To: <CAFZEwPN2BeogeVqXp7xh+GPUF6SDJa5JwQ3knZzOD_H6zcEwvQ@mail.gmail.com>

```


On 02/10/17 14:44, Pranit Bauva wrote:
[snip]
>> Look for []-ed comments in the commit messages for a note of
>> the changes I made to your original patches, in patches #2,
>> #4, #7-9, #11-12 and #14.
>>
>> The commits I added, which are just WIP, are as follows:
>>
>>   $ git log --oneline bisect~12..bisect
>>   7d7117040 (raj/bisect, bisect) bisect--helper: convert to struct object_id
>>   188ea5855 bisect--helper: add the get_bad_commit() function
>>   b75f46fb4 bisect--helper: add a log_commit() helper function
>>   4afc34403 bisect--helper: reduce the scope of a variable
>>   62495f6ae bisect--helper: remove useless sub-expression in condition
>>   964f4e2b0 bisect--helper: set correct term from --term-new= option
>>   62efc099f bisect--helper: remove redundant assignment to has_double_dash
>>   d35950b92 bisect--helper: remove redundant goto's
>>   b33f313ac bisect--helper: remove space just before \n in string
>>   3eb407156 bisect--helper: remove some unnecessary braces
>>   c2b89c9b8 bisect--helper: add some vertical whitespace
>>   8c883701c bisect--helper: fix up some coding style issues
>>   $
>>
>> Again IIRC, there are a couple of bug fixes in these commits ...
> 
> There is actually a major bug in the later part of previous series
> mostly in the bisect-next which actually caused delays. I think you
> have fixed it in your commit 682d0bff0. Although I would need to have
> a closer look at it. In original series, I did get a sigserv, and as
> you mention it in the commit that you have fixed it.

Yes, I also meant to tidy that up by removing some, now
redundant, initialisation later in that function.

Note, that wasn't the only bug! (I have probably forgotten
some of them, but look at 964f4e2b0, for example).

ATB,
Ramsay Jones


```

## Junio C Hamano, 2017-10-03 03:51

Subject: Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Message-ID: <xmqqa818lwi7.fsf@gitster.mtv.corp.google.com>
URL: https://gitlist.dev/e/xmqqa818lwi7.fsf%40gitster.mtv.corp.google.com
In-Reply-To: <72995d24-d337-39c0-0abd-64bce1c22028@ramsayjones.plus.com>

```
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> On 02/10/17 14:44, Pranit Bauva wrote:
> [snip]
>>...
> Yes, I also meant to tidy that up by removing some, now
> redundant, initialisation later in that function.
>
> Note, that wasn't the only bug! (I have probably forgotten
> some of them, but look at 964f4e2b0, for example).

It seems that Pranit needs a bit more work to take known fixes from
your efforts and we should wait for the series to be rerolled?

Thanks both for working on this.

```

## Pranit Bauva, 2017-10-03 04:35

Subject: Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Message-ID: <CAFZEwPNW96abYd-xTZdCx+K2jUJCf4SrM-Jr0R=i7fJXL+HzKw@mail.gmail.com>
URL: https://gitlist.dev/e/CAFZEwPNW96abYd-xTZdCx%2BK2jUJCf4SrM-Jr0R%3Di7fJXL%2BHzKw%40mail.gmail.com
In-Reply-To: <xmqqa818lwi7.fsf@gitster.mtv.corp.google.com>

```
Hey Junio,

On Tue, Oct 3, 2017 at 9:21 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
>
>> On 02/10/17 14:44, Pranit Bauva wrote:
>> [snip]
>>>...
>> Yes, I also meant to tidy that up by removing some, now
>> redundant, initialisation later in that function.
>>
>> Note, that wasn't the only bug! (I have probably forgotten
>> some of them, but look at 964f4e2b0, for example).
>
> It seems that Pranit needs a bit more work to take known fixes from
> your efforts and we should wait for the series to be rerolled?

This series is the initial part of the whole series. Things started to
get a little problematic after bisect_start(). The reason I am doing
the series in parts so that I can get the part which is solid to be
merged and then start work on later parts since I don't have a time
constraint now.

Regards,
Pranit Bauva

```

## Ramsay Jones, 2017-10-04 02:22

Subject: Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Message-ID: <b2816cf9-4657-1124-2172-e15b619d6d42@ramsayjones.plus.com>
URL: https://gitlist.dev/e/b2816cf9-4657-1124-2172-e15b619d6d42%40ramsayjones.plus.com
In-Reply-To: <xmqqa818lwi7.fsf@gitster.mtv.corp.google.com>

```


On 03/10/17 04:51, Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>> On 02/10/17 14:44, Pranit Bauva wrote:
>> [snip]
>>> ...
>> Yes, I also meant to tidy that up by removing some, now
>> redundant, initialisation later in that function.
>>
>> Note, that wasn't the only bug! (I have probably forgotten
>> some of them, but look at 964f4e2b0, for example).
> 
> It seems that Pranit needs a bit more work to take known fixes from
> your efforts and we should wait for the series to be rerolled?

This series is just the first few patches from the original 28/29
patch series; in particular, patches 1-5 and 8 of that series.
If I compare just the first 5 patches, then the differences are
small and (maybe) not worth a re-roll. For some reason, I changed
the message passed to the delete_refs() call from "bisect: remove"
to "bisect: clean", we both added "terms" to the 'builtin command'
list but in different positions and some calls to die() have
been replaced with 'return error(...)'.

Viz:

  $ git log --oneline v2.14.0..bisect~34
  d8ce077c8 t6030: explicitly test for bisection cleanup
  1fd8e44a1 bisect--helper: `bisect_clean_state` shell function in C
  5aa9d18eb bisect--helper: `write_terms` shell function in C
  1b94b2ff1 bisect: rewrite `check_term_format` shell function in C
  a97a96ccf bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
  $ 
  $ git log --oneline v2.14.0..pranit~1
  6012123c7 t6030: explicitly test for bisection cleanup
  7c945c391 bisect--helper: `bisect_clean_state` shell function in C
  9240c3962 bisect--helper: `write_terms` shell function in C
  ba496589c bisect--helper: rewrite `check_term_format` shell function in C
  f1563a33f bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
  $ 

Note that the subject line of patch #2 has also been corrected
in this new series (bisect: -> bisect--helper:). I haven't
compared the commit messages.

  $ git diff bisect~34 pranit~1
  diff --git a/bisect.c b/bisect.c
  index b19311ca7..2838d672d 100644
  --- a/bisect.c
  +++ b/bisect.c
  @@ -1066,7 +1066,7 @@ int bisect_clean_state(void)
   	struct string_list refs_for_removal = STRING_LIST_INIT_NODUP;
   	for_each_ref_in("refs/bisect", mark_for_removal, (void *) &refs_for_removal);
   	string_list_append(&refs_for_removal, xstrdup("BISECT_HEAD"));
  -	result = delete_refs("bisect: clean", &refs_for_removal, REF_NODEREF);
  +	result = delete_refs("bisect: remove", &refs_for_removal, REF_NODEREF);
   	refs_for_removal.strdup_strings = 1;
   	string_list_clear(&refs_for_removal, 0);
   	unlink_or_warn(git_path_bisect_expected_rev());
  diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
  index 2af024f60..0f4d4e41c 100644
  --- a/builtin/bisect--helper.c
  +++ b/builtin/bisect--helper.c
  @@ -43,8 +43,8 @@ static int check_term_format(const char *term, const char *orig_term)
   	if (res)
   		return error(_("'%s' is not a valid term"), term);
   
  -	if (one_of(term, "help", "start", "terms", "skip", "next", "reset",
  -			"visualize", "replay", "log", "run", NULL))
  +	if (one_of(term, "help", "start", "skip", "next", "reset",
  +			"visualize", "replay", "log", "run", "terms", NULL))
   		return error(_("can't use the builtin command '%s' as a term"), term);
   
   	/*
  @@ -111,14 +111,14 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
   		return bisect_next_all(prefix, no_checkout);
   	case WRITE_TERMS:
   		if (argc != 2)
  -			die(_("--write-terms requires two arguments"));
  +			return error(_("--write-terms requires two arguments"));
   		return write_terms(argv[0], argv[1]);
   	case BISECT_CLEAN_STATE:
   		if (argc != 0)
  -			die(_("--bisect-clean-state requires no arguments"));
  +			return error(_("--bisect-clean-state requires no arguments"));
   		return bisect_clean_state();
   	default:
  -		die("BUG: unknown subcommand '%d'", cmdmode);
  +		return error("BUG: unknown subcommand '%d'", cmdmode);
   	}
   	return 0;
   }
  diff --git a/git-bisect.sh b/git-bisect.sh
  index f1202dfb4..045830c39 100755
  --- a/git-bisect.sh
  +++ b/git-bisect.sh
  @@ -209,7 +209,7 @@ bisect_start() {
   	eval "$eval true" &&
   	if test $must_write_terms -eq 1
   	then
  -		git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD"
  +		git bisect--helper --write-terms "$TERM_BAD" "$TERM_GOOD" || exit
   	fi &&
   	echo "git bisect start$orig_args" >>"$GIT_DIR/BISECT_LOG" || exit
   	#
  $ 

So, the major differences and bug fixes are in later patches.

ATB,
Ramsay Jones


```

## Junio C Hamano, 2017-10-04 04:07

Subject: Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
Message-ID: <xmqq1smjfted.fsf@gitster.mtv.corp.google.com>
URL: https://gitlist.dev/e/xmqq1smjfted.fsf%40gitster.mtv.corp.google.com
In-Reply-To: <b2816cf9-4657-1124-2172-e15b619d6d42@ramsayjones.plus.com>

```
Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

> On 03/10/17 04:51, Junio C Hamano wrote:
>> 
>> It seems that Pranit needs a bit more work to take known fixes from
>> your efforts and we should wait for the series to be rerolled?
> 
> This series is just the first few patches from the original 28/29
> patch series; in particular, patches 1-5 and 8 of that series.
> ...
> So, the major differences and bug fixes are in later patches.

OK.  So these are good to go separately (as prerequistes for the
follow-on work)--thanks for clarification.

```

## Christian Couder, 2017-11-19 20:34

Subject: Re: [PATCH v16 6/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Message-ID: <CAP8UFD3SYU_+MOT-TGCV4uJ-cxrd9VhgOOprkq-XP0CUhtGTig@mail.gmail.com>
URL: https://gitlist.dev/e/CAP8UFD3SYU_%2BMOT-TGCV4uJ-cxrd9VhgOOprkq-XP0CUhtGTig%40mail.gmail.com
In-Reply-To: <0102015ecc65d76b-124658b5-5b79-4321-a6cc-01be807c62aa-000000@eu-west-1.amazonses.com>

```
On Fri, Sep 29, 2017 at 8:49 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
> Reimplement `is_expected_rev` & `check_expected_revs` shell function in
> C and add a `--check-expected-revs` subcommand to `git bisect--helper` to
> call it from git-bisect.sh .

It looks like this patch forgot to add the `--check-expected-revs`
subcommand to the usage string.

Pranit could you send a fix for that?

Thanks,
Christian.

```

## Junio C Hamano, 2017-11-20 03:05

Subject: Re: [PATCH v16 6/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Message-ID: <xmqqlgj1myyj.fsf@gitster.mtv.corp.google.com>
URL: https://gitlist.dev/e/xmqqlgj1myyj.fsf%40gitster.mtv.corp.google.com
In-Reply-To: <CAP8UFD3SYU_+MOT-TGCV4uJ-cxrd9VhgOOprkq-XP0CUhtGTig@mail.gmail.com>

```
Christian Couder <christian.couder@gmail.com> writes:

> On Fri, Sep 29, 2017 at 8:49 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> Reimplement `is_expected_rev` & `check_expected_revs` shell function in
>> C and add a `--check-expected-revs` subcommand to `git bisect--helper` to
>> call it from git-bisect.sh .
>
> It looks like this patch forgot to add the `--check-expected-revs`
> subcommand to the usage string.

Usage string of what?  "git bisect"?

I do not think bisect--helper even needs to have end-user facing
"usage string", but "git bisect--helper -h" still gets the new thing
listed in its output for free thanks to parse-options API, so that
is not what you found "this patch forgot to add" and you are making
me scratch my head.


```

## Christian Couder, 2017-11-20 07:03

Subject: Re: [PATCH v16 6/6] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Message-ID: <CAP8UFD30RWeXbRDhrxzcrT9_88WhBt-prcNMxC=XwPd7+4UgBA@mail.gmail.com>
URL: https://gitlist.dev/e/CAP8UFD30RWeXbRDhrxzcrT9_88WhBt-prcNMxC%3DXwPd7%2B4UgBA%40mail.gmail.com
In-Reply-To: <xmqqlgj1myyj.fsf@gitster.mtv.corp.google.com>

```
On Mon, Nov 20, 2017 at 4:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> On Fri, Sep 29, 2017 at 8:49 AM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>>> Reimplement `is_expected_rev` & `check_expected_revs` shell function in
>>> C and add a `--check-expected-revs` subcommand to `git bisect--helper` to
>>> call it from git-bisect.sh .
>>
>> It looks like this patch forgot to add the `--check-expected-revs`
>> subcommand to the usage string.
>
> Usage string of what?  "git bisect"?

bisect--helper

> I do not think bisect--helper even needs to have end-user facing
> "usage string", but "git bisect--helper -h" still gets the new thing
> listed in its output for free thanks to parse-options API, so that
> is not what you found "this patch forgot to add" and you are making
> me scratch my head.

Right now in master the usage string for bisect--helper is:

static const char * const git_bisect_helper_usage[] = {
    N_("git bisect--helper --next-all [--no-checkout]"),
    N_("git bisect--helper --write-terms <bad_term> <good_term>"),
    N_("git bisect--helper --bisect-clean-state"),
    NULL
};

So it lists 3 of the command modes out of 4. The missing one is for
--check-expected-revs.
For consistency I think it would be nice to add the missing one.

```