Re: [PATCH v2 0/8] Makefile: generate a hook-list.h, prep for config-based-hooks
From: Phillip Wood <hidden>
Date: 2021-09-27 18:02:04
On 27/09/2021 11:38, Ævar Arnfjörð Bjarmason wrote:
On Mon, Sep 27 2021, Phillip Wood wrote:quoted
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-PREFIXThis 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,
Yes you're right, looking more closely it is a context line in the range diff that has changed, sorry.
quoted hunk ↗ jump to hunk
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-PREFIXI.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.
Yes especially when it splits the diff just to keep an empty line or closing brace as context even though the line itself is not really interesting. Python's diff-lib has an option to ignore "junk" when determining which lines are unchanged, I keep meaning to see how it affects the diff but have never got round to it (the library uses a different slower algorithm for calculating the diff as well). Best Wishes Phillip
In any case, the end state looks better & the diff for v2 is more intuitive to look at.quoted
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 goodThanks for confirming.