This series is an incremental restart of the es/config-based-hooks and
ab/config-based-hooks-base topics, which should both be ejected in
favor of this.
Those topics have been lingering for a long time. The v5 of
ab/config-based-hooks-base[1] was re-rolled on September 2nd, and then
on September 9th I re-rolled a v4 of Emily's on-top topic[2] to solve
conflicts with the "base" one as other things in "seen" were colliding
with its v4 + Emily's older version.
But in the end the sum total of master..es/config-based-hooks is 43
commits and a rather daunting "44 files changed, 1870 insertions(+),
611 deletions(-)" diffstat. I think it's fair to say that anything
approaching a full picture of these patches only exists in our
respective wetwares, and wider reviewer attention is clearly needed.
That much larger topic also has a conflict Junio's been solving, and
by its presence has surely been holding up the progression of various
other submitted and unsubmitted topics.
So this is step #1 in an (at least) 5 step plan outlined in [3] (start
reading at "It seems due to") to get this greater series into git in a
more piecemeal fashion.
Here we're only setting up some build system changes needed for the
eventual master..ab/config-based-hooks-base topic, the range-diff is
against the relevant part of[1]. These changes are able to stand on
their own.
The only caveat to that is that we end up with a hook.[ch] with just
two functions, which is usually not the point at which we split out
new headers. As noted in the commit messages we expect those headers
to get much larger with the rest of ab/config-based-hooks-base.
I intentionally split this off from the much larger thread starting at
[4] and pruned the CC list. If anyone feels left out or is interested
a review of this is a most welcome way to be re-added :)
Changes since [1]:
* Resolved a conflict with advice.[ch]-related changes
* A new commit to skip the "mv $@+ $@" dance for
$(GENERATED_H). These patches originally pre-dated our use of
".DELETE_ON_ERROR" in the Makefile.
* There was a "while we're at it" change to a comment in
run-command.h as it was being moved, now this is a purely move-only
change. I'll either re-incorporate that minor change in re-rolls of
ab/config-based-hooks-base, or just drop it.
1. https://lore.kernel.org/git/cover-v5-00.36-00000000000-20210902T125110Z-avarab@gmail.com/#t
2. https://lore.kernel.org/git/cover-v4-0.5-00000000000-20210909T122802Z-avarab@gmail.com/
3. https://lore.kernel.org/git/875yut8nns.fsf@evledraar.gmail.com/
4. https://lore.kernel.org/git/20210311021037.3001235-1-emilyshaffer@google.com/#t
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 | 29 ++++++++++--------
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 | 18 +++++++++++
hook.c | 42 ++++++++++++++++++++++++++
hook.h | 16 ++++++++++
refs.c | 1 +
run-command.c | 35 +---------------------
run-command.h | 7 -----
sequencer.c | 3 +-
transport.c | 1 +
19 files changed, 126 insertions(+), 99 deletions(-)
create mode 100755 generate-hooklist.sh
create mode 100644 hook.c
create mode 100644 hook.h
Range-diff:
1: ac419613fdc = 1: 91d0cd09c8e Makefile: mark "check" target as .PHONY
2: a161b7f0a5c = 2: 804795771c6 Makefile: stop hardcoding {command,config}-list.h
-: ----------- > 3: 010701fd784 Makefile: don't perform "mv $@+ $@" dance for $(GENERATED_H)
3: ffef1d3257e = 4: b3cce74d259 Makefile: remove an out-of-date comment
4: 545e16c6f04 ! 5: 7dd874d50ec hook.[ch]: move find_hook() from run-command.c to hook.c
@@ hook.c (new)
+ err = errno;
+#endif
+
-+ if (err == EACCES && advice_ignored_hook) {
++ if (err == EACCES && advice_enabled(ADVICE_IGNORED_HOOK)) {
+ static struct string_list advise_given = STRING_LIST_INIT_DUP;
+
+ if (!string_list_lookup(&advise_given, name)) {
@@ hook.h (new)
+#ifndef HOOK_H
+#define HOOK_H
+
-+/**
++/*
+ * Returns the path to the hook file, or NULL if the hook is missing
+ * or disabled. Note that this points to static storage that will be
-+ * overwritten by further calls to find_hook().
++ * overwritten by further calls to find_hook and run_hook_*.
+ */
+const char *find_hook(const char *name);
+
@@ refs.c
## run-command.c ##
@@
- #include "string-list.h"
#include "quote.h"
#include "config.h"
+ #include "packfile.h"
+#include "hook.h"
void child_process_init(struct child_process *child)
@@ run-command.c: int async_with_fork(void)
- err = errno;
-#endif
-
-- if (err == EACCES && advice_ignored_hook) {
+- if (err == EACCES && advice_enabled(ADVICE_IGNORED_HOOK)) {
- static struct string_list advise_given = STRING_LIST_INIT_DUP;
-
- if (!string_list_lookup(&advise_given, name)) {
5: a9bc4519e9a = 6: db8893afee6 hook.c: add a hook_exists() wrapper and use it in bugreport.c
6: e99ec2e6f8f = 7: b61130dee5b hook.c users: use "hook_exists()" instead of "find_hook()"
7: 2ffb2332c8a ! 8: 80aae4d5c13 hook-list.h: add a generated list of hooks, like config-list.h
@@ Makefile: git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
@@ Makefile: command-list.h: $(wildcard Documentation/git*.txt)
$(patsubst %,--exclude-program %,$(EXCLUDED_PROGRAMS)) \
- command-list.txt >$@+ && mv $@+ $@
+ command-list.txt >$@
+hook-list.h: generate-hooklist.sh Documentation/githooks.txt
-+ $(QUIET_GEN)$(SHELL_PATH) ./generate-hooklist.sh \
-+ >$@+ && mv $@+ $@
++ $(QUIET_GEN)$(SHELL_PATH) ./generate-hooklist.sh >$@
+
SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
--
2.33.0.1229.g0a86d28df49
Fix a bug in 44c9e8594e (Fix up header file dependencies and add
sparse checking rules, 2005-07-03), we never marked the phony "check"
target as such.
Perhaps we should just remove it, since as of a combination of
912f9980d2 (Makefile: help people who run 'make check' by mistake,
2008-11-11) 0bcd9ae85d (sparse: Fix errors due to missing
target-specific variables, 2011-04-21) we've been suggesting the user
run "make sparse" directly.
But under that mode it still does something, as well as directing the
user to run "make test" under non-sparse. So let's punt that and
narrowly fix the PHONY bug.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 1 +
1 file changed, 1 insertion(+)
Change various places that hardcode the names of these two files to
refer to either $(GENERATED_H), or to a new generated-hdrs
target. That target is consistent with the *-objs targets I recently
added in 029bac01a8 (Makefile: add {program,xdiff,test,git,fuzz}-objs
& objects targets, 2021-02-23).
A subsequent commit will add a new generated hook-list.h. By doing
this refactoring we'll only need to add the new file to the
GENERATED_H variable, not EXCEPT_HDRS, the vcbuild/README etc.
Hardcoding command-list.h there seems to have been a case of
copy/paste programming in 976aaedca0 (msvc: add a Makefile target to
pre-generate the Visual Studio solution, 2019-07-29). The
config-list.h was added later in 709df95b78 (help: move
list_config_help to builtin/help, 2020-04-16).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 6 ++++--
compat/vcbuild/README | 2 +-
config.mak.uname | 6 +++---
3 files changed, 8 insertions(+), 6 deletions(-)
@@ -92,7 +92,7 @@ The Steps of Build Git with VS2008 the git operations. 3. Inside Git's directory run the command:- make command-list.h config-list.h+ make generated-hdrs to generate the header file needed to compile git. 4. Then either build Git with the GNU Make Makefile in the Git projects
Change the "cmd.sh > $@+ && mv $@+ $@" pattern used for generating the
config-list.h and command-list.h to just "cmd.sh >$@". This was needed
as a guard to ensure that we don't have an empty file if the script
failed, but since 7b76d6bf221 (Makefile: add and use the
".DELETE_ON_ERROR" flag, 2021-06-29) GNU make ensures that doesn't
happen.
There's still a lot of other places in the Makefile where we
needlessly use this pattern, but I'm just changing these because I'm
about to add a new $(GENERATED_H) target, let's have them all look and
act the same way.
Even with ".DELETE_ON_ERROR" there is still a point to using the "mv
$@+ $@" pattern in some cases, e.g. to ensure that you have a working
binary during recompilation (see [1] for the start of a long
discussion about that), but that doesn't apply here. Nothing external
uses $(GENERATED_H) directly, it's only ever used in the context of
the Makefile's own dependency (re-)generation.
1. https://lore.kernel.org/git/8735t93h0u.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
This comment added in dfea575017 (Makefile: lazily compute header
dependencies, 2010-01-26) has been out of date since
92b88eba9f (Makefile: use `git ls-files` to list header files, if
possible, 2019-03-04), when we did exactly what it tells us not to do
and added $(GENERATED_H) to $(OBJECTS) dependencies.
The rest of it was also somewhere between inaccurate and outdated,
since as of b8ba629264 (Makefile: fold MISC_H into LIB_H, 2012-06-20)
it's not followed by a list of header files, that got moved earlier in
the file into LIB_H in 60d24dd255 (Makefile: fold XDIFF_H and VCSSVN_H
into LIB_H, 2012-07-06).
Let's just remove it entirely, to the extent that we have anything
useful to say here the comment on the
"USE_COMPUTED_HEADER_DEPENDENCIES" variable a few lines above this
change does the job for us.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 7 -------
1 file changed, 7 deletions(-)
@@ -2508,13 +2508,6 @@ ifneq ($(dep_files_present),)include $(dep_files_present)endifelse-# Dependencies on header files, for platforms that do not support-# the gcc -MMD option.-#-# Dependencies on automatically generated headers such as command-list.h-# should _not_ be included here, since they are necessary even when-# building an object for the first time.-$(OBJECTS):$(LIB_H)$(GENERATED_H)endif
Move the find_hook() function from run-command.c to a new hook.c
library. This change establishes a stub library that's pretty
pointless right now, but will see much wider use with Emily Shaffer's
upcoming "configuration-based hooks" series.
Eventually all the hook related code will live in hook.[ch]. Let's
start that process by moving the simple find_hook() function over
as-is.
Signed-off-by: Emily Shaffer <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 1 +
builtin/am.c | 1 +
builtin/bugreport.c | 2 +-
builtin/commit.c | 1 +
builtin/merge.c | 1 +
builtin/receive-pack.c | 1 +
builtin/worktree.c | 1 +
hook.c | 37 +++++++++++++++++++++++++++++++++++++
hook.h | 11 +++++++++++
refs.c | 1 +
run-command.c | 35 +----------------------------------
run-command.h | 7 -------
sequencer.c | 1 +
transport.c | 1 +
14 files changed, 59 insertions(+), 42 deletions(-)
create mode 100644 hook.c
create mode 100644 hook.h
@@ -0,0 +1,37 @@+#include"cache.h"+#include"hook.h"+#include"run-command.h"++constchar*find_hook(constchar*name)+{+staticstructstrbufpath=STRBUF_INIT;++strbuf_reset(&path);+strbuf_git_path(&path,"hooks/%s",name);+if(access(path.buf,X_OK)<0){+interr=errno;++#ifdef STRIP_EXTENSION+strbuf_addstr(&path,STRIP_EXTENSION);+if(access(path.buf,X_OK)>=0)+returnpath.buf;+if(errno==EACCES)+err=errno;+#endif++if(err==EACCES&&advice_enabled(ADVICE_IGNORED_HOOK)){+staticstructstring_listadvise_given=STRING_LIST_INIT_DUP;++if(!string_list_lookup(&advise_given,name)){+string_list_insert(&advise_given,name);+advise(_("The '%s' hook was ignored because "+"it's not set as executable.\n"+"You can disable this warning with "+"`git config advice.ignoredHook false`."),+path.buf);+}+}+returnNULL;+}+returnpath.buf;+}
@@ -1322,40 +1323,6 @@ int async_with_fork(void)#endif}-constchar*find_hook(constchar*name)-{-staticstructstrbufpath=STRBUF_INIT;--strbuf_reset(&path);-strbuf_git_path(&path,"hooks/%s",name);-if(access(path.buf,X_OK)<0){-interr=errno;--#ifdef STRIP_EXTENSION-strbuf_addstr(&path,STRIP_EXTENSION);-if(access(path.buf,X_OK)>=0)-returnpath.buf;-if(errno==EACCES)-err=errno;-#endif--if(err==EACCES&&advice_enabled(ADVICE_IGNORED_HOOK)){-staticstructstring_listadvise_given=STRING_LIST_INIT_DUP;--if(!string_list_lookup(&advise_given,name)){-string_list_insert(&advise_given,name);-advise(_("The '%s' hook was ignored because "-"it's not set as executable.\n"-"You can disable this warning with "-"`git config advice.ignoredHook false`."),-path.buf);-}-}-returnNULL;-}-returnpath.buf;-}-intrun_hook_ve(constchar*const*env,constchar*name,va_listargs){structchild_processhook=CHILD_PROCESS_INIT;
From: Emily Shaffer <redacted>
Add a boolean version of the find_hook() function for those callers
who are only interested in checking whether the hook exists, not what
the path to it is.
Signed-off-by: Emily Shaffer <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/bugreport.c | 2 +-
hook.c | 5 +++++
hook.h | 5 +++++
3 files changed, 11 insertions(+), 1 deletion(-)
Use the new hook_exists() function instead of find_hook() where the
latter was called in boolean contexts. This make subsequent changes in
a series where we further refactor the hook API clearer, as we won't
conflate wanting to get the path of the hook with checking for its
existence.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/commit.c | 2 +-
builtin/merge.c | 2 +-
builtin/receive-pack.c | 2 +-
sequencer.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
Make githooks(5) the source of truth for what hooks git supports, and
punt out early on hooks we don't know about in find_hook(). This
ensures that the documentation and the C code's idea about existing
hooks doesn't diverge.
We still have Perl and Python code running its own hooks, but that'll
be addressed by Emily Shaffer's upcoming "git hook run" command.
This resolves a long-standing TODO item in bugreport.c of there being
no centralized listing of hooks, and fixes a bug with the bugreport
listing only knowing about 1/4 of the p4 hooks. It didn't know about
the recent "reference-transaction" hook either.
We could make the find_hook() function die() or BUG() out if the new
known_hook() returned 0, but let's make it return NULL just as it does
when it can't find a hook of a known type. Making it die() is overly
anal, and unlikely to be what we need in catching stupid typos in the
name of some new hook hardcoded in git.git's sources. By making this
be tolerant of unknown hook names, changes in a later series to make
"git hook run" run arbitrary user-configured hook names will be easier
to implement.
I have not been able to directly test the CMake change being made
here. Since 4c2c38e800 (ci: modification of main.yml to use cmake for
vs-build job, 2020-06-26) some of the Windows CI has a hard dependency
on CMake, this change works there, and is to my eyes an obviously
correct use of a pattern established in previous CMake changes,
namely:
- 061c2240b1 (Introduce CMake support for configuring Git,
2020-06-12)
- 709df95b78 (help: move list_config_help to builtin/help,
2020-04-16)
- 976aaedca0 (msvc: add a Makefile target to pre-generate the Visual
Studio solution, 2019-07-29)
The LC_ALL=C is needed because at least in my locale the dash ("-") is
ignored for the purposes of sorting, which results in a different
order. I'm not aware of anything in git that has a hard dependency on
the order, but e.g. the bugreport output would end up using whatever
locale was in effect when git was compiled.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Helped-by: René Scharfe [off-list ref]
---
.gitignore | 1 +
Makefile | 9 +++++-
builtin/bugreport.c | 44 ++++++-----------------------
contrib/buildsystems/CMakeLists.txt | 7 +++++
generate-hooklist.sh | 18 ++++++++++++
5 files changed, 42 insertions(+), 37 deletions(-)
create mode 100755 generate-hooklist.sh
POSIX does not support using a semicolon after a closing brace [1],
grepping our code base with
git grep 'sed .*};' '*.sh'
does not give any matches so I don't think we're using that pattern any
where else. Replacing the semicolon with ' -e' would fix it.
Best Wishes
Phillip
[1] "Editing commands other that {...}, a, b, c, i, r, t, w, : and # can
be followed by a <semicolon>" from
https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html
POSIX does not support using a semicolon after a closing brace [1],
grepping our code base with
git grep 'sed .*};' '*.sh'
does not give any matches so I don't think we're using that pattern
any where else. Replacing the semicolon with ' -e' would fix it.
Best Wishes
Phillip
Does this fail on any system you're aware of? If so what OS/version (and
preferably version of "sed").
René's downthread [off-list ref] seems to
suggest that this is fine.
Both beforehand and just now I've tested this on AIX, Solaris,
{Open,Net,Free}BSD, HP/UX, OSX and Linux (a few distros/versions).
All of them are able to generate the same hook-list.h using this version
of the patch.
POSIX does not support using a semicolon after a closing brace [1],
grepping our code base with
git grep 'sed .*};' '*.sh'
does not give any matches so I don't think we're using that pattern
any where else. Replacing the semicolon with ' -e' would fix it.
Best Wishes
Phillip
Does this fail on any system you're aware of? If so what OS/version (and
preferably version of "sed").
I wasn't aware of that restriction and my gut feeling is that enforcing
it in a sed implementation would be slightly harder than allowing "};".
René's downthread [off-list ref] seems to
suggest that this is fine.
That said, I didn't mean to suggest that we keep using this construct,
just that I couldn't find an implementation which would reject it.
Both beforehand and just now I've tested this on AIX, Solaris,
{Open,Net,Free}BSD, HP/UX, OSX and Linux (a few distros/versions).
All of them are able to generate the same hook-list.h using this version
of the patch.
Impressive list, but still it's probably better to move the last "x" to
its own -e, to appease the POSIX spirits. Small change..
René
POSIX does not support using a semicolon after a closing brace [1],
grepping our code base with
git grep 'sed .*};' '*.sh'
does not give any matches so I don't think we're using that pattern
any where else. Replacing the semicolon with ' -e' would fix it.
Best Wishes
Phillip
Does this fail on any system you're aware of? If so what OS/version (and
preferably version of "sed").
I wasn't aware of that restriction and my gut feeling is that enforcing
it in a sed implementation would be slightly harder than allowing "};".
quoted
René's downthread [off-list ref] seems to
suggest that this is fine.
That said, I didn't mean to suggest that we keep using this construct,
just that I couldn't find an implementation which would reject it.
I'll re-test without "};", my (seemingly incorrect) quick read of your
E-Mail + my own testing was that semicolons were fine there, but maybe
they're just accepted by most...
quoted
Both beforehand and just now I've tested this on AIX, Solaris,
{Open,Net,Free}BSD, HP/UX, OSX and Linux (a few distros/versions).
All of them are able to generate the same hook-list.h using this version
of the patch.
Impressive list.
FWIW that's just the GCC Farm which is fairly easy to get access to for
free software devlopment: https://cfarm.tetaneutral.net/machines/list/
The HP/UX box is a similar "private" setup mainly for Perl5 core
development, I'm getting to use it to get git working there.
but still it's probably better to move the last "x" to its own -e, to
appease the POSIX spirits. Small change..
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
+
--
2.33.0.1291.g8857a6a91ac
Fix a bug in 44c9e8594e (Fix up header file dependencies and add
sparse checking rules, 2005-07-03), we never marked the phony "check"
target as such.
Perhaps we should just remove it, since as of a combination of
912f9980d2 (Makefile: help people who run 'make check' by mistake,
2008-11-11) 0bcd9ae85d (sparse: Fix errors due to missing
target-specific variables, 2011-04-21) we've been suggesting the user
run "make sparse" directly.
But under that mode it still does something, as well as directing the
user to run "make test" under non-sparse. So let's punt that and
narrowly fix the PHONY bug.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 1 +
1 file changed, 1 insertion(+)
Change various places that hardcode the names of these two files to
refer to either $(GENERATED_H), or to a new generated-hdrs
target. That target is consistent with the *-objs targets I recently
added in 029bac01a8 (Makefile: add {program,xdiff,test,git,fuzz}-objs
& objects targets, 2021-02-23).
A subsequent commit will add a new generated hook-list.h. By doing
this refactoring we'll only need to add the new file to the
GENERATED_H variable, not EXCEPT_HDRS, the vcbuild/README etc.
Hardcoding command-list.h there seems to have been a case of
copy/paste programming in 976aaedca0 (msvc: add a Makefile target to
pre-generate the Visual Studio solution, 2019-07-29). The
config-list.h was added later in 709df95b78 (help: move
list_config_help to builtin/help, 2020-04-16).
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 6 ++++--
compat/vcbuild/README | 2 +-
config.mak.uname | 6 +++---
3 files changed, 8 insertions(+), 6 deletions(-)
@@ -92,7 +92,7 @@ The Steps of Build Git with VS2008 the git operations. 3. Inside Git's directory run the command:- make command-list.h config-list.h+ make generated-hdrs to generate the header file needed to compile git. 4. Then either build Git with the GNU Make Makefile in the Git projects
Change the "cmd.sh > $@+ && mv $@+ $@" pattern used for generating the
config-list.h and command-list.h to just "cmd.sh >$@". This was needed
as a guard to ensure that we don't have an empty file if the script
failed, but since 7b76d6bf221 (Makefile: add and use the
".DELETE_ON_ERROR" flag, 2021-06-29) GNU make ensures that doesn't
happen.
There's still a lot of other places in the Makefile where we
needlessly use this pattern, but I'm just changing these because I'm
about to add a new $(GENERATED_H) target, let's have them all look and
act the same way.
Even with ".DELETE_ON_ERROR" there is still a point to using the "mv
$@+ $@" pattern in some cases, e.g. to ensure that you have a working
binary during recompilation (see [1] for the start of a long
discussion about that), but that doesn't apply here. Nothing external
uses $(GENERATED_H) directly, it's only ever used in the context of
the Makefile's own dependency (re-)generation.
1. https://lore.kernel.org/git/8735t93h0u.fsf@evledraar.gmail.com/
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
This comment added in dfea575017 (Makefile: lazily compute header
dependencies, 2010-01-26) has been out of date since
92b88eba9f (Makefile: use `git ls-files` to list header files, if
possible, 2019-03-04), when we did exactly what it tells us not to do
and added $(GENERATED_H) to $(OBJECTS) dependencies.
The rest of it was also somewhere between inaccurate and outdated,
since as of b8ba629264 (Makefile: fold MISC_H into LIB_H, 2012-06-20)
it's not followed by a list of header files, that got moved earlier in
the file into LIB_H in 60d24dd255 (Makefile: fold XDIFF_H and VCSSVN_H
into LIB_H, 2012-07-06).
Let's just remove it entirely, to the extent that we have anything
useful to say here the comment on the
"USE_COMPUTED_HEADER_DEPENDENCIES" variable a few lines above this
change does the job for us.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 7 -------
1 file changed, 7 deletions(-)
@@ -2501,13 +2501,6 @@ ifneq ($(dep_files_present),)include $(dep_files_present)endifelse-# Dependencies on header files, for platforms that do not support-# the gcc -MMD option.-#-# Dependencies on automatically generated headers such as command-list.h-# should _not_ be included here, since they are necessary even when-# building an object for the first time.-$(OBJECTS):$(LIB_H)$(GENERATED_H)endif
Move the find_hook() function from run-command.c to a new hook.c
library. This change establishes a stub library that's pretty
pointless right now, but will see much wider use with Emily Shaffer's
upcoming "configuration-based hooks" series.
Eventually all the hook related code will live in hook.[ch]. Let's
start that process by moving the simple find_hook() function over
as-is.
Signed-off-by: Emily Shaffer <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
Makefile | 1 +
builtin/am.c | 1 +
builtin/bugreport.c | 2 +-
builtin/commit.c | 1 +
builtin/merge.c | 1 +
builtin/receive-pack.c | 1 +
builtin/worktree.c | 1 +
hook.c | 37 +++++++++++++++++++++++++++++++++++++
hook.h | 11 +++++++++++
refs.c | 1 +
run-command.c | 35 +----------------------------------
run-command.h | 7 -------
sequencer.c | 1 +
transport.c | 2 +-
14 files changed, 59 insertions(+), 43 deletions(-)
create mode 100644 hook.c
create mode 100644 hook.h
@@ -0,0 +1,37 @@+#include"cache.h"+#include"hook.h"+#include"run-command.h"++constchar*find_hook(constchar*name)+{+staticstructstrbufpath=STRBUF_INIT;++strbuf_reset(&path);+strbuf_git_path(&path,"hooks/%s",name);+if(access(path.buf,X_OK)<0){+interr=errno;++#ifdef STRIP_EXTENSION+strbuf_addstr(&path,STRIP_EXTENSION);+if(access(path.buf,X_OK)>=0)+returnpath.buf;+if(errno==EACCES)+err=errno;+#endif++if(err==EACCES&&advice_enabled(ADVICE_IGNORED_HOOK)){+staticstructstring_listadvise_given=STRING_LIST_INIT_DUP;++if(!string_list_lookup(&advise_given,name)){+string_list_insert(&advise_given,name);+advise(_("The '%s' hook was ignored because "+"it's not set as executable.\n"+"You can disable this warning with "+"`git config advice.ignoredHook false`."),+path.buf);+}+}+returnNULL;+}+returnpath.buf;+}
@@ -1322,40 +1323,6 @@ int async_with_fork(void)#endif}-constchar*find_hook(constchar*name)-{-staticstructstrbufpath=STRBUF_INIT;--strbuf_reset(&path);-strbuf_git_path(&path,"hooks/%s",name);-if(access(path.buf,X_OK)<0){-interr=errno;--#ifdef STRIP_EXTENSION-strbuf_addstr(&path,STRIP_EXTENSION);-if(access(path.buf,X_OK)>=0)-returnpath.buf;-if(errno==EACCES)-err=errno;-#endif--if(err==EACCES&&advice_enabled(ADVICE_IGNORED_HOOK)){-staticstructstring_listadvise_given=STRING_LIST_INIT_DUP;--if(!string_list_lookup(&advise_given,name)){-string_list_insert(&advise_given,name);-advise(_("The '%s' hook was ignored because "-"it's not set as executable.\n"-"You can disable this warning with "-"`git config advice.ignoredHook false`."),-path.buf);-}-}-returnNULL;-}-returnpath.buf;-}-intrun_hook_ve(constchar*const*env,constchar*name,va_listargs){structchild_processhook=CHILD_PROCESS_INIT;
From: Emily Shaffer <redacted>
Add a boolean version of the find_hook() function for those callers
who are only interested in checking whether the hook exists, not what
the path to it is.
Signed-off-by: Emily Shaffer <redacted>
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/bugreport.c | 2 +-
hook.c | 5 +++++
hook.h | 5 +++++
3 files changed, 11 insertions(+), 1 deletion(-)
Use the new hook_exists() function instead of find_hook() where the
latter was called in boolean contexts. This make subsequent changes in
a series where we further refactor the hook API clearer, as we won't
conflate wanting to get the path of the hook with checking for its
existence.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
---
builtin/commit.c | 2 +-
builtin/merge.c | 2 +-
builtin/receive-pack.c | 2 +-
sequencer.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
Make githooks(5) the source of truth for what hooks git supports, and
punt out early on hooks we don't know about in find_hook(). This
ensures that the documentation and the C code's idea about existing
hooks doesn't diverge.
We still have Perl and Python code running its own hooks, but that'll
be addressed by Emily Shaffer's upcoming "git hook run" command.
This resolves a long-standing TODO item in bugreport.c of there being
no centralized listing of hooks, and fixes a bug with the bugreport
listing only knowing about 1/4 of the p4 hooks. It didn't know about
the recent "reference-transaction" hook either.
We could make the find_hook() function die() or BUG() out if the new
known_hook() returned 0, but let's make it return NULL just as it does
when it can't find a hook of a known type. Making it die() is overly
anal, and unlikely to be what we need in catching stupid typos in the
name of some new hook hardcoded in git.git's sources. By making this
be tolerant of unknown hook names, changes in a later series to make
"git hook run" run arbitrary user-configured hook names will be easier
to implement.
I have not been able to directly test the CMake change being made
here. Since 4c2c38e800 (ci: modification of main.yml to use cmake for
vs-build job, 2020-06-26) some of the Windows CI has a hard dependency
on CMake, this change works there, and is to my eyes an obviously
correct use of a pattern established in previous CMake changes,
namely:
- 061c2240b1 (Introduce CMake support for configuring Git,
2020-06-12)
- 709df95b78 (help: move list_config_help to builtin/help,
2020-04-16)
- 976aaedca0 (msvc: add a Makefile target to pre-generate the Visual
Studio solution, 2019-07-29)
The LC_ALL=C is needed because at least in my locale the dash ("-") is
ignored for the purposes of sorting, which results in a different
order. I'm not aware of anything in git that has a hard dependency on
the order, but e.g. the bugreport output would end up using whatever
locale was in effect when git was compiled.
Signed-off-by: Ævar Arnfjörð Bjarmason <redacted>
Helped-by: René Scharfe [off-list ref]
---
.gitignore | 1 +
Makefile | 8 +++++-
builtin/bugreport.c | 44 ++++++-----------------------
contrib/buildsystems/CMakeLists.txt | 7 +++++
generate-hooklist.sh | 20 +++++++++++++
5 files changed, 43 insertions(+), 37 deletions(-)
create mode 100755 generate-hooklist.sh
POSIX does not support using a semicolon after a closing brace [1],
grepping our code base with
git grep 'sed .*};' '*.sh'
does not give any matches so I don't think we're using that pattern
any where else. Replacing the semicolon with ' -e' would fix it.
Best Wishes
Phillip
Does this fail on any system you're aware of? If so what OS/version (and
preferably version of "sed").
I'm not aware of any such system but I rarely use anything other than
linux. As this departure from POSIX is not already in the code base I
thought it was worth flagging it. I did wonder if it would be supported
by the various BSDs but you testing shows it is actually quite widely
supported.
Best Wishes
Phillip
René's downthread [off-list ref] seems to
suggest that this is fine.
Both beforehand and just now I've tested this on AIX, Solaris,
{Open,Net,Free}BSD, HP/UX, OSX and Linux (a few distros/versions).
All of them are able to generate the same hook-list.h using this version
of the patch.
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/
POSIX does not support using a semicolon after a closing brace [1],
grepping our code base with
git grep 'sed .*};' '*.sh'
does not give any matches so I don't think we're using that pattern
any where else. Replacing the semicolon with ' -e' would fix it.
Best Wishes
Phillip
Does this fail on any system you're aware of? If so what OS/version
(and
preferably version of "sed").
I'm not aware of any such system but I rarely use anything other than
linux. As this departure from POSIX is not already in the code base I
thought it was worth flagging it. I did wonder if it would be
supported by the various BSDs but you testing shows it is actually
quite widely supported.
Best Wishes
Phillip
Thanks, I completely agree that it's worth changing either way (as I did
in my v2). I just wondered if it was a careful reading of the standard
or having run into it on a system in the wild, in that case I'd try to
test on that system in the future.
Good eyes :)
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/;
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:
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.
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/;
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.
quoted 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:
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.
hook-list.h is only included from buitin/bugreport.c, so
builtin/bugreport.o should be the one with the hook-list.h dependency,
shouldn't it?
Well spotted, yes. This is a mistake. I think from some earlier WIP
version of the series.
In practice we don't really miss dependencies due to these sorts of
mistakes since we use the .depends files, i.e. GCC & Clang figure this
out for us:
$ grep hook-list .depend/* */.depend/*
builtin/.depend/bugreport.o.d: compat/compiler.h git-compat-util.h hook.h hook-list.h
builtin/.depend/bugreport.o.d:hook-list.h:
But we do over-depend a bit, if you touch hook-list.h and make
builtin/help.o we'll re-generate it due to this line.
If you or anyone else is more generally interested in the Makefile I'd
really like to get some reviews over at:
https://lore.kernel.org/git/cover-v2-00.18-00000000000-20211112T214150Z-avarab@gmail.com/
I've got some follow-up work that solve these dependencies more
generally, e.g. in this case we should really not need to slavishly
maintain these fallback dependency lists by hand, or have automated ways
of validating their correctness.
hook-list.h is only included from buitin/bugreport.c, so
builtin/bugreport.o should be the one with the hook-list.h dependency,
shouldn't it?
Well spotted, yes. This is a mistake. I think from some earlier WIP
version of the series.
In practice we don't really miss dependencies due to these sorts of
mistakes since we use the .depends files, i.e. GCC & Clang figure this
out for us:
$ grep hook-list .depend/* */.depend/*
builtin/.depend/bugreport.o.d: compat/compiler.h git-compat-util.h hook.h hook-list.h
builtin/.depend/bugreport.o.d:hook-list.h:
But aren't those .depends files are only created when compiling object
files, such that builtin/.depend/bugreport.o.d wouldn't exist until
bugreport.c is compiled, which would fail if hook-list.h wasn't created
before that?
Mike
hook-list.h is only included from buitin/bugreport.c, so
builtin/bugreport.o should be the one with the hook-list.h dependency,
shouldn't it?
Well spotted, yes. This is a mistake. I think from some earlier WIP
version of the series.
In practice we don't really miss dependencies due to these sorts of
mistakes since we use the .depends files, i.e. GCC & Clang figure this
out for us:
$ grep hook-list .depend/* */.depend/*
builtin/.depend/bugreport.o.d: compat/compiler.h git-compat-util.h hook.h hook-list.h
builtin/.depend/bugreport.o.d:hook-list.h:
But aren't those .depends files are only created when compiling object
files, such that builtin/.depend/bugreport.o.d wouldn't exist until
bugreport.c is compiled, which would fail if hook-list.h wasn't created
before that?
Fail how?
I don't think it could fail, because the purpose of these dependency
relationships is to avoid needless *re*builds. So if you're building for
the first time it doesn't matter, your compiler will find the relevant
things to include for you. It doesn't need what's in the Makefile to do
that.
See [1], what I said about LIB_H there applies more generally for the
.depends files.
It will only fail in the sense that it over-depends, i.e. if you do:
git clean -dxf; make builtin/help.o
We'll generate the hook list, as opposed to for buildin/add.o or
whatever, but I'd say that generally doesn't matter all that much, what
matters is that we don't miss dependency relationships.
Anyway, I'll try to loop around to fixing this, just wanted to gather
some interest in the more general set of fixes I've got. I've got some
fixes queued up after that that fix/improve the *.sp and *.s parts of
the line you quoted (and other similar lines). I'd prefer just to fix
this along with those so the two don't textually conflict.
1. https://lore.kernel.org/git/211110.86h7cki0uo.gmgdl@evledraar.gmail.com/
hook-list.h is only included from buitin/bugreport.c, so
builtin/bugreport.o should be the one with the hook-list.h dependency,
shouldn't it?
Well spotted, yes. This is a mistake. I think from some earlier WIP
version of the series.
In practice we don't really miss dependencies due to these sorts of
mistakes since we use the .depends files, i.e. GCC & Clang figure this
out for us:
$ grep hook-list .depend/* */.depend/*
builtin/.depend/bugreport.o.d: compat/compiler.h git-compat-util.h hook.h hook-list.h
builtin/.depend/bugreport.o.d:hook-list.h:
But aren't those .depends files are only created when compiling object
files, such that builtin/.depend/bugreport.o.d wouldn't exist until
bugreport.c is compiled, which would fail if hook-list.h wasn't created
before that?
Fail how?
I don't think it could fail, because the purpose of these dependency
relationships is to avoid needless *re*builds. So if you're building for
the first time it doesn't matter, your compiler will find the relevant
things to include for you. It doesn't need what's in the Makefile to do
that.
See [1], what I said about LIB_H there applies more generally for the
.depends files.
It will only fail in the sense that it over-depends, i.e. if you do:
git clean -dxf; make builtin/help.o
Try
git clean -dxf; make builtin/bugreport.o
It fails with:
CC builtin/bugreport.o
builtin/bugreport.c:7:10: fatal error: hook-list.h: そのようなファイルやディレクトリはありません
7 | #include "hook-list.h"
| ^~~~~~~~~~~~~
compilation terminated.
make: *** [Makefile:2500: builtin/bugreport.o] エラー 1
The only reason I can see why it builds at all normally is that hook.o
is built soon enough that by the time builtin/bugreport.o is built
hook-list.h has already been generated.
Mike
hook-list.h is only included from buitin/bugreport.c, so
builtin/bugreport.o should be the one with the hook-list.h dependency,
shouldn't it?
Well spotted, yes. This is a mistake. I think from some earlier WIP
version of the series.
In practice we don't really miss dependencies due to these sorts of
mistakes since we use the .depends files, i.e. GCC & Clang figure this
out for us:
$ grep hook-list .depend/* */.depend/*
builtin/.depend/bugreport.o.d: compat/compiler.h git-compat-util.h hook.h hook-list.h
builtin/.depend/bugreport.o.d:hook-list.h:
But aren't those .depends files are only created when compiling object
files, such that builtin/.depend/bugreport.o.d wouldn't exist until
bugreport.c is compiled, which would fail if hook-list.h wasn't created
before that?
Fail how?
I don't think it could fail, because the purpose of these dependency
relationships is to avoid needless *re*builds. So if you're building for
the first time it doesn't matter, your compiler will find the relevant
things to include for you. It doesn't need what's in the Makefile to do
that.
See [1], what I said about LIB_H there applies more generally for the
.depends files.
It will only fail in the sense that it over-depends, i.e. if you do:
git clean -dxf; make builtin/help.o
Try
git clean -dxf; make builtin/bugreport.o
It fails with:
CC builtin/bugreport.o
builtin/bugreport.c:7:10: fatal error: hook-list.h: そのようなファイルやディレクトリはありません
7 | #include "hook-list.h"
| ^~~~~~~~~~~~~
compilation terminated.
make: *** [Makefile:2500: builtin/bugreport.o] エラー 1
The only reason I can see why it builds at all normally is that hook.o
is built soon enough that by the time builtin/bugreport.o is built
hook-list.h has already been generated.
From: Junio C Hamano <hidden> Date: 2021-11-17 08:39:16
Ævar Arnfjörð Bjarmason [off-list ref] writes:
I submitted a re-roll of the greater dependency fix-up & optimization
series I've got kicking around, which includes a fix for this
issue. Thank you for the report:
Makes me wonder what other breakages are introduced in such a big
series, in exchange for correcting this single issue X-<. We'll see
soon enough, I guess ;-)