diff mbox series

[v4,8/9] commit: use config-based hooks

Message ID 20200909004939.1942347-9-emilyshaffer@google.com
State New
Headers show
Series propose config-based hooks | expand

Commit Message

Emily Shaffer Sept. 9, 2020, 12:49 a.m. UTC
As part of the adoption of config-based hooks, teach run_commit_hook()
to call hook.h instead of run-command.h. This covers 'pre-commit',
'commit-msg', and 'prepare-commit-msg'. Additionally, ask the hook
library - not run-command - whether any hooks will be run, as it's
possible hooks may exist in the config but not the hookdir.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
---
 builtin/commit.c                                 |  3 ++-
 builtin/merge.c                                  |  3 ++-
 commit.c                                         | 13 ++++++++++++-
 t/t7503-pre-commit-and-pre-merge-commit-hooks.sh | 13 +++++++++++++
 4 files changed, 29 insertions(+), 3 deletions(-)

Comments

Phillip Wood Sept. 10, 2020, 1:50 p.m. UTC | #1
Hi Emily

On 09/09/2020 01:49, Emily Shaffer wrote:
> As part of the adoption of config-based hooks, teach run_commit_hook()
> to call hook.h instead of run-command.h. This covers 'pre-commit',
> 'commit-msg', and 'prepare-commit-msg'. Additionally, ask the hook
> library - not run-command - whether any hooks will be run, as it's
> possible hooks may exist in the config but not the hookdir.
> 
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
>   builtin/commit.c                                 |  3 ++-
>   builtin/merge.c                                  |  3 ++-
>   commit.c                                         | 13 ++++++++++++-
>   t/t7503-pre-commit-and-pre-merge-commit-hooks.sh | 13 +++++++++++++
>   4 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 69ac78d5e5..a19c6478eb 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -36,6 +36,7 @@
>   #include "help.h"
>   #include "commit-reach.h"
>   #include "commit-graph.h"
> +#include "hook.h"
>   
>   static const char * const builtin_commit_usage[] = {
>   	N_("git commit [<options>] [--] <pathspec>..."),
> @@ -985,7 +986,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>   		return 0;
>   	}
>   
> -	if (!no_verify && find_hook("pre-commit")) {
> +	if (!no_verify && hook_exists("pre-commit")) {
>   		/*
>   		 * 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
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 74829a838e..c1a9d0083d 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -41,6 +41,7 @@
>   #include "commit-reach.h"
>   #include "wt-status.h"
>   #include "commit-graph.h"
> +#include "hook.h"
>   
>   #define DEFAULT_TWOHEAD (1<<0)
>   #define DEFAULT_OCTOPUS (1<<1)
> @@ -829,7 +830,7 @@ 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 (find_hook("pre-merge-commit"))
> +	if (hook_exists("pre-merge-commit"))
>   		discard_cache();
>   	read_cache_from(index_file);
>   	strbuf_addbuf(&msg, &merge_msg);
> diff --git a/commit.c b/commit.c
> index 4ce8cb38d5..c7a243e848 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -21,6 +21,7 @@
>   #include "commit-reach.h"
>   #include "run-command.h"
>   #include "shallow.h"
> +#include "hook.h"
>   
>   static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);
>   
> @@ -1632,8 +1633,13 @@ int run_commit_hook(int editor_is_used, const char *index_file,
>   {
>   	struct strvec hook_env = STRVEC_INIT;
>   	va_list args;
> +	const char *arg;
> +	struct strvec hook_args = STRVEC_INIT;
> +	struct strbuf hook_name = STRBUF_INIT;
>   	int ret;
>   
> +	strbuf_addstr(&hook_name, name);

Seeing this makes me wonder if it would be better for run_hooks() to 
take a string for the name rather than an strbuf, I suspect that 
virtually all callers have a fixed hook name.

Best Wishes

Phillip

>   	strvec_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
>   
>   	/*
> @@ -1643,9 +1649,14 @@ int run_commit_hook(int editor_is_used, const char *index_file,
>   		strvec_push(&hook_env, "GIT_EDITOR=:");
>   
>   	va_start(args, name);
> -	ret = run_hook_ve(hook_env.v, name, args);
> +	while ((arg = va_arg(args, const char *)))
> +		strvec_push(&hook_args, arg);
>   	va_end(args);
> +
> +	ret = run_hooks(hook_env.v, &hook_name, &hook_args);
>   	strvec_clear(&hook_env);
> +	strvec_clear(&hook_args);
> +	strbuf_release(&hook_name);
>   
>   	return ret;
>   }
> diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
> index b3485450a2..cef8085dcc 100755
> --- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
> +++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
> @@ -103,6 +103,19 @@ test_expect_success 'with succeeding hook' '
>   	test_cmp expected_hooks actual_hooks
>   '
>   
> +# NEEDSWORK: when 'git hook add' and 'git hook remove' have been added, use that
> +# instead
> +test_expect_success 'with succeeding hook (config-based)' '
> +	test_when_finished "git config --unset hook.pre-commit.command success.sample" &&
> +	test_when_finished "rm -f expected_hooks actual_hooks" &&
> +	git config hook.pre-commit.command "$HOOKDIR/success.sample" &&
> +	echo "$HOOKDIR/success.sample" >expected_hooks &&
> +	echo "more" >>file &&
> +	git add file &&
> +	git commit -m "more" &&
> +	test_cmp expected_hooks actual_hooks
> +'
> +
>   test_expect_success 'with succeeding hook (merge)' '
>   	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
>   	cp "$HOOKDIR/success.sample" "$PREMERGE" &&
>
Junio C Hamano Sept. 10, 2020, 10:21 p.m. UTC | #2
Phillip Wood <phillip.wood123@gmail.com> writes:

>> +	const char *arg;
>> +	struct strvec hook_args = STRVEC_INIT;
>> +	struct strbuf hook_name = STRBUF_INIT;
>>   	int ret;
>>   +	strbuf_addstr(&hook_name, name);
>
> Seeing this makes me wonder if it would be better for run_hooks() to
> take a string for the name rather than an strbuf, I suspect that
> virtually all callers have a fixed hook name.

Yeah, that is a good point.  It is always a good discipline to keep
the type of the parameters callers need to pass to the minimum.
Jonathan Tan Sept. 23, 2020, 11:47 p.m. UTC | #3
> -	if (!no_verify && find_hook("pre-commit")) {
> +	if (!no_verify && hook_exists("pre-commit")) {

A reviewer would probably need to look at all instances of "pre-commit"
(and likewise for the other hooks) but if the plan is to convert all
hooks, then the reviewer wouldn't need to do this since we could just
delete the "find_hook" function.

Overall comments about the design and scope of the patch set:

 - I think that the abilities of the current patch set regarding
   overriding order of globally-set hook commands is sufficient. We
   should also have some way of disabling globally-set hooks, perhaps
   by implementing the "skip" variable mentioned in patch 1 or by
   allowing the redefinition of hookcmd sections (e.g. by redefining a
   command to "/usr/bin/true"). To me, these provide substantial
   user-facing value, and would be sufficient for a first version - and
   other things like parallelization can come later.

 - As for the UI that should be exposed through the "git hook" command,
   I think that "git hook list" and "git hook run" are sufficient.
   Editing the config files are not too difficult, and "git hook add"
   etc. can be added later.

 - As for whether (1) it is OK for none of the hooks to be converted (and
   instead rely on the user to edit their hook scripts to call "git hook
   run ???"), or if (2) we should require some hooks to be
   converted, or if (3) we should require all hooks to be converted: I'd
   rather have (2) or (3) so that we don't have dead code. I prefer (3),
   especially since a reviewer wouldn't have to worry about leftover
   usages of old functions like find_hook() (as I mentioned at the start
   of this email), but I'm not fully opposed to (2) either.
Emily Shaffer Oct. 5, 2020, 9:27 p.m. UTC | #4
On Wed, Sep 23, 2020 at 04:47:34PM -0700, Jonathan Tan wrote:
> 
> > -	if (!no_verify && find_hook("pre-commit")) {
> > +	if (!no_verify && hook_exists("pre-commit")) {
> 
> A reviewer would probably need to look at all instances of "pre-commit"
> (and likewise for the other hooks) but if the plan is to convert all
> hooks, then the reviewer wouldn't need to do this since we could just
> delete the "find_hook" function.
> 
> Overall comments about the design and scope of the patch set:
> 
>  - I think that the abilities of the current patch set regarding
>    overriding order of globally-set hook commands is sufficient. We
>    should also have some way of disabling globally-set hooks, perhaps
>    by implementing the "skip" variable mentioned in patch 1 or by
>    allowing the redefinition of hookcmd sections (e.g. by redefining a
>    command to "/usr/bin/true"). To me, these provide substantial
>    user-facing value, and would be sufficient for a first version - and
>    other things like parallelization can come later.

OK. I will send 'skip' in the next reroll. Thanks for pointing it out!

> 
>  - As for the UI that should be exposed through the "git hook" command,
>    I think that "git hook list" and "git hook run" are sufficient.
>    Editing the config files are not too difficult, and "git hook add"
>    etc. can be added later.
> 
>  - As for whether (1) it is OK for none of the hooks to be converted (and
>    instead rely on the user to edit their hook scripts to call "git hook
>    run ???"), or if (2) we should require some hooks to be
>    converted, or if (3) we should require all hooks to be converted: I'd
>    rather have (2) or (3) so that we don't have dead code. I prefer (3),
>    especially since a reviewer wouldn't have to worry about leftover
>    usages of old functions like find_hook() (as I mentioned at the start
>    of this email), but I'm not fully opposed to (2) either.

I personally prefer (3) - I think the user experience with (2) in a
release (or even in 'next', which all Googlers use) is pretty bad. The
downside, of course, is that a large topic gets merged all at once and
makes some pretty nasty reviewer overhead.

Junio, I wonder if you can give any advice here? What would be really
ideal for me would be to do something like Stolee has been doing with
his maintenance series - config-based hooks pt. I containing the library
code and config-based hooks pt. II containing the conversion of
preexisting hooks. Does that make the overhead for you significantly
worse?

 - Emily
Jonathan Nieder Oct. 5, 2020, 11:48 p.m. UTC | #5
Emily Shaffer wrote:
> On Wed, Sep 23, 2020 at 04:47:34PM -0700, Jonathan Tan wrote:

>>  - As for whether (1) it is OK for none of the hooks to be converted (and
>>    instead rely on the user to edit their hook scripts to call "git hook
>>    run ???"), or if (2) we should require some hooks to be
>>    converted, or if (3) we should require all hooks to be converted: I'd
>>    rather have (2) or (3) so that we don't have dead code. I prefer (3),
>>    especially since a reviewer wouldn't have to worry about leftover
>>    usages of old functions like find_hook() (as I mentioned at the start
>>    of this email), but I'm not fully opposed to (2) either.
>
> I personally prefer (3) - I think the user experience with (2) in a
> release (or even in 'next', which all Googlers use) is pretty bad. The
> downside, of course, is that a large topic gets merged all at once and
> makes some pretty nasty reviewer overhead.

One approach is to build up a series with "git hook run" and "git hook
list" demonstrating and testing the functionality and [PATCH n+1/n]
extra patches at the end converting existing hooks.  The user
experience from "git hook run" and even "git hook list" supporting a
preview of the future without built-in commands living in that future
yet would not be so bad, methinks.  And then a final series could
update the built-in commands' usage of hooks and would still be fairly
small.

In other words, I think I like (1), except *without* the
recommendation for users to edit their hook scripts to call "git hook
run" --- instead, the recommendation would be "try running this
command if you want to see what hooks will do in the future".

Thanks,
Jonathan
Emily Shaffer Oct. 6, 2020, 7:08 p.m. UTC | #6
On Mon, Oct 05, 2020 at 04:48:39PM -0700, Jonathan Nieder wrote:
> 
> Emily Shaffer wrote:
> > On Wed, Sep 23, 2020 at 04:47:34PM -0700, Jonathan Tan wrote:
> 
> >>  - As for whether (1) it is OK for none of the hooks to be converted (and
> >>    instead rely on the user to edit their hook scripts to call "git hook
> >>    run ???"), or if (2) we should require some hooks to be
> >>    converted, or if (3) we should require all hooks to be converted: I'd
> >>    rather have (2) or (3) so that we don't have dead code. I prefer (3),
> >>    especially since a reviewer wouldn't have to worry about leftover
> >>    usages of old functions like find_hook() (as I mentioned at the start
> >>    of this email), but I'm not fully opposed to (2) either.
> >
> > I personally prefer (3) - I think the user experience with (2) in a
> > release (or even in 'next', which all Googlers use) is pretty bad. The
> > downside, of course, is that a large topic gets merged all at once and
> > makes some pretty nasty reviewer overhead.
> 
> One approach is to build up a series with "git hook run" and "git hook
> list" demonstrating and testing the functionality and [PATCH n+1/n]
> extra patches at the end converting existing hooks.  The user
> experience from "git hook run" and even "git hook list" supporting a
> preview of the future without built-in commands living in that future
> yet would not be so bad, methinks.  And then a final series could
> update the built-in commands' usage of hooks and would still be fairly
> small.
> 
> In other words, I think I like (1), except *without* the
> recommendation for users to edit their hook scripts to call "git hook
> run" --- instead, the recommendation would be "try running this
> command if you want to see what hooks will do in the future".

Ok. I'll fix up the wording in the design doc and follow through with my
plan to split the series into two parts.

 - Emily
diff mbox series

Patch

diff --git a/builtin/commit.c b/builtin/commit.c
index 69ac78d5e5..a19c6478eb 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -36,6 +36,7 @@ 
 #include "help.h"
 #include "commit-reach.h"
 #include "commit-graph.h"
+#include "hook.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [<options>] [--] <pathspec>..."),
@@ -985,7 +986,7 @@  static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	if (!no_verify && find_hook("pre-commit")) {
+	if (!no_verify && hook_exists("pre-commit")) {
 		/*
 		 * 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
diff --git a/builtin/merge.c b/builtin/merge.c
index 74829a838e..c1a9d0083d 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -41,6 +41,7 @@ 
 #include "commit-reach.h"
 #include "wt-status.h"
 #include "commit-graph.h"
+#include "hook.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -829,7 +830,7 @@  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 (find_hook("pre-merge-commit"))
+	if (hook_exists("pre-merge-commit"))
 		discard_cache();
 	read_cache_from(index_file);
 	strbuf_addbuf(&msg, &merge_msg);
diff --git a/commit.c b/commit.c
index 4ce8cb38d5..c7a243e848 100644
--- a/commit.c
+++ b/commit.c
@@ -21,6 +21,7 @@ 
 #include "commit-reach.h"
 #include "run-command.h"
 #include "shallow.h"
+#include "hook.h"
 
 static struct commit_extra_header *read_commit_extra_header_lines(const char *buf, size_t len, const char **);
 
@@ -1632,8 +1633,13 @@  int run_commit_hook(int editor_is_used, const char *index_file,
 {
 	struct strvec hook_env = STRVEC_INIT;
 	va_list args;
+	const char *arg;
+	struct strvec hook_args = STRVEC_INIT;
+	struct strbuf hook_name = STRBUF_INIT;
 	int ret;
 
+	strbuf_addstr(&hook_name, name);
+
 	strvec_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
 
 	/*
@@ -1643,9 +1649,14 @@  int run_commit_hook(int editor_is_used, const char *index_file,
 		strvec_push(&hook_env, "GIT_EDITOR=:");
 
 	va_start(args, name);
-	ret = run_hook_ve(hook_env.v, name, args);
+	while ((arg = va_arg(args, const char *)))
+		strvec_push(&hook_args, arg);
 	va_end(args);
+
+	ret = run_hooks(hook_env.v, &hook_name, &hook_args);
 	strvec_clear(&hook_env);
+	strvec_clear(&hook_args);
+	strbuf_release(&hook_name);
 
 	return ret;
 }
diff --git a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
index b3485450a2..cef8085dcc 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -103,6 +103,19 @@  test_expect_success 'with succeeding hook' '
 	test_cmp expected_hooks actual_hooks
 '
 
+# NEEDSWORK: when 'git hook add' and 'git hook remove' have been added, use that
+# instead
+test_expect_success 'with succeeding hook (config-based)' '
+	test_when_finished "git config --unset hook.pre-commit.command success.sample" &&
+	test_when_finished "rm -f expected_hooks actual_hooks" &&
+	git config hook.pre-commit.command "$HOOKDIR/success.sample" &&
+	echo "$HOOKDIR/success.sample" >expected_hooks &&
+	echo "more" >>file &&
+	git add file &&
+	git commit -m "more" &&
+	test_cmp expected_hooks actual_hooks
+'
+
 test_expect_success 'with succeeding hook (merge)' '
 	test_when_finished "rm -f \"$PREMERGE\" expected_hooks actual_hooks" &&
 	cp "$HOOKDIR/success.sample" "$PREMERGE" &&