Re: [PATCH] hook API: don't segfault on strbuf_addf() to NULL "out"

["Đoàn Trần Công Danh" <congdanhqx@gmail.com>]

On 2022-08-05 16:15:33+0200, Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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@0upti.me>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> 
> 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@gmail.com>

> +	test_cmp expect actual
> +'
> +
>  test_done
> -- 
> 2.37.1.1283.g2c56c4202c5
> 

-- 
Danh