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
> (<Johannes.Schindelin@xxxxxx>) escribió:
> >
> > Hi Miriam,
> >
> > On Thu, 23 Apr 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 | 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()`.
> >
> > > +}
> > > +
> > > +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@xxxxxxxxxxxxxxxxx/
> >
> > 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:
> >
> > > + 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?
> >
> > > +
> > > + 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
>
>
> > > +
> > > 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
> >
> > > 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.
>
