Emily Shaffer [off-list ref] writes:
quoted hunk
diff --git a/builtin/hook.c b/builtin/hook.c
index c36b05376c..3aa65dd791 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -46,7 +46,7 @@ static int list(int argc, const char **argv, const char *prefix)
hookname = argv[0];
- head = hook_list(hookname);
+ head = hook_list(hookname, 1);
if (list_empty(head)) {
printf(_("no commands configured for hook '%s'\n"),@@ -108,7 +108,7 @@ static int run(int argc, const char **argv, const char *prefix)
git_config(git_default_config, NULL);
hook_name = argv[0];
- hooks = hook_list(hook_name);
+ hooks = hook_list(hook_name, 1);
if (list_empty(hooks)) {
/* ... act like run_hooks_oneshot() under --ignore-missing */
if (ignore_missing)
This is minor, as I expect that the callers of hook_list() will
always confined in builtin/hook.c, but it probably is easier to read
if you gave two functions, just like you have the pair of helpers
find_hook() and find_hook_gently(), as the literal "1" forces the
readers to remember if that means "die if not found", or "ok if it
is a bogus name".
In addition, it may make more sense to keep hook_list() signal
failure by returning NULL and leave the dying to the caller.
In-code callers (as opposed to "git hook run" that can throw any
random string that came from the user at the API) will never throw a
bogus name unless there is a bug, and they'll have to check for an
error return from hook_list() anyway and the error message they
would give may have to be different from the one that is given
against a hook name randomly thrown at us by the user.
On Thu, Aug 12, 2021 at 12:08:10PM -0700, Junio C Hamano wrote:
Emily Shaffer [off-list ref] writes:
quoted
diff --git a/builtin/hook.c b/builtin/hook.c
index c36b05376c..3aa65dd791 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -46,7 +46,7 @@ static int list(int argc, const char **argv, const char *prefix)
hookname = argv[0];
- head = hook_list(hookname);
+ head = hook_list(hookname, 1);
if (list_empty(head)) {
printf(_("no commands configured for hook '%s'\n"),@@ -108,7 +108,7 @@ static int run(int argc, const char **argv, const char *prefix)
git_config(git_default_config, NULL);
hook_name = argv[0];
- hooks = hook_list(hook_name);
+ hooks = hook_list(hook_name, 1);
if (list_empty(hooks)) {
/* ... act like run_hooks_oneshot() under --ignore-missing */
if (ignore_missing)
This is minor, as I expect that the callers of hook_list() will
always confined in builtin/hook.c, but it probably is easier to read
if you gave two functions, just like you have the pair of helpers
find_hook() and find_hook_gently(), as the literal "1" forces the
readers to remember if that means "die if not found", or "ok if it
is a bogus name".
Yes, I see what you mean. Ok. I have been wanting to change the naming
anyways - most functions in hook.h are verb-y ("find hook", "run hooks",
so on) but hook_list stands out as the only noun-y function.
So I considered changing it to "list_hooks" and "list_hooks_gently", to align
with find_hook(_gently)....
In addition, it may make more sense to keep hook_list() signal
failure by returning NULL and leave the dying to the caller.
In-code callers (as opposed to "git hook run" that can throw any
random string that came from the user at the API) will never throw a
bogus name unless there is a bug, and they'll have to check for an
error return from hook_list() anyway and the error message they
would give may have to be different from the one that is given
against a hook name randomly thrown at us by the user.
Sure, that makes sense enough... but then I wonder if it would be better
to let the caller check whether the name is allowed at all, first,
separately from the hook_list() call.
On the one hand, having hook_list() do the validation of the hook name
makes it harder for a hook doing something very unusual to neglect to
add documentation. (I'm thinking of, for example, a hook doing something
equally weird to the proc-receive hook, which cannot use the hook
library because it needs to be able to do this weird two-way
communication thing.
(https://lore.kernel.org/git/20210527000856.695702-31-emilyshaffer%40google.com))
It would be pretty bad for a hook which is already complicated to also
forget to include documentation.
On the other hand, as it is now - builtin/hook.c hardcodes "I don't care
if the hook is unknown" and hook.c hardcodes "reject if the hook is
unknown" and nobody else calls hook_list at all - it isn't so bad to
bail early, before even calling hook_list() in the first place, if the
hook is unknown.
I also think that approach would make a callsite easier to understand
than checking for null from hook_list().
const char *hookname = "my-new-hook";
/* Here it's pretty clear what the reason for the error was... */
if (!known_hook(hookname))
BUG("is hook '%s' in Documentation/githooks.txt?", hookname);
hooks = hook_list(hookname);
...
vs.
const char *hookname = "my-new-hook";
hooks = hook_list(hookname);
/*
* But here, I have to go and look at the hook_list() source to
* understand why null 'hooks' means I missed some doc step.
*/
if (!hookname)
BUG("is hook '%s' in Documentation/githooks.txt?", hookname);
...
Maybe others disagree with me, but I would guess the first example is
more easily understandable to someone unfamiliar with the hook code. So
I think I will go with that approach, and include some notice in the doc
comment over hook_list().
- Emily
On Wed, Aug 18, 2021 at 01:51:58PM -0700, Emily Shaffer wrote:
On Thu, Aug 12, 2021 at 12:08:10PM -0700, Junio C Hamano wrote:
quoted
Emily Shaffer [off-list ref] writes:
quoted
diff --git a/builtin/hook.c b/builtin/hook.c
index c36b05376c..3aa65dd791 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -46,7 +46,7 @@ static int list(int argc, const char **argv, const char *prefix)
hookname = argv[0];
- head = hook_list(hookname);
+ head = hook_list(hookname, 1);
if (list_empty(head)) {
printf(_("no commands configured for hook '%s'\n"),@@ -108,7 +108,7 @@ static int run(int argc, const char **argv, const char *prefix)
git_config(git_default_config, NULL);
hook_name = argv[0];
- hooks = hook_list(hook_name);
+ hooks = hook_list(hook_name, 1);
if (list_empty(hooks)) {
/* ... act like run_hooks_oneshot() under --ignore-missing */
if (ignore_missing)
This is minor, as I expect that the callers of hook_list() will
always confined in builtin/hook.c, but it probably is easier to read
if you gave two functions, just like you have the pair of helpers
find_hook() and find_hook_gently(), as the literal "1" forces the
readers to remember if that means "die if not found", or "ok if it
is a bogus name".
Yes, I see what you mean. Ok. I have been wanting to change the naming
anyways - most functions in hook.h are verb-y ("find hook", "run hooks",
so on) but hook_list stands out as the only noun-y function.
So I considered changing it to "list_hooks" and "list_hooks_gently", to align
with find_hook(_gently)....
quoted
In addition, it may make more sense to keep hook_list() signal
failure by returning NULL and leave the dying to the caller.
In-code callers (as opposed to "git hook run" that can throw any
random string that came from the user at the API) will never throw a
bogus name unless there is a bug, and they'll have to check for an
error return from hook_list() anyway and the error message they
would give may have to be different from the one that is given
against a hook name randomly thrown at us by the user.
Sure, that makes sense enough... but then I wonder if it would be better
to let the caller check whether the name is allowed at all, first,
separately from the hook_list() call.
On the one hand, having hook_list() do the validation of the hook name
makes it harder for a hook doing something very unusual to neglect to
add documentation. (I'm thinking of, for example, a hook doing something
equally weird to the proc-receive hook, which cannot use the hook
library because it needs to be able to do this weird two-way
communication thing.
(https://lore.kernel.org/git/20210527000856.695702-31-emilyshaffer%40google.com))
It would be pretty bad for a hook which is already complicated to also
forget to include documentation.
On the other hand, as it is now - builtin/hook.c hardcodes "I don't care
if the hook is unknown" and hook.c hardcodes "reject if the hook is
unknown" and nobody else calls hook_list at all - it isn't so bad to
bail early, before even calling hook_list() in the first place, if the
hook is unknown.
I also think that approach would make a callsite easier to understand
than checking for null from hook_list().
const char *hookname = "my-new-hook";
/* Here it's pretty clear what the reason for the error was... */
if (!known_hook(hookname))
BUG("is hook '%s' in Documentation/githooks.txt?", hookname);
hooks = hook_list(hookname);
...
vs.
const char *hookname = "my-new-hook";
hooks = hook_list(hookname);
/*
* But here, I have to go and look at the hook_list() source to
* understand why null 'hooks' means I missed some doc step.
*/
if (!hookname)
BUG("is hook '%s' in Documentation/githooks.txt?", hookname);
...
Maybe others disagree with me, but I would guess the first example is
more easily understandable to someone unfamiliar with the hook code. So
I think I will go with that approach, and include some notice in the doc
comment over hook_list().
Hm. Now that I sit to type it, I guess putting the onus on the
strange-new-hook caller to also type "if known_hook()" is about the same
as just expecting the strange-new-hook caller to know they are supposed
to document their hook. Plus, known_hook() is static right now.
I think it still makes sense to BUG() instead of error() or die() in
'list_hooks()' (non-gently) - the failure of that call is a developer
error, either in not having documented their hook correctly or in
calling 'list_hooks()' instead of 'list_hooks_gently()' when they meant
the latter. So I will not take the NULL return approach.
- Emily