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

On Mon, Sep 27 2021, Phillip Wood wrote:

On 26/09/2021 20:03, Ævar Arnfjörð Bjarmason wrote:

quoted

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/ (local);
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 (local)
2. https://lore.kernel.org/git/87fstt3gzd.fsf@evledraar.gmail.com/ (local)
[...]
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.

quoted

       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.