Hi Miriam,

On Sat, 20 Jun 2020, Miriam R. wrote:

Hi Johannes,
I'm finishing the next patch series version and I have an issue about
one of your suggestions:

El sáb., 23 may. 2020 a las 0:06, Johannes Schindelin
([off-list ref]) escribió:

quoted

Hi Miriam,

On Thu, 23 Apr 2020, Miriam Rubio wrote:

quoted

From: Pranit Bauva <redacted>

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 [off-list ref]
Mentored-by: Christian Couder [off-list ref]
Mentored-by: Johannes Schindelin [off-list ref]
Signed-off-by: Pranit Bauva <redacted>
Signed-off-by: Tanushree Tumane <redacted>
Signed-off-by: Miriam Rubio <redacted>
---
 builtin/bisect--helper.c | 70 +++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 55 +++----------------------------
 2 files changed, 73 insertions(+), 52 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 2d8660c79f..9db72f5891 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
 };

@@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms)
      return bisect_start(terms, 0, NULL, 0);
 }

+static int bisect_head(struct object_id *oid)
+{
+     if (!file_exists(git_path_bisect_head()))
+             return get_oid("HEAD", oid);
+
+     return get_oid("BISECT_HEAD", oid);

This can be easily reduced to

        return get_oid(file_exists(git_path_bisect_head()) ?
                       "BISECT_HEAD" : "HEAD", oid);

At the same time, it is wrong, just like the shell script version was
wrong: in particular in light of the `hn/reftable` effort, we do _not_
want to assume that all refs are backed by files!

So really, what this should do instead is this:

        enum get_oid_result res = get_oid("BISECT_HEAD", oid);

        if (res == MISSING_OBJECT)
                res = get_oid("HEAD", oid);

Given that this is still only three lines long, the overhead of having it
in its own function for just a _single_ call seems excessive. I'd prefer
it to be inlined in `bisect_state()`.

quoted

+}
+
+static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,
+                     int argc)
+{

I offered a lengthy discussion about this function in
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002272244150.9783@tvgsbejvaqbjf.bet/ (local)

It does not look, however, as if v3 benefitted from the entirety of my
analysis: All the `check_expected_revs()` function does is to verify that
the passed list of revs matches exactly the contents of the
`BISECT_EXPECTED_REV` file.

That can be done in a much simpler way, though, by first reading the file
and parsing the contents into an OID, and then comparing to that parsed
OID instead.

Besides, `check_expected_revs()` is only used to check one rev at a time.

In other words, it could be simplified to something like this:

static void check_expected_rev(struct object_id *oid)
{
        struct object_id expected;
        struct strbuf buf = STRBUF_INIT;

        if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0)
              < the_hash_algo->hexsz ||
            get_oid_hex(buf.buf, &expected) < 0)
                return; /* Ignore invalid file contents */

        if (!oideq(oid, &expected)) {
                ... unlink ...
                return;
        }
}

But even that would be wasteful, as we would read the file over and over
and over again.

The good news is that we do not even _need_ `check_expected_rev()`.
Because we do not need to have two call sites, we can simplify the code
much further. See below:

quoted

+     const char *state;
+     const char *hex;
+     int i;
+     struct oid_array revs = OID_ARRAY_INIT;
+     struct object_id oid;
+
+     if (!argc)
+             return error(_("Please call `--bisect-state` with at least one argument"));
+     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 (!strcmp(state, terms->term_bad) && (argc > 1))
+             return error(_("'git bisect %s' can take only one argument."),terms->term_bad);
+     if (argc == 0) {
+             if (bisect_head(&oid))
+                     return error(_("Bad bisect_head rev input"));
+             hex = oid_to_hex(&oid);
+             if (bisect_write(state, hex, terms, 0))
+                     return BISECT_FAILED;
+             check_expected_revs(&hex, 1);
+             return bisect_auto_next(terms, NULL);
+     }
+
+     /* Here argc > 0 */
+     for (; argc; argc--, argv++) {
+             struct object_id oid;
+             if (get_oid(*argv, &oid))
+                     return error(_("Bad rev input: %s"), *argv);
+             oid_array_append(&revs, &oid);
+     }

It really does not make sense to parse the arguments into an OID array,
_then_ iterate over the array once, and then immediately releasing it.
That OID array is not needed at all.

