Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL

[Ramsay Jones <ramsay@ramsayjones.plus.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;
+}