diff mbox series

[3/5] hook API: support passing stdin to hooks, convert am's 'post-rewrite'

Message ID patch-3.5-c6b9b69c516-20230123T170551Z-avarab@gmail.com (mailing list archive)
State Superseded
Headers show
Series hook API: support stdin, convert post-rewrite | expand

Commit Message

Ævar Arnfjörð Bjarmason Jan. 23, 2023, 5:15 p.m. UTC 
From: Emily Shaffer <emilyshaffer@google.com>

Convert the invocation of the 'post-rewrite' hook run by 'git am' to
use the hook.h library. To do this we need to add a "path_to_stdin"
member to "struct run_hooks_opt".

In our API this is supported by asking for a file path, rather
than by reading stdin. Reading directly from stdin would involve caching
the entire stdin (to memory or to disk) once the hook API is made to
support "jobs" larger than 1, along with support for executing N hooks
at a time (i.e. the upcoming config-based hooks).

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/am.c | 20 ++++----------------
 hook.c       |  8 +++++++-
 hook.h       |  5 +++++
 3 files changed, 16 insertions(+), 17 deletions(-)

Comments

Junio C Hamano Jan. 23, 2023, 11:10 p.m. UTC | #1 
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/hook.c b/hook.c
> index a4fa1031f28..86c6dc1fe70 100644
> --- a/hook.c
> +++ b/hook.c
> @@ -53,8 +53,14 @@ static int pick_next_hook(struct child_process *cp,
>  	if (!hook_path)
>  		return 0;
>  
> -	cp->no_stdin = 1;
>  	strvec_pushv(&cp->env, hook_cb->options->env.v);
> +	/* reopen the file for stdin; run_command closes it. */
> +	if (hook_cb->options->path_to_stdin) {
> +		cp->no_stdin = 0;
> +		cp->in = xopen(hook_cb->options->path_to_stdin, O_RDONLY);
> +	} else {
> +		cp->no_stdin = 1;
> +	}

Do we need this else clause?  I thought that we've made sure
no_stdin is the default.  Is it just being explicit?

> diff --git a/hook.h b/hook.h
> index 4258b13da0d..19ab9a5806e 100644
> --- a/hook.h
> +++ b/hook.h
> @@ -30,6 +30,11 @@ struct run_hooks_opt
>  	 * was invoked.
>  	 */
>  	int *invoked_hook;
> +
> +	/**
> +	 * Path to file which should be piped to stdin for each hook.
> +	 */
> +	const char *path_to_stdin;
>  };
>  
>  #define RUN_HOOKS_OPT_INIT { \
Junio C Hamano Jan. 23, 2023, 11:13 p.m. UTC | #2 
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> @@ -53,8 +53,14 @@ static int pick_next_hook(struct child_process *cp,
>  	if (!hook_path)
>  		return 0;
>  
> -	cp->no_stdin = 1;
>  	strvec_pushv(&cp->env, hook_cb->options->env.v);
> +	/* reopen the file for stdin; run_command closes it. */
> +	if (hook_cb->options->path_to_stdin) {
> +		cp->no_stdin = 0;
> +		cp->in = xopen(hook_cb->options->path_to_stdin, O_RDONLY);
> +	} else {
> +		cp->no_stdin = 1;
> +	}

By the way, using the path_to_stdin as the customization machinery
for the API users, and keeping it to the API implementation to
actually open the file and stuff .in member with it, is a good way
to make sure that multiple processes do not compete for the same
standard input stream.  IOW, what I was worried about in my review
of [2/5] is addressed by this mechanism.

Thanks.
diff mbox series

Patch

diff --git a/builtin/am.c b/builtin/am.c
index 82a41cbfc4e..8be91617fef 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -495,24 +495,12 @@  static int run_applypatch_msg_hook(struct am_state *state)
  */
 static int run_post_rewrite_hook(const struct am_state *state)
 {
-	struct child_process cp = CHILD_PROCESS_INIT;
-	const char *hook = find_hook("post-rewrite");
-	int ret;
-
-	if (!hook)
-		return 0;
+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
 
-	strvec_push(&cp.args, hook);
-	strvec_push(&cp.args, "rebase");
+	strvec_push(&opt.args, "rebase");
+	opt.path_to_stdin = am_path(state, "rewritten");
 
-	cp.in = xopen(am_path(state, "rewritten"), O_RDONLY);
-	cp.stdout_to_stderr = 1;
-	cp.trace2_hook_name = "post-rewrite";
-
-	ret = run_command(&cp);
-
-	close(cp.in);
-	return ret;
+	return run_hooks_opt("post-rewrite", &opt);
 }
 
 /**
diff --git a/hook.c b/hook.c
index a4fa1031f28..86c6dc1fe70 100644
--- a/hook.c
+++ b/hook.c
@@ -53,8 +53,14 @@  static int pick_next_hook(struct child_process *cp,
 	if (!hook_path)
 		return 0;
 
-	cp->no_stdin = 1;
 	strvec_pushv(&cp->env, hook_cb->options->env.v);
+	/* reopen the file for stdin; run_command closes it. */
+	if (hook_cb->options->path_to_stdin) {
+		cp->no_stdin = 0;
+		cp->in = xopen(hook_cb->options->path_to_stdin, O_RDONLY);
+	} else {
+		cp->no_stdin = 1;
+	}
 	cp->stdout_to_stderr = 1;
 	cp->trace2_hook_name = hook_cb->hook_name;
 	cp->dir = hook_cb->options->dir;
diff --git a/hook.h b/hook.h
index 4258b13da0d..19ab9a5806e 100644
--- a/hook.h
+++ b/hook.h
@@ -30,6 +30,11 @@  struct run_hooks_opt
 	 * was invoked.
 	 */
 	int *invoked_hook;
+
+	/**
+	 * Path to file which should be piped to stdin for each hook.
+	 */
+	const char *path_to_stdin;
 };
 
 #define RUN_HOOKS_OPT_INIT { \