[8/8] hook-list.h: add a generated list of hooks, like config-list.h

Commit Message

Ævar Arnfjörð Bjarmason Sept. 23, 2021, 10:30 a.m. UTC

Make githooks(5) the source of truth for what hooks git supports, and
punt out early on hooks we don't know about in find_hook(). This
ensures that the documentation and the C code's idea about existing
hooks doesn't diverge.

We still have Perl and Python code running its own hooks, but that'll
be addressed by Emily Shaffer's upcoming "git hook run" command.

This resolves a long-standing TODO item in bugreport.c of there being
no centralized listing of hooks, and fixes a bug with the bugreport
listing only knowing about 1/4 of the p4 hooks. It didn't know about
the recent "reference-transaction" hook either.

We could make the find_hook() function die() or BUG() out if the new
known_hook() returned 0, but let's make it return NULL just as it does
when it can't find a hook of a known type. Making it die() is overly
anal, and unlikely to be what we need in catching stupid typos in the
name of some new hook hardcoded in git.git's sources. By making this
be tolerant of unknown hook names, changes in a later series to make
"git hook run" run arbitrary user-configured hook names will be easier
to implement.

I have not been able to directly test the CMake change being made
here. Since 4c2c38e800 (ci: modification of main.yml to use cmake for
vs-build job, 2020-06-26) some of the Windows CI has a hard dependency
on CMake, this change works there, and is to my eyes an obviously
correct use of a pattern established in previous CMake changes,
namely:

 - 061c2240b1 (Introduce CMake support for configuring Git,
    2020-06-12)
 - 709df95b78 (help: move list_config_help to builtin/help,
    2020-04-16)
 - 976aaedca0 (msvc: add a Makefile target to pre-generate the Visual
   Studio solution, 2019-07-29)

The LC_ALL=C is needed because at least in my locale the dash ("-") is
ignored for the purposes of sorting, which results in a different
order. I'm not aware of anything in git that has a hard dependency on
the order, but e.g. the bugreport output would end up using whatever
locale was in effect when git was compiled.

Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
Helped-by: René Scharfe <[e-mail]>
---
 .gitignore                          |  1 +
 Makefile                            |  9 +++++-
 builtin/bugreport.c                 | 44 ++++++-----------------------
 contrib/buildsystems/CMakeLists.txt |  7 +++++
 generate-hooklist.sh                | 18 ++++++++++++
 5 files changed, 42 insertions(+), 37 deletions(-)
 create mode 100755 generate-hooklist.sh

Comments

Phillip Wood Sept. 24, 2021, 10:19 a.m. UTC | #1

