On 2022-08-05 16:15:33+0200, Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
> Fix a logic error in a082345372e (hook API: fix v2.36.0 regression:
> hooks should be connected to a TTY, 2022-06-07). When it started using
> the "ungroup" API added in fd3aaf53f71 (run-command: add an "ungroup"
> option to run_process_parallel(), 2022-06-07) it should have made the
> same sort of change that fd3aaf53f71 itself made in
> "t/helper/test-run-command.c".
>
> The correct way to emit this "Couldn't start" output with "ungroup"
> would be:
>
> fprintf(stderr, _("Couldn't start hook '%s'\n"), hook_path);
>
> But we should instead remove the emitting of this output. As the added
> test shows we already emit output when we can't run the child. The
> "cannot run" output here is emitted by run-command.c's
> child_err_spew().
>
> So the addition of the "Couldn't start hook" output here in
> 96e7225b310 (hook: add 'run' subcommand, 2021-12-22) was always
> redundant. For the pre-commit hook we'll now emit exactly the same
> output as we did before f443246b9f2 (commit: convert
> {pre-commit,prepare-commit-msg} hook to hook.h, 2021-12-22) (and
> likewise for others).
>
> We could at this point add this to the pick_next_hook() callbacks in
> hook.c:
>
> assert(!out);
> assert(!*pp_task_cb);
>
> And this to notify_start_failure() and notify_hook_finished() (in the
> latter case the parameter is called "pp_task_cp"):
>
> assert(!out);
> assert(!pp_task_cb);
>
> But let's leave any such instrumentation for some eventual cleanup of
> the "ungroup" API.
>
> Reported-by: Ilya K <me@xxxxxxxx>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
>
> As suspected I needed to bounce this off the CI once to due to the
> OS-specific messaging around failed hooks, it passes now:
> https://github.com/avar/git/actions/runs/2802782493
>
> hook.c | 7 -------
> t/t1800-hook.sh | 26 ++++++++++++++++++++++++++
> 2 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/hook.c b/hook.c
> index d113ee7faae..a493939a4fc 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -62,9 +62,6 @@ static int pick_next_hook(struct child_process *cp,
> strvec_push(&cp->args, hook_path);
> strvec_pushv(&cp->args, hook_cb->options->args.v);
>
> - /* Provide context for errors if necessary */
> - *pp_task_cb = (char *)hook_path;
> -
> /*
> * This pick_next_hook() will be called again, we're only
> * running one hook, so indicate that no more work will be
> @@ -80,13 +77,9 @@ static int notify_start_failure(struct strbuf *out,
> void *pp_task_cp)
> {
> struct hook_cb_data *hook_cb = pp_cb;
> - const char *hook_path = pp_task_cp;
>
> hook_cb->rc |= 1;
>
> - strbuf_addf(out, _("Couldn't start hook '%s'\n"),
> - hook_path);
> -
> return 1;
> }
>
> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
> index 210f4298872..64096adac7e 100755
> --- a/t/t1800-hook.sh
> +++ b/t/t1800-hook.sh
> @@ -151,4 +151,30 @@ test_expect_success TTY 'git commit: stdout and stderr are connected to a TTY' '
> test_hook_tty commit -m"B.new"
> '
>
> +test_expect_success 'git hook run a hook with a bad shebang' '
> + test_when_finished "rm -rf bad-hooks" &&
> + mkdir bad-hooks &&
> + write_script bad-hooks/test-hook "/bad/path/no/spaces" </dev/null &&
> +
> + # TODO: We should emit the same (or at least a more similar)
> + # error on Windows and !Windows. See the OS-specific code in
> + # start_command()
> + if test_have_prereq !WINDOWS
> + then
> + cat >expect <<-\EOF
> + fatal: cannot run bad-hooks/test-hook: ...
> + EOF
> + else
> + cat >expect <<-\EOF
> + error: cannot spawn bad-hooks/test-hook: ...
> + EOF
> + fi &&
> + test_expect_code 1 git \
> + -c core.hooksPath=bad-hooks \
> + hook run test-hook >out 2>err &&
> + test_must_be_empty out &&
> + sed -e "s/test-hook: .*/test-hook: .../" <err >actual &&
If we're using "sed" here, can we also s/cannot run/cannot spawn/
in order to have the same expectation?
Otherwise, the fix looks sane to me (obviously, since I also suggest
removing the line entirely).
Reviewed-by: Đoàn Trần Công Danh <congdanhqx@xxxxxxxxx>
> + test_cmp expect actual
> +'
> +
> test_done
> --
> 2.37.1.1283.g2c56c4202c5
>
--
Danh
