git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [PATCH] commit: use config-based hooks (config-based hooks part II)
@ 2020-10-14 23:25 Emily Shaffer
  2020-10-16 18:34 ` Junio C Hamano
  0 siblings, 1 reply; 2+ messages in thread
From: Emily Shaffer @ 2020-10-14 23:25 UTC (permalink / raw)
  To: git; +Cc: Emily Shaffer

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>
---

This is based on es/config-based-hooks (v5).

Since config-based hooks v4, I split a new branch for the conversions of
existing hook callers. I'm hoping this will make it easier for me to
work on the architecture without needing to juggle these
hopefully-slow-moving patches as frequently, and will reduce the load on
reviewers.

Since v4 also I removed the second patch changing the run_commit_hook()
API. I had figured it might not be well received when I sent it, and it
wasn't, so no worries.

I'm hoping to have updates to this series pretty soon too with new
conversions... so I guess this one may just be noise. Ah well.

 - Emily

 builtin/commit.c                                |  3 ++-
 builtin/merge.c                                 |  3 ++-
 commit.c                                        | 10 +++++++++-
 ...503-pre-commit-and-pre-merge-commit-hooks.sh | 17 +++++++++++++++--
 4 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1dfd799ec5..a337ecd4c2 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>..."),
@@ -983,7 +984,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", configured_hookdir_opt())) {
 		/*
 		 * 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 9d5359edc2..36ba138f4e 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", configured_hookdir_opt()))
 		discard_cache();
 	read_cache_from(index_file);
 	strbuf_addbuf(&msg, &merge_msg);
diff --git a/commit.c b/commit.c
index f53429c0ac..b338f50103 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 **);
 
@@ -1635,8 +1636,11 @@ 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;
 	int ret;
 
+
 	strvec_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
 
 	/*
@@ -1646,9 +1650,13 @@ 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, name, &hook_args, configured_hookdir_opt());
 	strvec_clear(&hook_env);
+	strvec_clear(&hook_args);
 
 	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..fc93bc3d23 100755
--- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
+++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
@@ -5,8 +5,8 @@ test_description='pre-commit and pre-merge-commit hooks'
 . ./test-lib.sh
 
 HOOKDIR="$(git rev-parse --git-dir)/hooks"
-PRECOMMIT="$HOOKDIR/pre-commit"
-PREMERGE="$HOOKDIR/pre-merge-commit"
+PRECOMMIT="$(pwd)/$HOOKDIR/pre-commit"
+PREMERGE="$(pwd)/$HOOKDIR/pre-merge-commit"
 
 # Prepare sample scripts that write their $0 to actual_hooks
 test_expect_success 'sample script setup' '
@@ -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" &&
-- 
2.28.0.rc0.142.g3c755180ce-goog


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] commit: use config-based hooks (config-based hooks part II)
  2020-10-14 23:25 [PATCH] commit: use config-based hooks (config-based hooks part II) Emily Shaffer
@ 2020-10-16 18:34 ` Junio C Hamano
  0 siblings, 0 replies; 2+ messages in thread
From: Junio C Hamano @ 2020-10-16 18:34 UTC (permalink / raw)
  To: Emily Shaffer; +Cc: git

Emily Shaffer <emilyshaffer@google.com> writes:

> @@ -983,7 +984,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", configured_hookdir_opt())) {

So, the migration from find_hook(), which is the traditional "only
on the filesystem in $GIT_DIR/hooks", to hook_exists(), which knows
the ones defined in the configuration files, is the same as the
previous round's.  Understood.

> diff --git a/commit.c b/commit.c
> index f53429c0ac..b338f50103 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 **);
>  
> @@ -1635,8 +1636,11 @@ 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;
>  	int ret;
>  
> +
>  	strvec_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);

Noise.

> @@ -1646,9 +1650,13 @@ 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, name, &hook_args, configured_hookdir_opt());

And this is the calling convention change.  Generally speaking,
run_hook_ve() and friends (does it still have friends?) are on their
way out, and run_hooks() will be the only thing we need to learn in
the future.

> 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..fc93bc3d23 100755
> --- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
> +++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh
> @@ -5,8 +5,8 @@ test_description='pre-commit and pre-merge-commit hooks'
>  . ./test-lib.sh
>  
>  HOOKDIR="$(git rev-parse --git-dir)/hooks"
> -PRECOMMIT="$HOOKDIR/pre-commit"
> -PREMERGE="$HOOKDIR/pre-merge-commit"
> +PRECOMMIT="$(pwd)/$HOOKDIR/pre-commit"
> +PREMERGE="$(pwd)/$HOOKDIR/pre-merge-commit"

What makes this change necessary?  Are we going to chdir around or
something?  Isn't the configuration file the ONLY thing that needs
to know their full location?

Should and does hook.*.command configuration honor ~ expansion?  I
think our $TRASH_DIRECTORY is our $HOME during tests, so 

	git config hook.foo.command "~/.git/hooks/success.sample"

might be a worthy thing to test.  Also, do we have a clear
definition on what happens to a relative pathname specified for the
hook commands, or is it left "undefined--do not do that"?  If the
former, that would also be worth testing.  I'd imagine that majority
of hooks defined in ~/.gitconfig WILL point at full path so testing
relative path may not matter too much for that particular use case,
but for ones defined in .git/config, I suspect it would be most
natural to take it as relative to the root of the working tree.

It may be a good change in the longer term, but it felt surprising
to see such a change that would affect 103-5=98 lines of existing
tests was made centrally here, without any explanation in the
proposed log message.

Thanks.

>  # Prepare sample scripts that write their $0 to actual_hooks
>  test_expect_success 'sample script setup' '
> @@ -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" &&

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2020-10-16 18:34 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-14 23:25 [PATCH] commit: use config-based hooks (config-based hooks part II) Emily Shaffer
2020-10-16 18:34 ` Junio C Hamano

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git