Hi Ævar
On 23/09/2021 11:30, Ævar Arnfjörð Bjarmason wrote:
> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
> new file mode 100755
> index 00000000000..6d4e56d1a31
> --- /dev/null
> +++ b/generate-hooklist.sh
> @@ -0,0 +1,18 @@
> +#!/bin/sh
> +#
> +# Usage: ./generate-hooklist.sh >hook-list.h
> +
> +cat <<EOF
> +/* Automatically generated by generate-hooklist.sh */
> +
> +static const char *hook_name_list[] = {
> +EOF
> +
> +sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \

POSIX does not support using a semicolon after a closing brace [1], 
grepping our code base with
	git grep 'sed .*};' '*.sh'
does not give any matches so I don't think we're using that pattern any 
where else. Replacing the semicolon with ' -e' would fix it.

Best Wishes

Phillip

[1] "Editing commands other that {...}, a, b, c, i, r, t, w, : and # can 
be followed by a <semicolon>" from 
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html

Junio C Hamano Sept. 24, 2021, 3:51 p.m. UTC | #2

Phillip Wood <[e-mail]> writes:

>> +sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
>
> POSIX does not support using a semicolon after a closing brace [1],
> ...
> [1] "Editing commands other that {...}, a, b, c, i, r, t, w, : and #
> can be followed by a <semicolon>" from 
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html

Would sed implementation of BSD ancestry fail reliably to be a good
coalmine canary?

René Scharfe Sept. 24, 2021, 4:39 p.m. UTC | #3

Am 24.09.21 um 17:51 schrieb Junio C Hamano:
> Phillip Wood <[e-mail]> writes:
>
>>> +sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
>>
>> POSIX does not support using a semicolon after a closing brace [1],
>> ...
>> [1] "Editing commands other that {...}, a, b, c, i, r, t, w, : and #
>> can be followed by a <semicolon>" from
>> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html
>
> Would sed implementation of BSD ancestry fail reliably to be a good
> coalmine canary?

I doubt it.  https://man.openbsd.org/sed says:

   "Multiple commands may be specified separated by newlines or
    semicolons"

   "The a, c, i, r, and w functions cannot be followed by another
    command separated with a semicolon."

   "Following the b, t, or : commands with a semicolon and another
    command is an extension to the specification."

   "The use of newlines to separate multiple commands on the command
    line is non-portable"

https://www.freebsd.org/cgi/man.cgi?query=sed doesn't mention semicolons
at all.

On macOS 11.6 (FreeBSD-based userland) the semicolon is accepted:

   $ echo a | sed -n -e '{p;}; p'
   a
   a

René

Ævar Arnfjörð Bjarmason Sept. 24, 2021, 7:30 p.m. UTC | #4

On Fri, Sep 24 2021, Phillip Wood wrote:

> Hi Ævar
> On 23/09/2021 11:30, Ævar Arnfjörð Bjarmason wrote:
>> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
>> new file mode 100755
>> index 00000000000..6d4e56d1a31
>> --- /dev/null
>> +++ b/generate-hooklist.sh
>> @@ -0,0 +1,18 @@
>> +#!/bin/sh
>> +#
>> +# Usage: ./generate-hooklist.sh >hook-list.h
>> +
>> +cat <<EOF
>> +/* Automatically generated by generate-hooklist.sh */
>> +
>> +static const char *hook_name_list[] = {
>> +EOF
>> +
>> +sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
>
> POSIX does not support using a semicolon after a closing brace [1],
> grepping our code base with
> 	git grep 'sed .*};' '*.sh'
> does not give any matches so I don't think we're using that pattern
> any where else. Replacing the semicolon with ' -e' would fix it.
>
> Best Wishes
>
> Phillip

Does this fail on any system you're aware of? If so what OS/version (and
preferably version of "sed").

René's downthread <[e-mail]> seems to
suggest that this is fine.

Both beforehand and just now I've tested this on AIX, Solaris,
{Open,Net,Free}BSD, HP/UX, OSX and Linux (a few distros/versions).

All of them are able to generate the same hook-list.h using this version
of the patch.

René Scharfe Sept. 24, 2021, 7:56 p.m. UTC | #5

Am 24.09.21 um 21:30 schrieb Ævar Arnfjörð Bjarmason:
>
> On Fri, Sep 24 2021, Phillip Wood wrote:
>
>> Hi Ævar
>> On 23/09/2021 11:30, Ævar Arnfjörð Bjarmason wrote:
>>> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
>>> new file mode 100755
>>> index 00000000000..6d4e56d1a31
>>> --- /dev/null
>>> +++ b/generate-hooklist.sh
>>> @@ -0,0 +1,18 @@
>>> +#!/bin/sh
>>> +#
>>> +# Usage: ./generate-hooklist.sh >hook-list.h
>>> +
>>> +cat <<EOF
>>> +/* Automatically generated by generate-hooklist.sh */
>>> +
>>> +static const char *hook_name_list[] = {
>>> +EOF
>>> +
>>> +sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
>>
>> POSIX does not support using a semicolon after a closing brace [1],
>> grepping our code base with
>> 	git grep 'sed .*};' '*.sh'
>> does not give any matches so I don't think we're using that pattern
>> any where else. Replacing the semicolon with ' -e' would fix it.
>>
>> Best Wishes
>>
>> Phillip
>
> Does this fail on any system you're aware of? If so what OS/version (and
> preferably version of "sed").

I wasn't aware of that restriction and my gut feeling is that enforcing
it in a sed implementation would be slightly harder than allowing "};".

> René's downthread <[e-mail]> seems to
> suggest that this is fine.

That said, I didn't mean to suggest that we keep using this construct,
just that I couldn't find an implementation which would reject it.

> Both beforehand and just now I've tested this on AIX, Solaris,
> {Open,Net,Free}BSD, HP/UX, OSX and Linux (a few distros/versions).
>
> All of them are able to generate the same hook-list.h using this version
> of the patch.

Impressive list, but still it's probably better to move the last "x" to
its own -e, to appease the POSIX spirits.  Small change..

René

Ævar Arnfjörð Bjarmason Sept. 24, 2021, 8:09 p.m. UTC | #6

On Fri, Sep 24 2021, René Scharfe wrote:

> Am 24.09.21 um 21:30 schrieb Ævar Arnfjörð Bjarmason:
>>
>> On Fri, Sep 24 2021, Phillip Wood wrote:
>>
>>> Hi Ævar
>>> On 23/09/2021 11:30, Ævar Arnfjörð Bjarmason wrote:
>>>> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
>>>> new file mode 100755
>>>> index 00000000000..6d4e56d1a31
>>>> --- /dev/null
>>>> +++ b/generate-hooklist.sh
>>>> @@ -0,0 +1,18 @@
>>>> +#!/bin/sh
>>>> +#
>>>> +# Usage: ./generate-hooklist.sh >hook-list.h
>>>> +
>>>> +cat <<EOF
>>>> +/* Automatically generated by generate-hooklist.sh */
>>>> +
>>>> +static const char *hook_name_list[] = {
>>>> +EOF
>>>> +
>>>> +sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
>>>
>>> POSIX does not support using a semicolon after a closing brace [1],
>>> grepping our code base with
>>> 	git grep 'sed .*};' '*.sh'
>>> does not give any matches so I don't think we're using that pattern
>>> any where else. Replacing the semicolon with ' -e' would fix it.
>>>
>>> Best Wishes
>>>
>>> Phillip
>>
>> Does this fail on any system you're aware of? If so what OS/version (and
>> preferably version of "sed").
>
> I wasn't aware of that restriction and my gut feeling is that enforcing
> it in a sed implementation would be slightly harder than allowing "};".
>
>> René's downthread <[e-mail]> seems to
>> suggest that this is fine.
>
> That said, I didn't mean to suggest that we keep using this construct,
> just that I couldn't find an implementation which would reject it.

I'll re-test without "};", my (seemingly incorrect) quick read of your
E-Mail + my own testing was that semicolons were fine there, but maybe
they're just accepted by most...

>> Both beforehand and just now I've tested this on AIX, Solaris,
>> {Open,Net,Free}BSD, HP/UX, OSX and Linux (a few distros/versions).
>>
>> All of them are able to generate the same hook-list.h using this version
>> of the patch.
>
> Impressive list.

FWIW that's just the GCC Farm which is fairly easy to get access to for
free software devlopment: https://cfarm.tetaneutral.net/machines/list/

The HP/UX box is a similar "private" setup mainly for Perl5 core
development, I'm getting to use it to get git working there.

> but still it's probably better to move the last "x" to its own -e, to
> appease the POSIX spirits.  Small change..

*nod*, will test with some version of that.

Phillip Wood Sept. 27, 2021, 9:24 a.m. UTC | #7

On 24/09/2021 20:30, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Sep 24 2021, Phillip Wood wrote:
> 
>> Hi Ævar
>> On 23/09/2021 11:30, Ævar Arnfjörð Bjarmason wrote:
>>> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
>>> new file mode 100755
>>> index 00000000000..6d4e56d1a31
>>> --- /dev/null
>>> +++ b/generate-hooklist.sh
>>> @@ -0,0 +1,18 @@
>>> +#!/bin/sh
>>> +#
>>> +# Usage: ./generate-hooklist.sh >hook-list.h
>>> +
>>> +cat <<EOF
>>> +/* Automatically generated by generate-hooklist.sh */
>>> +
>>> +static const char *hook_name_list[] = {
>>> +EOF
>>> +
>>> +sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
>>
>> POSIX does not support using a semicolon after a closing brace [1],
>> grepping our code base with
>> 	git grep 'sed .*};' '*.sh'
>> does not give any matches so I don't think we're using that pattern
>> any where else. Replacing the semicolon with ' -e' would fix it.
>>
>> Best Wishes
>>
>> Phillip
> 
> Does this fail on any system you're aware of? If so what OS/version (and
> preferably version of "sed").

I'm not aware of any such system but I rarely use anything other than 
linux. As this departure from POSIX is not already in the code base I 
thought it was worth flagging it. I did wonder if it would be supported 
by the various BSDs but you testing shows it is actually quite widely 
supported.

Best Wishes

Phillip

> René's downthread <[e-mail]> seems to
> suggest that this is fine.
> 
> Both beforehand and just now I've tested this on AIX, Solaris,
> {Open,Net,Free}BSD, HP/UX, OSX and Linux (a few distros/versions).
> 
> All of them are able to generate the same hook-list.h using this version
> of the patch.
>

Ævar Arnfjörð Bjarmason Sept. 27, 2021, 10:36 a.m. UTC | #8

On Mon, Sep 27 2021, Phillip Wood wrote:

> On 24/09/2021 20:30, Ævar Arnfjörð Bjarmason wrote:
>> On Fri, Sep 24 2021, Phillip Wood wrote:
>> 
>>> Hi Ævar
>>> On 23/09/2021 11:30, Ævar Arnfjörð Bjarmason wrote:
>>>> diff --git a/generate-hooklist.sh b/generate-hooklist.sh
>>>> new file mode 100755
>>>> index 00000000000..6d4e56d1a31
>>>> --- /dev/null
>>>> +++ b/generate-hooklist.sh
>>>> @@ -0,0 +1,18 @@
>>>> +#!/bin/sh
>>>> +#
>>>> +# Usage: ./generate-hooklist.sh >hook-list.h
>>>> +
>>>> +cat <<EOF
>>>> +/* Automatically generated by generate-hooklist.sh */
>>>> +
>>>> +static const char *hook_name_list[] = {
>>>> +EOF
>>>> +
>>>> +sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
>>>
>>> POSIX does not support using a semicolon after a closing brace [1],
>>> grepping our code base with
>>> 	git grep 'sed .*};' '*.sh'
>>> does not give any matches so I don't think we're using that pattern
>>> any where else. Replacing the semicolon with ' -e' would fix it.
>>>
>>> Best Wishes
>>>
>>> Phillip
>> Does this fail on any system you're aware of? If so what OS/version
>> (and
>> preferably version of "sed").
>
> I'm not aware of any such system but I rarely use anything other than
> linux. As this departure from POSIX is not already in the code base I 
> thought it was worth flagging it. I did wonder if it would be
> supported by the various BSDs but you testing shows it is actually
> quite widely supported.
>
> Best Wishes
>
> Phillip

Thanks, I completely agree that it's worth changing either way (as I did
in my v2). I just wondered if it was a careful reading of the standard
or having run into it on a system in the wild, in that case I'd try to
test on that system in the future.

Good eyes :)

