Hi Miriam,
On Mon, 31 Aug 2020, Miriam Rubio wrote:
> From: Pranit Bauva <pranit.bauva@xxxxxxxxx>
>
> Reimplement the `bisect_state()` shell functions in C and also add a
> subcommand `--bisect-state` to `git-bisect--helper` to call them from
> git-bisect.sh .
>
> Using `--bisect-state` 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 will be called by some
> other methods.
>
> `bisect_head()` is only called from `bisect_state()`, thus it is not
> required to introduce another subcommand.
>
> Mentored-by: Lars Schneider <larsxschneider@xxxxxxxxx>
> Mentored-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
> Mentored-by: Johannes Schindelin <Johannes.Schindelin@xxxxxx>
> Signed-off-by: Pranit Bauva <pranit.bauva@xxxxxxxxx>
> Signed-off-by: Tanushree Tumane <tanushreetumane@xxxxxxxxx>
> Signed-off-by: Miriam Rubio <mirucam@xxxxxxxxx>
> ---
> builtin/bisect--helper.c | 78 +++++++++++++++++++++++++++++++++++++++-
> git-bisect.sh | 55 +++-------------------------
> 2 files changed, 81 insertions(+), 52 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 0796e51672..a976f4739c 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = {
> N_("git bisect--helper --bisect-next"),
> N_("git bisect--helper --bisect-auto-next"),
> N_("git bisect--helper --bisect-autostart"),
> + N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
> + N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
> NULL
> };
>
> @@ -862,6 +864,72 @@ static int bisect_autostart(struct bisect_terms *terms)
> return res;
> }
>
> +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,
> + int argc)
> +{
> + const char *state;
> + int i, verify_expected = 1;
> + struct object_id oid, expected;
> + struct strbuf buf = STRBUF_INIT;
> + struct oid_array revs = OID_ARRAY_INIT;
> +
> + if (!argc)
> + return error(_("Please call `--bisect-state` with at least one argument"));
Good. Technically, this is a deviation from the scripted version, but it
is good not to check and set the terms first, only to then error out
anyway.
However, I think we will need a call to `bisect_autostart()` somewhere
around here, as that is what the scripted version did first thing.
> +
> + state = argv[0];
> + if (check_and_set_terms(terms, state) ||
> + !one_of(state, terms->term_good, terms->term_bad, "skip", NULL))
> + return BISECT_FAILED;
> +
> + argv++;
> + argc--;
> + if (argc > 1 && !strcmp(state, terms->term_bad))
> + return error(_("'git bisect %s' can take only one argument."), terms->term_bad);
> +
> + if (argc == 0) {
> + enum get_oid_result res_head = get_oid("BISECT_HEAD", &oid);
> + if (res_head == MISSING_OBJECT)
> + res_head = get_oid("HEAD", &oid);
> + if (res_head) {
> + error(_("Bad bisect_head rev input"));
If I am not mistaken, we should convert this shell command more faithfully
here:
die "$(eval_gettext "Bad rev input: \$bisected_head")"
i.e. something like this:
const char *head = "BISECT_HEAD";
enum get_oid_result res_head = get_oid(head, &oid);
if (res_head == MISSING_OBJECT) {
head = "HEAD";
res_head = get_oid(head, &oid);
}
if (res_head)
error(_("Bad rev input: %s"), head);
> + return BISECT_FAILED;
> + }
> + oid_array_append(&revs, &oid);
> + }
> +
> + /*
> + * All input revs must be checked before executing bisect_write()
> + * to discard junk revs.
> + */
> +
> + for (; argc; argc--, argv++) {
> + if (get_oid(*argv, &oid)){
> + error(_("Bad rev input: %s"), *argv);
> + return BISECT_FAILED;
> + }
> + oid_array_append(&revs, &oid);
> + }
> +
> + if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||
> + get_oid_hex(buf.buf, &expected) < 0)
> + verify_expected = 0; /* Ignore invalid file contents */
> + strbuf_release(&buf);
> +
> + for (i = 0; i < revs.nr; i++) {
> + if (bisect_write(state, oid_to_hex(&revs.oid[i]), terms, 0))
> + return BISECT_FAILED;
> +
> + if (verify_expected && !oideq(&revs.oid[i], &expected)) {
> + unlink_or_warn(git_path_bisect_ancestors_ok());
> + unlink_or_warn(git_path_bisect_expected_rev());
> + verify_expected = 0;
> + }
> + }
Nice! This reads so much easier than the shell script version.
> +
> + oid_array_clear(&revs);
> + return bisect_auto_next(terms, NULL);
> +}
> +
> int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> {
> enum {
> @@ -875,7 +943,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> BISECT_START,
> BISECT_NEXT,
> BISECT_AUTO_NEXT,
> - BISECT_AUTOSTART
> + BISECT_AUTOSTART,
> + BISECT_STATE
> } cmdmode = 0;
> int res = 0, nolog = 0;
> struct option options[] = {
> @@ -901,6 +970,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
> OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
> N_("start the bisection if it has not yet been started"), BISECT_AUTOSTART),
> + OPT_CMDMODE(0, "bisect-state", &cmdmode,
> + N_("mark the state of ref (or refs)"), BISECT_STATE),
> OPT_BOOL(0, "no-log", &nolog,
> N_("no log for BISECT_WRITE")),
> OPT_END()
> @@ -971,6 +1042,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> set_terms(&terms, "bad", "good");
> res = bisect_autostart(&terms);
> break;
> + case BISECT_STATE:
> + set_terms(&terms, "bad", "good");
> + get_terms(&terms);
> + res = bisect_state(&terms, argv, argc);
> + break;
> default:
> BUG("unknown subcommand %d", cmdmode);
> }
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 356264caf0..7a8f796251 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -39,16 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
> TERM_BAD=bad
> TERM_GOOD=good
>
> -bisect_head()
> -{
> - if git rev-parse --verify -q BISECT_HEAD > /dev/null
> - then
> - echo BISECT_HEAD
> - else
> - echo HEAD
> - fi
> -}
> -
> bisect_skip() {
> all=''
> for arg in "$@"
> @@ -61,43 +51,7 @@ bisect_skip() {
> esac
> all="$all $revs"
> done
> - eval bisect_state 'skip' $all
> -}
> -
> -bisect_state() {
> - git bisect--helper --bisect-autostart
> - state=$1
> - git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
> - get_terms
> - case "$#,$state" in
> - 0,*)
> - die "Please call 'bisect_state' with at least one argument." ;;
> - 1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip)
> - bisected_head=$(bisect_head)
> - rev=$(git rev-parse --verify "$bisected_head") ||
> - die "$(eval_gettext "Bad rev input: \$bisected_head")"
> - git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
> - git bisect--helper --check-expected-revs "$rev" ;;
> - 2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
> - shift
> - hash_list=''
> - for rev in "$@"
> - do
> - sha=$(git rev-parse --verify "$rev^{commit}") ||
> - die "$(eval_gettext "Bad rev input: \$rev")"
> - hash_list="$hash_list $sha"
> - done
> - for rev in $hash_list
> - do
> - git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
> - done
> - git bisect--helper --check-expected-revs $hash_list ;;
> - *,"$TERM_BAD")
> - die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;;
> - *)
> - usage ;;
> - esac
> - git bisect--helper --bisect-auto-next
> + eval git bisect--helper --bisect-state 'skip' $all
Since `bisect--helper` cannot cause the shell script to `exit`, the `eval`
should no longer be necessary.
> }
>
> bisect_visualize() {
> @@ -187,8 +141,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> state="$TERM_GOOD"
> fi
>
> - # We have to use a subshell because "bisect_state" can exit.
> - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
> + git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN"
> res=$?
>
> cat "$GIT_DIR/BISECT_RUN"
> @@ -203,7 +156,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> if [ $res -ne 0 ]
> then
> eval_gettextln "bisect run failed:
> -'bisect_state \$state' exited with error code \$res" >&2
> +'git bisect--helper --bisect-state \$state' exited with error code \$res" >&2
I don't think we should include implementation details such as the use of
`bisect--helper` in the error message. Personally, I would either keep
`bisect_state` or paraphrase what failed in English to begin with.
Thank you,
Johannes
> exit $res
> fi
>
> @@ -244,7 +197,7 @@ case "$#" in
> start)
> git bisect--helper --bisect-start "$@" ;;
> bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
> - bisect_state "$cmd" "$@" ;;
> + git bisect--helper --bisect-state "$cmd" "$@" ;;
> skip)
> bisect_skip "$@" ;;
> next)
> --
> 2.25.0
>
>