So we'll end up with this loop in case `argc > 0` (where we now call
`get_oid()`, too), and note how the loop body looks _eerily_ similar to
the conditional `argc == 0` code block above?

quoted

+
+     for (i = 0; i < revs.nr; i++) {
+             hex = oid_to_hex(&revs.oid[i]);
+             if (bisect_write(state, hex, terms, 0)) {
+                     oid_array_clear(&revs);
+                     return BISECT_FAILED;
+             }
+             check_expected_revs(&hex, 1);
+     }
+
+     oid_array_clear(&revs);
+     return bisect_auto_next(terms, NULL);
+}

So really, this function pretty much _wants_ to look this way (modulo
bugs, as I did not even test-compile the code):

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;

        if (!argc)
                return error(_("Please call `--bisect-state` with at least one argument"));

        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 (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 */


        for (i = 0; i < argc + !argc; i++) {
                if (argc) {
                        if (get_oid(argv[i], &oid)) {
                                error(_("Bad rev input: %s"), *argv);
                                return BISECT_FAILED;
                        }
                } else {
                        enum get_oid_result res = get_oid("BISECT_HEAD", &oid);

                        if (res == MISSING_OBJECT)
                                res = get_oid("HEAD", &oid);
                        if (res) {
                                error(_("Bad bisect_head rev input"));
                                return BISECT_FAILED;
                        }
                }

                if (bisect_write(state, oid_to_hex(&oid), terms, 0))
                        return BISECT_FAILED;

                if (verify_expected && !oideq(&oid, &expected)) {
                        unlink_or_warn(git_path_bisect_ancestors_ok());
                        unlink_or_warn(git_path_bisect_expected_rev());
                        verify_expected = 0;
                }
        }

        return bisect_auto_next(terms, NULL);
}

There, not bad, is it?

 After implementing this solution some tests failed. After debugging
them, I found that with Pranit's solution, that arguments were parsed
into an OID array, if bisect received some junk rev the function
returned and bisect_write() was not executed.

That's a good point, and should be turned into a code comment lest
overzealous developers would attempt the simplification I proposed in the
future.

With the new solution, if junk rev is received after a valid rev,
bisect_write() was executed for the valid and the function returned with
the junk rev.
So, there is garbage in the file and when for example bisect-porcelain
test number 5 - 'bisect fails if given any junk instead of revs'
executes 'test -z' fails.

Should I keep the original patch and add a comment in the code that
explains why we use an oid array?.

I still think that the `check_expected_revs()` function is not needed, and
unnecessarily complex.

If you _want_ to keep Pranit's original implementation, then I would _at
least_ want you to forward-port the `verify_expected` changes I suggested.

(I also have implemented an alternative solution that when some junk
rev is found, I delete all refs written, but maybe is too complicated
or not totally correct:
https://gitlab.com/mirucam/git/-/commit/93f669877b87d09a30a5d07f0967667b22026511
)

You seem to share my preference for not writing out bogus revs in the
first place.

Thanks,
Johannes

quoted

quoted

+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
      enum {

@@ -847,7 +907,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 no_checkout = 0, res = 0, nolog = 0;
      struct option options[] = {

@@ -873,6 +934,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 BISECT_START is empty or missing"), BISECT_AUTOSTART),
+             OPT_CMDMODE(0, "bisect-state", &cmdmode,
+                      N_("mark the state of ref (or refs)"), BISECT_STATE),
              OPT_BOOL(0, "no-checkout", &no_checkout,
                       N_("update BISECT_HEAD instead of checking out the current commit")),
              OPT_BOOL(0, "no-log", &nolog,

@@ -945,6 +1008,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", (int)cmdmode);
      }

diff --git a/git-bisect.sh b/git-bisect.sh
index 049ffacdff..2da0810b1a 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 test -f "$GIT_DIR/BISECT_HEAD"
-     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
 }

 bisect_visualize() {

@@ -185,8 +139,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"

@@ -201,7 +154,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

This is not your fault, of course, but it does make me shudder to see such
an obvious implementation detail in a user-facing error message.

Maybe something to fix up in a follow-up?

Ciao,
Dscho

quoted

                      exit $res
              fi

@@ -242,7 +195,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

Best,
Miriam.