Re: [PATCH v2 0/8] Makefile: generate a hook-list.h, prep for config-based-hooks

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Sep 27 2021, Phillip Wood wrote:

> On 26/09/2021 20:03, Ævar Arnfjörð Bjarmason wrote:
>> This series is an incremental restart of the now-ejected
>> es/config-based-hooks and ab/config-based-hooks-base topics. See [1]
>> for a summary of the plan and progression.
>> In v2 the "sed" invocation that generates the new hook-list.h has
>> been
>> changed to be portable under POSIX. See the thread starting at
>> https://lore.kernel.org/git/92471ff9-7573-c3e4-e9fd-63a5cbf5738f@gmail.com/;
>> The portability issue is AFAICT theoretical in that any "sed"
>> command
>> I've tried accepts the old version (I tried the large list of OS's
>> listed in [2]), but better safe than sorry.
>> Other changes:
>>   * I noticed that the run-command.h inclusion in transport.c become
>>     redundant, I removed that and validated the other ones that have
>>     the new hook.h, they all still need run-command.h.
>>   * A whitespace change in v1 in a change to the Makefile makes the
>>     diff for 8/8 easier to read.
>> 1. http://lore.kernel.org/git/cover-0.8-00000000000-20210923T095326Z-avarab@gmail.com
>> 2. https://lore.kernel.org/git/87fstt3gzd.fsf@evledraar.gmail.com/
>> [...]
>> 8:  80aae4d5c13 ! 8:  7420267ce09 hook-list.h: add a generated list of hooks, like config-list.h
>>      @@ Makefile: XDIFF_LIB = xdiff/lib.a
>>        generated-hdrs: $(GENERATED_H)
>>              @@ Makefile: git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS)
>> $(GITLIBS)
>>      + 		$(filter %.o,$^) $(LIBS)
>>               help.sp help.s help.o: command-list.h
>>      ++hook.sp hook.s hook.o: hook-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
>
> This is billed as a whitespace change above but this line has actually
> changed since the last version - was that intentional?

I think you're mistaken here, it is a whitespace-only change to the
end-state, but the diff and range-diff are confusing. If I diff the two
Makefiles I end up with when applying v1 and v2 I get:

@@ -2217,7 +2210,6 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 		$(filter %.o,$^) $(LIBS)
 
 help.sp help.s help.o: command-list.h
-
 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

I.e. only the line between the old command-list.h and new hook-list.h
line is gone.

But the diff for v1 is D/A/A/A and for v2 A/D/A (D = Deletion, A =
Addition).

I.e. it's one of those times when "git diff" produces a valid diff, and
one that's actually smaller than couldu have been produced with a
v2-like diff given the change in v1.

As an aside I've sometimes wished we had a --diff-algorithm=maximal or
something, i.e. there's a lot of cases where by just adding one line to
the diff you can produce a bigger but IMO less confusing one.

In any case, the end state looks better & the diff for v2 is more
intuitive to look at.

>>        builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
>>        	'-DGIT_HTML_PATH="$(htmldir_relative_SQ)"' \
>>      @@ generate-hooklist.sh (new)
>>       +static const char *hook_name_list[] = {
>>       +EOF
>>       +
>>      -+sed -n -e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}; x' \
>>      ++sed -n \
>>      ++	-e '/^~~~~*$/ {x; s/^.*$/	"&",/; p;}' \
>>      ++	-e 'x' \
>>       +	<Documentation/githooks.txt |
>>       +	LC_ALL=C sort
>>       +
>
> The sed change looks good

Thanks for confirming.