Re: [PATCH 01/11] bisect run: fix the error message

> On Fri, Jan 28, 2022 at 3:27 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > In d1bbbe45df8 (bisect--helper: reimplement `bisect_run` shell function
> > in C, 2021-09-13), we ported the `bisect run` subcommand to C, including
> > the part that prints out an error message when the implicit `git bisect
> > bad` or `git bisect good` failed.
> >
> > However, the error message was supposed to print out whether the state
> > was "good" or "bad", but used a bogus (because non-populated) `args`
> > variable for it.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> >  builtin/bisect--helper.c | 8 ++------
> >  1 file changed, 2 insertions(+), 6 deletions(-)
> >
> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> > index 28a2e6a5750..4208206af07 100644
> > --- a/builtin/bisect--helper.c
> > +++ b/builtin/bisect--helper.c
> > @@ -1093,7 +1093,6 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
> >  {
> >         int res = BISECT_OK;
> >         struct strbuf command = STRBUF_INIT;
> > -       struct strvec args = STRVEC_INIT;
> >         struct strvec run_args = STRVEC_INIT;
> >         const char *new_state;
> >         int temporary_stdout_fd, saved_stdout;
> > @@ -1111,8 +1110,6 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
> >         strvec_push(&run_args, command.buf);
> >
> >         while (1) {
> > -               strvec_clear(&args);
> > -
> >                 printf(_("running %s\n"), command.buf);
> >                 res = run_command_v_opt(run_args.v, RUN_USING_SHELL);
> >
> > @@ -1157,14 +1154,13 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
> >                         printf(_("bisect found first bad commit"));
> >                         res = BISECT_OK;
> >                 } else if (res) {
> > -                       error(_("bisect run failed: 'git bisect--helper --bisect-state"
> > -                       " %s' exited with error code %d"), args.v[0], res);
> > +                       error(_("bisect run failed: 'git bisect"
> > +                       " %s' exited with error code %d"), new_state, res);
> >                 } else {
> >                         continue;
> >                 }
> >
> >                 strbuf_release(&command);
> > -               strvec_clear(&args);
> >                 strvec_clear(&run_args);
> >                 return res;
> >         }
> > --
> > gitgitgadget
>
> Good catch.  Looks like this printed "(null)" on glibc, and probably
> crashed elsewhere.  Perhaps it'd help to add a test that would have
> caught this with something like (I'm hoping gmail doesn't corrupt
> this):
>
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index 1be85d064e..28b54ba41b 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -980,4 +980,15 @@ test_expect_success 'bisect visualize with a
> filename with dash and space' '
>         git bisect visualize -p -- "-hello 2"
>  '
>
> +test_expect_success 'testing' '
> +       git bisect reset &&
> +       git bisect start $HASH4 $HASH1 &&
> +       write_script test_script.sh <<-\EOF &&
> +       rm .git/BISECT*
> +       EOF
> +       test_must_fail git bisect run ./test_script.sh 2>error &&
> +       cat error &&
> +       grep git.bisect.good..exited.with.error.code error
> +'
> +
>  test_done

> Also, as a side note, it appears that another error message in this
> same function has a suboptimal error message, which could be fixed
> with
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 28a2e6a575..6187d9fbcb 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -1118,7 +1118,7 @@ static int bisect_run(struct bisect_terms
> *terms, const char **argv, int argc)
>
>                 if (res < 0 || 128 <= res) {
>                         error(_("bisect run failed: exit code %d from"
> -                               " '%s' is < 0 or >= 128"), res, command.buf);
> +                               " %s is < 0 or >= 128"), res, command.buf);
>                         strbuf_release(&command);
>                         return res;
>                 }
>
> In particular, the line of code just above here:
>       sq_quote_argv(&command, argv);
> means that we get double single quotes without this fix, which looks
> ugly.  Of course, this doesn't need to be included in your series, but
> since you're cleaning up other error messages anyway, I thought I'd at
> least mention it.