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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 5, 2022, 15:51 UTC
Message-ID: <220805.86v8r6r8in.gmgdl@evledraar.gmail.com>
In-Reply-To: <Yu08MCCoGwTxeM+g@danh.dev>

On Fri, Aug 05 2022, Đoàn Trần Công Danh wrote:

Show 31 quoted lines

> On 2022-08-05 16:15:33+0200, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> [...]
>> +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).

We could, but then we'd miss some weird regression where we changed the non-Windows message to "fatal: cannot spawn", which neither emit now.

We could of course do the "sed"-ing in an identical "test_have_prereq" block, but I think we'd just be back to square one then, and it would be more readable to just "cat" what we expect to happen there.

So I think I'd prefer to keep it as-is, it also makes a subsequent change where we unify these error messages more obvious, i.e. we'd keep the !WINDOWS branch of that if/else in thath case.

> Reviewed-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>

Thanks!