From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Fri, 05 Aug 2022 15:51:17 GMT
Subject: Re: [PATCH] hook API: don't segfault on strbuf_addf() to NULL "out"
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:

> 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!