Hi Elijah,
On Tue, 8 Feb 2022, Elijah Newren wrote:
> On Fri, Jan 28, 2022 at 3:27 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@xxxxxxxxx> wrote:
> >
> > From: Johannes Schindelin <johannes.schindelin@xxxxxx>
> >
> > 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@xxxxxx>
> > ---
> > 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
Excellent, thank you!
> 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.
Sure, and I think it is not _too_ much out of scope to fix it in the same
patch series, too.
Thank you!
Dscho