Mike Hommey Nov. 15, 2021, 10:04 p.m. UTC | #9

On Thu, Sep 23, 2021 at 12:30:03PM +0200, Ævar Arnfjörð Bjarmason wrote:
> -builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX
> +hook.sp hook.s hook.o: hook-list.h
> +
> +builtin/help.sp builtin/help.s builtin/help.o: config-list.h hook-list.h GIT-PREFIX

hook-list.h is only included from buitin/bugreport.c, so
builtin/bugreport.o should be the one with the hook-list.h dependency,
shouldn't it?

Mike

Ævar Arnfjörð Bjarmason Nov. 15, 2021, 10:26 p.m. UTC | #10

On Tue, Nov 16 2021, Mike Hommey wrote:

> On Thu, Sep 23, 2021 at 12:30:03PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> -builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX
>> +hook.sp hook.s hook.o: hook-list.h
>> +
>> +builtin/help.sp builtin/help.s builtin/help.o: config-list.h hook-list.h GIT-PREFIX
>
> hook-list.h is only included from buitin/bugreport.c, so
> builtin/bugreport.o should be the one with the hook-list.h dependency,
> shouldn't it?

Well spotted, yes. This is a mistake. I think from some earlier WIP
version of the series.

In practice we don't really miss dependencies due to these sorts of
mistakes since we use the .depends files, i.e. GCC & Clang figure this
out for us:

    $ grep hook-list .depend/* */.depend/*
    builtin/.depend/bugreport.o.d: compat/compiler.h git-compat-util.h hook.h hook-list.h
    builtin/.depend/bugreport.o.d:hook-list.h:

But we do over-depend a bit, if you touch hook-list.h and make
builtin/help.o we'll re-generate it due to this line.

If you or anyone else is more generally interested in the Makefile I'd
really like to get some reviews over at:
https://lore.kernel.org/git/[e-mail]/

I've got some follow-up work that solve these dependencies more
generally, e.g. in this case we should really not need to slavishly
maintain these fallback dependency lists by hand, or have automated ways
of validating their correctness.

Mike Hommey Nov. 15, 2021, 10:40 p.m. UTC | #11

On Mon, Nov 15, 2021 at 11:26:36PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Nov 16 2021, Mike Hommey wrote:
> 
> > On Thu, Sep 23, 2021 at 12:30:03PM +0200, Ævar Arnfjörð Bjarmason wrote:
> >> -builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX
> >> +hook.sp hook.s hook.o: hook-list.h
> >> +
> >> +builtin/help.sp builtin/help.s builtin/help.o: config-list.h hook-list.h GIT-PREFIX
> >
> > hook-list.h is only included from buitin/bugreport.c, so
> > builtin/bugreport.o should be the one with the hook-list.h dependency,
> > shouldn't it?
> 
> Well spotted, yes. This is a mistake. I think from some earlier WIP
> version of the series.
> 
> In practice we don't really miss dependencies due to these sorts of
> mistakes since we use the .depends files, i.e. GCC & Clang figure this
> out for us:
> 
>     $ grep hook-list .depend/* */.depend/*
>     builtin/.depend/bugreport.o.d: compat/compiler.h git-compat-util.h hook.h hook-list.h
>     builtin/.depend/bugreport.o.d:hook-list.h:

But aren't those .depends files are only created when compiling object
files, such that builtin/.depend/bugreport.o.d wouldn't exist until
bugreport.c is compiled, which would fail if hook-list.h wasn't created
before that?

Mike

Ævar Arnfjörð Bjarmason Nov. 15, 2021, 10:49 p.m. UTC | #12

On Tue, Nov 16 2021, Mike Hommey wrote:

> On Mon, Nov 15, 2021 at 11:26:36PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Tue, Nov 16 2021, Mike Hommey wrote:
>> 
>> > On Thu, Sep 23, 2021 at 12:30:03PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> >> -builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX
>> >> +hook.sp hook.s hook.o: hook-list.h
>> >> +
>> >> +builtin/help.sp builtin/help.s builtin/help.o: config-list.h hook-list.h GIT-PREFIX
>> >
>> > hook-list.h is only included from buitin/bugreport.c, so
>> > builtin/bugreport.o should be the one with the hook-list.h dependency,
>> > shouldn't it?
>> 
>> Well spotted, yes. This is a mistake. I think from some earlier WIP
>> version of the series.
>> 
>> In practice we don't really miss dependencies due to these sorts of
>> mistakes since we use the .depends files, i.e. GCC & Clang figure this
>> out for us:
>> 
>>     $ grep hook-list .depend/* */.depend/*
>>     builtin/.depend/bugreport.o.d: compat/compiler.h git-compat-util.h hook.h hook-list.h
>>     builtin/.depend/bugreport.o.d:hook-list.h:
>
> But aren't those .depends files are only created when compiling object
> files, such that builtin/.depend/bugreport.o.d wouldn't exist until
> bugreport.c is compiled, which would fail if hook-list.h wasn't created
> before that?

Fail how?

I don't think it could fail, because the purpose of these dependency
relationships is to avoid needless *re*builds. So if you're building for
the first time it doesn't matter, your compiler will find the relevant
things to include for you. It doesn't need what's in the Makefile to do
that.

See [1], what I said about LIB_H there applies more generally for the
.depends files.

It will only fail in the sense that it over-depends, i.e. if you do:

    git clean -dxf; make builtin/help.o

We'll generate the hook list, as opposed to for buildin/add.o or
whatever, but I'd say that generally doesn't matter all that much, what
matters is that we don't miss dependency relationships.

Anyway, I'll try to loop around to fixing this, just wanted to gather
some interest in the more general set of fixes I've got. I've got some
fixes queued up after that that fix/improve the *.sp and *.s parts of
the line you quoted (and other similar lines). I'd prefer just to fix
this along with those so the two don't textually conflict.

1. https://lore.kernel.org/git/[e-mail]/

Mike Hommey Nov. 15, 2021, 11 p.m. UTC | #13

On Mon, Nov 15, 2021 at 11:49:31PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Nov 16 2021, Mike Hommey wrote:
> 
> > On Mon, Nov 15, 2021 at 11:26:36PM +0100, Ævar Arnfjörð Bjarmason wrote:
> >> 
> >> On Tue, Nov 16 2021, Mike Hommey wrote:
> >> 
> >> > On Thu, Sep 23, 2021 at 12:30:03PM +0200, Ævar Arnfjörð Bjarmason wrote:
> >> >> -builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX
> >> >> +hook.sp hook.s hook.o: hook-list.h
> >> >> +
> >> >> +builtin/help.sp builtin/help.s builtin/help.o: config-list.h hook-list.h GIT-PREFIX
> >> >
> >> > hook-list.h is only included from buitin/bugreport.c, so
> >> > builtin/bugreport.o should be the one with the hook-list.h dependency,
> >> > shouldn't it?
> >> 
> >> Well spotted, yes. This is a mistake. I think from some earlier WIP
> >> version of the series.
> >> 
> >> In practice we don't really miss dependencies due to these sorts of
> >> mistakes since we use the .depends files, i.e. GCC & Clang figure this
> >> out for us:
> >> 
> >>     $ grep hook-list .depend/* */.depend/*
> >>     builtin/.depend/bugreport.o.d: compat/compiler.h git-compat-util.h hook.h hook-list.h
> >>     builtin/.depend/bugreport.o.d:hook-list.h:
> >
> > But aren't those .depends files are only created when compiling object
> > files, such that builtin/.depend/bugreport.o.d wouldn't exist until
> > bugreport.c is compiled, which would fail if hook-list.h wasn't created
> > before that?
> 
> Fail how?
> 
> I don't think it could fail, because the purpose of these dependency
> relationships is to avoid needless *re*builds. So if you're building for
> the first time it doesn't matter, your compiler will find the relevant
> things to include for you. It doesn't need what's in the Makefile to do
> that.
> 
> See [1], what I said about LIB_H there applies more generally for the
> .depends files.
> 
> It will only fail in the sense that it over-depends, i.e. if you do:
> 
>     git clean -dxf; make builtin/help.o

Try

    git clean -dxf; make builtin/bugreport.o

It fails with:

      CC builtin/bugreport.o
  builtin/bugreport.c:7:10: fatal error: hook-list.h: そのようなファイルやディレクトリはありません
      7 | #include "hook-list.h"
        |          ^~~~~~~~~~~~~
  compilation terminated.
  make: *** [Makefile:2500: builtin/bugreport.o] エラー 1

The only reason I can see why it builds at all normally is that hook.o
is built soon enough that by the time builtin/bugreport.o is built
hook-list.h has already been generated.

Mike

Ævar Arnfjörð Bjarmason Nov. 16, 2021, 12:01 p.m. UTC | #14

On Tue, Nov 16 2021, Mike Hommey wrote:

> On Mon, Nov 15, 2021 at 11:49:31PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> 
>> On Tue, Nov 16 2021, Mike Hommey wrote:
>> 
>> > On Mon, Nov 15, 2021 at 11:26:36PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> >> 
>> >> On Tue, Nov 16 2021, Mike Hommey wrote:
>> >> 
>> >> > On Thu, Sep 23, 2021 at 12:30:03PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> >> >> -builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX
>> >> >> +hook.sp hook.s hook.o: hook-list.h
>> >> >> +
>> >> >> +builtin/help.sp builtin/help.s builtin/help.o: config-list.h hook-list.h GIT-PREFIX
>> >> >
>> >> > hook-list.h is only included from buitin/bugreport.c, so
>> >> > builtin/bugreport.o should be the one with the hook-list.h dependency,
>> >> > shouldn't it?
>> >> 
>> >> Well spotted, yes. This is a mistake. I think from some earlier WIP
>> >> version of the series.
>> >> 
>> >> In practice we don't really miss dependencies due to these sorts of
>> >> mistakes since we use the .depends files, i.e. GCC & Clang figure this
>> >> out for us:
>> >> 
>> >>     $ grep hook-list .depend/* */.depend/*
>> >>     builtin/.depend/bugreport.o.d: compat/compiler.h git-compat-util.h hook.h hook-list.h
>> >>     builtin/.depend/bugreport.o.d:hook-list.h:
>> >
>> > But aren't those .depends files are only created when compiling object
>> > files, such that builtin/.depend/bugreport.o.d wouldn't exist until
>> > bugreport.c is compiled, which would fail if hook-list.h wasn't created
>> > before that?
>> 
>> Fail how?
>> 
>> I don't think it could fail, because the purpose of these dependency
>> relationships is to avoid needless *re*builds. So if you're building for
>> the first time it doesn't matter, your compiler will find the relevant
>> things to include for you. It doesn't need what's in the Makefile to do
>> that.
>> 
>> See [1], what I said about LIB_H there applies more generally for the
>> .depends files.
>> 
>> It will only fail in the sense that it over-depends, i.e. if you do:
>> 
>>     git clean -dxf; make builtin/help.o
>
> Try
>
>     git clean -dxf; make builtin/bugreport.o
>
> It fails with:
>
>       CC builtin/bugreport.o
>   builtin/bugreport.c:7:10: fatal error: hook-list.h: そのようなファイルやディレクトリはありません
>       7 | #include "hook-list.h"
>         |          ^~~~~~~~~~~~~
>   compilation terminated.
>   make: *** [Makefile:2500: builtin/bugreport.o] エラー 1
>
> The only reason I can see why it builds at all normally is that hook.o
> is built soon enough that by the time builtin/bugreport.o is built
> hook-list.h has already been generated.

Ah, you're obviously right. I don't know what I was thinking yesterday.

I submitted a re-roll of the greater dependency fix-up & optimization
series I've got kicking around, which includes a fix for this
issue. Thank you for the report:
https://lore.kernel.org/git/[e-mail]

Junio C Hamano Nov. 17, 2021, 8:39 a.m. UTC | #15

Ævar Arnfjörð Bjarmason <[e-mail]> writes:

> I submitted a re-roll of the greater dependency fix-up & optimization
> series I've got kicking around, which includes a fix for this
> issue. Thank you for the report:

Makes me wonder what other breakages are introduced in such a big
series, in exchange for correcting this single issue X-<.  We'll see
soon enough, I guess ;-)

Patch

diff --git a/.gitignore b/.gitignore
index 311841f9bed..6be9de41ae8 100644
--- a/.gitignore
+++ b/.gitignore
@@ -190,6 +190,7 @@ 
 /gitweb/static/gitweb.min.*
 /config-list.h
 /command-list.h
+/hook-list.h
 *.tar.gz
 *.dsc
 *.deb
diff --git a/Makefile b/Makefile
index ad2aeff68f0..f883657fa26 100644
--- a/Makefile
+++ b/Makefile
@@ -817,6 +817,8 @@  XDIFF_LIB = xdiff/lib.a
 
 GENERATED_H += command-list.h
 GENERATED_H += config-list.h
+GENERATED_H += hook-list.h
+
 .PHONY: generated-hdrs
 generated-hdrs: $(GENERATED_H)
 
@@ -2216,7 +2218,9 @@  git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 
 help.sp help.s help.o: command-list.h
 
-builtin/help.sp builtin/help.s builtin/help.o: config-list.h GIT-PREFIX
+hook.sp hook.s hook.o: hook-list.h
+
+builtin/help.sp builtin/help.s builtin/help.o: config-list.h hook-list.h GIT-PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
 	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
 	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
@@ -2248,6 +2252,9 @@  command-list.h: $(wildcard Documentation/git*.txt)
 		$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
 		command-list.txt >$@
 
+hook-list.h: generate-hooklist.sh Documentation/githooks.txt
+	$(QUIET_GEN)$(SHELL_PATH) ./generate-hooklist.sh >$@
+
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
 	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
 	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV):\
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index a02c2540bb1..9de32bc96e7 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -4,6 +4,7 @@ 
 #include "help.h"
 #include "compat/compiler.h"
 #include "hook.h"
+#include "hook-list.h"
 
 
 static void get_system_info(struct strbuf *sys_info)
@@ -41,39 +42,7 @@  static void get_system_info(struct strbuf *sys_info)
 
 static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 {
-	/*
-	 * NEEDSWORK: Doesn't look like there is a list of all possible hooks;
-	 * so below is a transcription of `git help hooks`. Later, this should
-	 * be replaced with some programmatically generated list (generated from
-	 * doc or else taken from some library which tells us about all the
-	 * hooks)
-	 */
-	static const char *hook[] = {
-		"applypatch-msg",
-		"pre-applypatch",
-		"post-applypatch",
-		"pre-commit",
-		"pre-merge-commit",
-		"prepare-commit-msg",
-		"commit-msg",
-		"post-commit",
-		"pre-rebase",
-		"post-checkout",
-		"post-merge",
-		"pre-push",
-		"pre-receive",
-		"update",
-		"post-receive",
-		"post-update",
-		"push-to-checkout",
-		"pre-auto-gc",
-		"post-rewrite",
-		"sendemail-validate",
-		"fsmonitor-watchman",
-		"p4-pre-submit",
-		"post-index-change",
-	};
-	int i;
+	const char **p;
 
 	if (nongit) {
 		strbuf_addstr(hook_info,
@@ -81,9 +50,12 @@  static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 		return;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(hook); i++)
-		if (hook_exists(hook[i]))
-			strbuf_addf(hook_info, "%s\n", hook[i]);
+	for (p = hook_name_list; *p; p++) {
+		const char *hook = *p;
+
+		if (hook_exists(hook))
+			strbuf_addf(hook_info, "%s\n", hook);
+	}
 }
 
 static const char * const bugreport_usage[] = {
diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
index 171b4124afe..fd1399c440f 100644
--- a/contrib/buildsystems/CMakeLists.txt
+++ b/contrib/buildsystems/CMakeLists.txt
@@ -624,6 +624,13 @@  if(NOT EXISTS ${CMAKE_BINARY_DIR}/config-list.h)
 			OUTPUT_FILE ${CMAKE_BINARY_DIR}/config-list.h)
 endif()
 
+if(NOT EXISTS ${CMAKE_BINARY_DIR}/hook-list.h)
+	message("Generating hook-list.h")
+	execute_process(COMMAND ${SH_EXE} ${CMAKE_SOURCE_DIR}/generate-hooklist.sh
+			WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}
+			OUTPUT_FILE ${CMAKE_BINARY_DIR}/hook-list.h)
+endif()
+
 include_directories(${CMAKE_BINARY_DIR})
 
 #build
diff --git a/generate-hooklist.sh b/generate-hooklist.sh
new file mode 100755
index 00000000000..6d4e56d1a31
--- /dev/null
+++ b/generate-hooklist.sh
@@ -0,0 +1,18 @@ 
+#!/bin/sh
+#
+# Usage: ./generate-hooklist.sh >hook-list.h
+
+cat <<EOF
+/* Automatically generated by generate-hooklist.sh */
+
+static const char *hook_name_list[] = {
+EOF
+
+sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
+	<Documentation/githooks.txt |
+	LC_ALL=C sort
+
+cat <<EOF
+	NULL,
+};
+EOF