Re: [PATCH v6 01/17] hook: add 'run' subcommand
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 7, 2022, 23:08 UTC
- Message-ID
- <xmqqfspz5ekc.fsf@gitster.g>
- In-Reply-To
- <Ydi2XEvZywEyImfC@google.com>
Emily Shaffer <emilyshaffer@google.com> writes:
Show 33 quoted lines
> On Wed, Dec 22, 2021 at 04:59:27AM +0100, Ævar Arnfjörð Bjarmason wrote:
>> diff --git a/hook.h b/hook.h
>> +struct run_hooks_opt
>> +{
>> + /* Environment vars to be set for each hook */
>> + struct strvec env;
>> +
>> + /* Args to be passed to each hook */
>> + struct strvec args;
>> +
>> + /* Emit an error if the hook is missing */
>> + unsigned int error_if_missing:1;
>
> I wonder if it's premature to include error_if_missing, if we will
> always set it to 1 until way down in patch 11.
>
> But I do not care all that much - at this point, I'll be honest, I'd
> just like to see the series merged.
>
>> diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
>> new file mode 100755
>> +test_expect_success 'git hook run: nonexistent hook' '
>> + cat >stderr.expect <<-\EOF &&
>> + error: cannot find a hook named test-hook
>> + EOF
>> + test_expect_code 1 git hook run test-hook 2>stderr.actual &&
>> + test_cmp stderr.expect stderr.actual
>> +'
>
> It's a little unclear to me what "nonexistent" means - does it mean that
> it's a hook unknown to Git (e.g. not in hook-list.h)? Does it mean that
> the hook path doesn't exist? Does it mean the hook path is not
> executable?I think I saw the same puzzlement from another reviewer. It needs to be clarified if the documentation patch does not do a good job.
> It might be nice to at least leave a comment on this test case and point > out that it relies on nobody having created a test-hook script in an > earlier test. Or to clean up that path just in case, at the beginning of > this one.
Yup, I agree that the latter is a good practice.