Re: [PATCH v16 1/6] bisect--helper: use OPT_CMDMODE instead of OPT_BOOL
On 29/09/17 07:49, Pranit Bauva wrote:
Show 24 quoted lines
> `--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 --
Show changes to 6 files +44 −60
bisect.c, builtin/am.c, builtin/bisect--helper.c, cache.h, git-bisect.sh, wrapper.c
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
