On Thu, Mar 11 2021, Emily Shaffer wrote:
> Since v7:
> - Addressed Jonathan Tan's review of part I
> - Addressed Junio's review of part I and II
> - Combined parts I and II
>
> I think the updates to patch 1 between the rest of the work I've been
> doing probably have covered Ævar's comments.
A range-diff between iterations of such a large series would be most
useful. Do you have a public repo with tags or whatever the different
versions, for those who'd like an easier way to follow along the
differing versions than scraping the ML archive?
While reading this I came up with the following fixup patches on top,
for discussion, maybe not something you want as-is:
Documentation/git-hook.txt | 8 +++++
builtin/bugreport.c | 8 +++--
builtin/commit.c | 3 +-
builtin/hook.c | 79 ++++++++++++++++++++--------------------------
builtin/merge.c | 3 +-
builtin/receive-pack.c | 11 +++----
hook.c | 21 +++++-------
hook.h | 5 +--
refs.c | 4 ++-
sequencer.c | 4 ++-
10 files changed, 73 insertions(+), 73 deletions(-)
diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
index 4ad31ac360a..5c9af30b43e 100644
--- a/Documentation/git-hook.txt
+++ b/Documentation/git-hook.txt
@@ -150,10 +150,18 @@ message body and cannot be parallelized.
CONFIGURATION
-------------
+
+The below documentation is the same as what's found in
+linkgit:git-config[1]:
+
include::config/hook.txt[]
HOOKS
-----
+
+The below documentation is the same as what's found in
+linkgit:githooks[5]:
+
include::native-hooks.txt[]
Noted in another reply, including it here for completeness.
GIT
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 04467cd1d3a..b64e53fd625 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -81,9 +81,13 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
return;
}
- for (i = 0; i < ARRAY_SIZE(hook); i++)
- if (hook_exists(hook[i], HOOKDIR_USE_CONFIG))
+ for (i = 0; i < ARRAY_SIZE(hook); i++) {
+ struct strbuf config;
+ strbuf_addf(&config, "hook.%s.config", hook[i]);
+ if (hook_exists(hook[i], config.buf, HOOKDIR_USE_CONFIG))
strbuf_addf(hook_info, "%s\n", hook[i]);
+ strbuf_release(&config);
+ }
}
Less strbuf, see below.
static const char * const bugreport_usage[] = {
diff --git a/builtin/commit.c b/builtin/commit.c
index 31df571f123..fc9f1f5ee58 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -984,7 +984,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
return 0;
}
- if (!no_verify && hook_exists("pre-commit", HOOKDIR_USE_CONFIG)) {
+ if (!no_verify && hook_exists("pre-commit", "hook.pre-commit.command",
+ HOOKDIR_USE_CONFIG)) {
/*
* Re-read the index as pre-commit hook could have updated it,
* and write it out as a tree. We must do this before we invoke
..ditto.
diff --git a/builtin/hook.c b/builtin/hook.c
index b4f4adb1dea..d0b56ee47f8 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -18,8 +18,6 @@ static enum hookdir_opt should_run_hookdir;
static int list(int argc, const char **argv, const char *prefix)
{
struct list_head *head, *pos;
- struct strbuf hookname = STRBUF_INIT;
- struct strbuf hookdir_annotation = STRBUF_INIT;
struct option list_options[] = {
OPT_END(),
@@ -33,67 +31,60 @@ static int list(int argc, const char **argv, const char *prefix)
builtin_hook_usage, list_options);
}
- strbuf_addstr(&hookname, argv[0]);
-
- head = hook_list(&hookname);
+ head = hook_list(argv[0]);
if (list_empty(head)) {
printf(_("no commands configured for hook '%s'\n"),
- hookname.buf);
- strbuf_release(&hookname);
+ argv[0]);
return 0;
}
- switch (should_run_hookdir) {
- case HOOKDIR_NO:
- strbuf_addstr(&hookdir_annotation, _(" (will not run)"));
- break;
- case HOOKDIR_ERROR:
- strbuf_addstr(&hookdir_annotation, _(" (will error and not run)"));
- break;
- case HOOKDIR_INTERACTIVE:
- strbuf_addstr(&hookdir_annotation, _(" (will prompt)"));
- break;
- case HOOKDIR_WARN:
- strbuf_addstr(&hookdir_annotation, _(" (will warn but run)"));
- break;
- case HOOKDIR_YES:
- /*
- * The default behavior should agree with
- * hook.c:configured_hookdir_opt(). HOOKDIR_UNKNOWN should just
- * do the default behavior.
- */
- case HOOKDIR_UNKNOWN:
- default:
- break;
- }
-
list_for_each(pos, head) {
struct hook *item = list_entry(pos, struct hook, list);
item = list_entry(pos, struct hook, list);
if (item) {
- /* Don't translate 'hookdir' - it matches the config */
- printf("%s: %s%s\n",
- (item->from_hookdir
+ const char *scope = item->from_hookdir
? "hookdir"
- : config_scope_name(item->origin)),
- item->command.buf,
- (item->from_hookdir
- ? hookdir_annotation.buf
- : ""));
+ : config_scope_name(item->origin);
+ switch (should_run_hookdir) {
+ case HOOKDIR_NO:
+ printf(_("%s: %s (will not run)\n"),
+ scope, item->command.buf);
+ break;
+ case HOOKDIR_ERROR:
+ printf(_("%s: %s (will error and not run)\n"),
+ scope, item->command.buf);
+ break;
+ case HOOKDIR_INTERACTIVE:
+ printf(_("%s: %s (will prompt)\n"),
+ scope, item->command.buf);
+ break;
+ case HOOKDIR_WARN:
+ printf(_("%s: %s (will warn but run)\n"),
+ scope, item->command.buf);
+ break;
+ case HOOKDIR_YES:
+ /*
+ * The default behavior should agree with
+ * hook.c:configured_hookdir_opt(). HOOKDIR_UNKNOWN should just
+ * do the default behavior.
+ */
+ case HOOKDIR_UNKNOWN:
+ default:
+ printf(_("%s: %s\n"),
+ scope, item->command.buf);
+ break;
+ }
}
}
clear_hook_list(head);
- strbuf_release(&hookdir_annotation);
- strbuf_release(&hookname);
return 0;
}
I think this is better to avoid i18n lego, as noted in another reply
(but I didn't include the patch).
More on strbuf below:
static int run(int argc, const char **argv, const char *prefix)
{
- struct strbuf hookname = STRBUF_INIT;
struct run_hooks_opt opt;
int rc = 0;
@@ -118,12 +109,10 @@ static int run(int argc, const char **argv, const char *prefix)
usage_msg_opt(_("You must specify a hook event to run."),
builtin_hook_usage, run_options);
- strbuf_addstr(&hookname, argv[0]);
opt.run_hookdir = should_run_hookdir;
- rc = run_hooks(hookname.buf, &opt);
+ rc = run_hooks(argv[0], &opt);
- strbuf_release(&hookname);
run_hooks_opt_clear(&opt);
return rc;
diff --git a/builtin/merge.c b/builtin/merge.c
index 3a2af257a6b..df4ff72fbc7 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -848,7 +848,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
* and write it out as a tree. We must do this before we invoke
* the editor and after we invoke run_status above.
*/
- if (hook_exists("pre-merge-commit", HOOKDIR_USE_CONFIG))
+ if (hook_exists("pre-merge-commit", "hook.pre-merge-commit.command",
+ HOOKDIR_USE_CONFIG))
discard_cache();
read_cache_from(index_file);
strbuf_addbuf(&msg, &merge_msg);
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index eaedeeb1e8b..a76069ea592 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1123,12 +1123,10 @@ static int run_proc_receive_hook(struct command *commands,
int version = 0;
int code;
- struct strbuf hookname = STRBUF_INIT;
struct hook *proc_receive = NULL;
struct list_head *pos, *hooks;
- strbuf_addstr(&hookname, "proc-receive");
- hooks = hook_list(&hookname);
+ hooks = hook_list("proc-receive");
list_for_each(pos, hooks) {
if (proc_receive) {
@@ -1460,8 +1458,6 @@ static const char *push_to_deploy(unsigned char *sha1,
return NULL;
}
-static const char *push_to_checkout_hook = "push-to-checkout";
-
static const char *push_to_checkout(unsigned char *hash,
struct strvec *env,
const char *work_tree)
@@ -1472,7 +1468,7 @@ static const char *push_to_checkout(unsigned char *hash,
strvec_pushf(env, "GIT_WORK_TREE=%s", absolute_path(work_tree));
strvec_pushv(&opt.env, env->v);
strvec_push(&opt.args, hash_to_hex(hash));
- if (run_hooks(push_to_checkout_hook, &opt)) {
+ if (run_hooks("push-to-checkout", &opt)) {
run_hooks_opt_clear(&opt);
return "push-to-checkout hook declined";
} else {
@@ -1502,7 +1498,8 @@ static const char *update_worktree(unsigned char *sha1, const struct worktree *w
strvec_pushf(&env, "GIT_DIR=%s", absolute_path(git_dir));
- if (!hook_exists(push_to_checkout_hook, HOOKDIR_USE_CONFIG))
+ if (!hook_exists("push-to-checkout", "hook.push-to-checkout.command",
+ HOOKDIR_USE_CONFIG))
retval = push_to_deploy(sha1, &env, work_tree);
else
retval = push_to_checkout(sha1, &env, work_tree);
diff --git a/hook.c b/hook.c
index 7f6f3b9a616..49c3861ce00 100644
--- a/hook.c
+++ b/hook.c
@@ -247,7 +247,7 @@ static const char *find_legacy_hook(const char *name)
}
-struct list_head* hook_list(const struct strbuf* hookname)
+struct list_head* hook_list(const char *hookname)
{
struct strbuf hook_key = STRBUF_INIT;
struct list_head *hook_head = xmalloc(sizeof(struct list_head));
@@ -256,14 +256,14 @@ struct list_head* hook_list(const struct strbuf* hookname)
INIT_LIST_HEAD(hook_head);
if (!hookname)
- return NULL;
+ BUG("???");;
- strbuf_addf(&hook_key, "hook.%s.command", hookname->buf);
+ strbuf_addf(&hook_key, "hook.%s.command", hookname);
git_config(hook_config_lookup, &cb_data);
if (have_git_dir()) {
- const char *legacy_hook_path = find_legacy_hook(hookname->buf);
+ const char *legacy_hook_path = find_legacy_hook(hookname);
/* Unconditionally add legacy hook, but annotate it. */
if (legacy_hook_path) {
@@ -300,10 +300,10 @@ void run_hooks_opt_init_async(struct run_hooks_opt *o)
o->jobs = configured_hook_jobs();
}
-int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir)
+int hook_exists(const char *hookname, const char *hook_config,
+ enum hookdir_opt should_run_hookdir)
{
const char *value = NULL; /* throwaway */
- struct strbuf hook_key = STRBUF_INIT;
int could_run_hookdir;
if (should_run_hookdir == HOOKDIR_USE_CONFIG)
@@ -314,9 +314,7 @@ int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir)
should_run_hookdir == HOOKDIR_YES)
&& !!find_legacy_hook(hookname);
- strbuf_addf(&hook_key, "hook.%s.command", hookname);
-
- return (!git_config_get_value(hook_key.buf, &value)) || could_run_hookdir;
+ return (!git_config_get_value(hook_config, &value)) || could_run_hookdir;
}
void run_hooks_opt_clear(struct run_hooks_opt *o)
@@ -438,7 +436,6 @@ static int notify_hook_finished(int result,
int run_hooks(const char *hookname, struct run_hooks_opt *options)
{
- struct strbuf hookname_str = STRBUF_INIT;
struct list_head *to_run, *pos = NULL, *tmp = NULL;
struct hook_cb_data cb_data = { 0, NULL, NULL, options };
@@ -448,9 +445,7 @@ int run_hooks(const char *hookname, struct run_hooks_opt *options)
if (options->path_to_stdin && options->feed_pipe)
BUG("choose only one method to populate stdin");
- strbuf_addstr(&hookname_str, hookname);
-
- to_run = hook_list(&hookname_str);
+ to_run = hook_list(hookname);
list_for_each_safe(pos, tmp, to_run) {
struct hook *hook = list_entry(pos, struct hook, list);
diff --git a/hook.h b/hook.h
index 4ff9999b049..bfbbf36882d 100644
--- a/hook.h
+++ b/hook.h
@@ -26,7 +26,7 @@ struct hook {
* Provides a linked list of 'struct hook' detailing commands which should run
* in response to the 'hookname' event, in execution order.
*/
-struct list_head* hook_list(const struct strbuf *hookname);
+struct list_head* hook_list(const char *hookname);
enum hookdir_opt
{
@@ -123,7 +123,8 @@ void run_hooks_opt_clear(struct run_hooks_opt *o);
* Like with run_hooks, if you take a --run-hookdir flag, reflect that
* user-specified behavior here instead.
*/
-int hook_exists(const char *hookname, enum hookdir_opt should_run_hookdir);
+int hook_exists(const char *hookname, const char *hook_config,
+ enum hookdir_opt should_run_hookdir);
/*
* Runs all hooks associated to the 'hookname' event in order. Each hook will be
diff --git a/refs.c b/refs.c
index 334fdd9103c..f01995fe64f 100644
--- a/refs.c
+++ b/refs.c
@@ -1966,7 +1966,9 @@ static int run_transaction_hook(struct ref_transaction *transaction,
run_hooks_opt_init_async(&opt);
- if (!hook_exists("reference-transaction", HOOKDIR_USE_CONFIG))
+ if (!hook_exists("reference-transaction",
+ "hook.reference-transaction.command",
+ HOOKDIR_USE_CONFIG))
return ret;
strvec_push(&opt.args, state);
diff --git a/sequencer.c b/sequencer.c
index 34ff275f0d1..52c067c1688 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1436,7 +1436,9 @@ static int try_to_commit(struct repository *r,
}
}
- if (hook_exists("prepare-commit-msg", HOOKDIR_USE_CONFIG)) {
+ if (hook_exists("prepare-commit-msg",
+ "hook.prepare-commit-msg.command",
+ HOOKDIR_USE_CONFIG)) {
res = run_prepare_commit_msg_hook(r, msg, hook_commit);
if (res)
goto out;
There was another reply (from JT I believe, but didn't go back and look
it up) about the over use of strbuf.
I tend to agree, as much as I love the API it's really not better to
write C with it if all you need is a const char* that's never modified,
particularly if you get it from elsewhere.
So it's really not meant for or good for "everything we need a const
char*", but to avoid verbose realloc() dances all over the place, and
for things like getline() loops without a hardcoded buffer size.
E.g. in the first hunk here we're creating a strbuf just to copy argv[0]
to it, and then throwing it away, let's just pass down argv[0].
For hook_exists I think just having the code more grep-able and having
the config value inline is better, but I admit that's a matter of taste.
I didn't try to find all such strbuf() occurrences, anyway, in the
overall scheme of things it's a relatively small nit.
I'm hoping to do some deeper diving into this series, in particular the
parallelism, but just sending the shallow-ish comments I have for now.
Thanks for working on this!
