Re: [PATCH v14 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Pranit Bauva writes: > Hey Junio, > > On Thu, Aug 25, 2016 at 3:43 AM, Junio C Hamano wrote: >> Pranit Bauva writes: >> >>> +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) >>> >= 0) { >>> + strbuf_trim(&actual_hex); >>> + res = !strcmp(actual_hex.buf, expected_hex); >> >> If it is known to have 40-hex: >> >> (1) accepting ">= 0" seems way too lenient. You only expect a >> 41-byte file (or 42 if somebody would write CRLF, but I do not >> think anybody other than yourself is expected to write into >> this file, and you do not write CRLF yourself); >> >> (2) strbuf_trim() is overly loose. You only want to trim the >> terimnating LF and it is an error to have other trailing >> whitespaces. >> >> I think the latter is not a new problem and it is OK to leave it >> as-is; limiting (1) to >= 40 may still be a good change, though, >> because it makes the intention of the code clearer. > > I can do the first change. Since this wasn't present in the shell > code, I will also mention it in the commit message. There is no need for that, as that was present in the original. The original compared the string with something that is known to be 40-hex, so anything shorter than 40 wouldn't have matched. It is merely a trivial and obvious optimization to do that in C rewrite.
Re: [PATCH v14 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Hey Junio, On Thu, Aug 25, 2016 at 3:43 AM, Junio C Hamano wrote: > Pranit Bauva writes: > >> +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) >> >= 0) { >> + strbuf_trim(&actual_hex); >> + res = !strcmp(actual_hex.buf, expected_hex); > > If it is known to have 40-hex: > > (1) accepting ">= 0" seems way too lenient. You only expect a > 41-byte file (or 42 if somebody would write CRLF, but I do not > think anybody other than yourself is expected to write into > this file, and you do not write CRLF yourself); > > (2) strbuf_trim() is overly loose. You only want to trim the > terimnating LF and it is an error to have other trailing > whitespaces. > > I think the latter is not a new problem and it is OK to leave it > as-is; limiting (1) to >= 40 may still be a good change, though, > because it makes the intention of the code clearer. I can do the first change. Since this wasn't present in the shell code, I will also mention it in the commit message. Regards, Pranit Bauva -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v14 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
Pranit Bauva writes: > +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) >= > 0) { > + strbuf_trim(&actual_hex); > + res = !strcmp(actual_hex.buf, expected_hex); If it is known to have 40-hex: (1) accepting ">= 0" seems way too lenient. You only expect a 41-byte file (or 42 if somebody would write CRLF, but I do not think anybody other than yourself is expected to write into this file, and you do not write CRLF yourself); (2) strbuf_trim() is overly loose. You only want to trim the terimnating LF and it is an error to have other trailing whitespaces. I think the latter is not a new problem and it is OK to leave it as-is; limiting (1) to >= 40 may still be a good change, though, because it makes the intention of the code clearer. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v14 08/27] bisect--helper: `is_expected_rev` & `check_expected_revs` shell function in C
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 Mentored-by: Lars Schneider Mentored-by: Christian Couder Signed-off-by: Pranit Bauva --- builtin/bisect--helper.c | 33 - git-bisect.sh| 20 ++-- 2 files changed, 34 insertions(+), 19 deletions(-) diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 9aba094..711be75 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -123,13 +123,40 @@ static int bisect_reset(const char *commit) return bisect_clean_state(); } +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) >= 0) { + strbuf_trim(&actual_hex); + res = !strcmp(actual_hex.buf, expected_hex); + } + strbuf_release(&actual_hex); + return res; +} + +static int 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()); + return 0; + } + } + return 0; +} + int cmd_bisect__helper(int argc, const char **argv, const char *prefix) { enum { NEXT_ALL = 1, WRITE_TERMS, BISECT_CLEAN_STATE, - BISECT_RESET + BISECT_RESET, + CHECK_EXPECTED_REVS } cmdmode = 0; int no_checkout = 0; struct option options[] = { @@ -141,6 +168,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) N_("cleanup the bisection state"), BISECT_CLEAN_STATE), OPT_CMDMODE(0, "bisect-reset", &cmdmode, N_("reset the bisection state"), BISECT_RESET), + 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() @@ -167,6 +196,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) if (argc > 1) die(_("--bisect-reset requires either zero or one arguments")); return bisect_reset(argc ? argv[0] : NULL); + case CHECK_EXPECTED_REVS: + return check_expected_revs(argv, argc); default: die("BUG: unknown subcommand '%d'", cmdmode); } diff --git a/git-bisect.sh b/git-bisect.sh index 442397b..c3e43248 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/287 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.htm