| Message ID | 93d19d85ee38f50019d5f05605ce7b5eca76cbd6.1643328752.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Finish converting git bisect into a built-in | expand |
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.
Hi Elijah, On Tue, 8 Feb 2022, Elijah Newren wrote: > 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 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
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; }