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

[Phillip Wood <phillip.wood123@gmail.com>]

On 27/09/2021 11:38, Ævar Arnfjörð Bjarmason wrote:
> 
> 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,

Yes you're right, looking more closely it is a context line in the range 
diff that has changed, sorry.

> 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.

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.
> 
>>>         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.
>