On Fri, Aug 5, 2022 at 1:27 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
> -- >8 --
> Subject: [PATCH] hook API: don't segfault on strbuf_addf() to NULL "out"
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> 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().
Nice, thanks. And child_err_spew() gives argv[0] in most cases, so
it's the same context as we'd get from this line anyway.
>
> 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.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
Reviewed-by: Emily Shaffer <emilyshaffer@xxxxxxxxxx>
> ---
> hook.c | 7 -------
> t/t1800-hook.sh | 15 +++++++++++++++
> 2 files changed, 15 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..75f72ffbf27 100755
> --- a/t/t1800-hook.sh
> +++ b/t/t1800-hook.sh
> @@ -151,4 +151,19 @@ 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 &&
> + cat >expect <<-\EOF &&
> + fatal: cannot run bad-hooks/test-hook: ...
> + EOF
> + test_expect_code 1 git \
> + -c core.hooksPath=bad-hooks \
> + hook run test-hook >out 2>actual.raw &&
> + test_must_be_empty out &&
> + sed -e "s/test-hook: .*/test-hook: .../" <actual.raw >actual &&
> + test_cmp expect actual
> +'
> +
> test_done
> --
