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

[Elijah Newren <newren@gmail.com>]

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.