| Message ID | cover-v2-0.8-00000000000-20210926T185800Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Makefile: generate a hook-list.h, prep for config-based-hooks | expand |
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? > 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 Phillip
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.
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. >
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/ Emily Shaffer (1): hook.c: add a hook_exists() wrapper and use it in bugreport.c Ævar Arnfjörð Bjarmason (7): Makefile: mark "check" target as .PHONY Makefile: stop hardcoding {command,config}-list.h Makefile: don't perform "mv $@+ $@" dance for $(GENERATED_H) Makefile: remove an out-of-date comment hook.[ch]: move find_hook() from run-command.c to hook.c hook.c users: use "hook_exists()" instead of "find_hook()" hook-list.h: add a generated list of hooks, like config-list.h .gitignore | 1 + Makefile | 28 ++++++++++-------- builtin/am.c | 1 + builtin/bugreport.c | 46 ++++++----------------------- builtin/commit.c | 3 +- builtin/merge.c | 3 +- builtin/receive-pack.c | 3 +- builtin/worktree.c | 1 + compat/vcbuild/README | 2 +- config.mak.uname | 6 ++-- contrib/buildsystems/CMakeLists.txt | 7 +++++ generate-hooklist.sh | 20 +++++++++++++ hook.c | 42 ++++++++++++++++++++++++++ hook.h | 16 ++++++++++ refs.c | 1 + run-command.c | 35 +--------------------- run-command.h | 7 ----- sequencer.c | 3 +- transport.c | 2 +- 19 files changed, 127 insertions(+), 100 deletions(-) create mode 100755 generate-hooklist.sh create mode 100644 hook.c create mode 100644 hook.h Range-diff against v1: 1: 91d0cd09c8e = 1: 095ef66c29e Makefile: mark "check" target as .PHONY 2: 804795771c6 = 2: 520e4baede6 Makefile: stop hardcoding {command,config}-list.h 3: 010701fd784 = 3: 312b353c651 Makefile: don't perform "mv $@+ $@" dance for $(GENERATED_H) 4: b3cce74d259 = 4: aea4868f988 Makefile: remove an out-of-date comment 5: 7dd874d50ec ! 5: 64f3af94161 hook.[ch]: move find_hook() from run-command.c to hook.c @@ sequencer.c ## transport.c ## @@ + #include "cache.h" #include "config.h" #include "transport.h" - #include "run-command.h" +-#include "run-command.h" +#include "hook.h" #include "pkt-line.h" #include "fetch-pack.h" 6: db8893afee6 = 6: 4eabe7ca39f hook.c: add a hook_exists() wrapper and use it in bugreport.c 7: b61130dee5b = 7: 6237a1a5549 hook.c users: use "hook_exists()" instead of "find_hook()" 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 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 +