git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/5] hook API: support stdin, convert post-rewrite
@ 2023-01-23 17:15 Ævar Arnfjörð Bjarmason
  2023-01-23 17:15 ` [PATCH 1/5] run-command.c: remove dead assignment in while-loop Ævar Arnfjörð Bjarmason
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-23 17:15 UTC (permalink / raw)
  To: git
  Cc: Emily Shaffer, Junio C Hamano, Eric Sunshine, Felipe Contreras,
	Taylor Blau, Michael Strawbridge,
	Ævar Arnfjörð Bjarmason

The greater "config-based-hooks" topic has been stalled for a while.

In the last couple of cycles it was held up by run-command.c API
changes (which will make the rest of this much simpler), and lack of
time on my part.

But let's get the ball rolling again. The last time this was
on-list[1] it was part of a 36 patch series, these 5 patches only
convert one hook (implemented by both sequencer & git-am) to the hook
API, but it's an important step: To do so we need so support reading
from stdin, and passing that to the hooks.

The (passing) CI & topic branch for this is at[2].

The immediate motivation for this is to supply a "stdin" interface
that git-send-email.perl can use, see [3].

Changes since the [1] v5:

 * A new 1/5 here, picked from
   https://lore.kernel.org/git/patch-v2-07.22-b90961ae76d-20221012T084850Z-avarab@gmail.com/;
   A small while-at-it cleanup of a related API.

 * Updates for the aforementioned run-command API changes.

 * Commit message updates & clarifications.

 * The previous version of the "sequencer.c" change changed a variable
   name while at it, now it doesn't, making the diff much easier to
   read.

 * Updates for the *.txt and -h usage, so that we'll pass the
   since-added t0450 test.

1. https://lore.kernel.org/git/cover-v5-00.36-00000000000-20210902T125110Z-avarab@gmail.com/
2. https://github.com/avar/git/tree/es-avar/config-based-hooks-the-beginning
3. https://lore.kernel.org/git/230123.86wn5ds602.gmgdl@evledraar.gmail.com/

Emily Shaffer (4):
  run-command: allow stdin for run_processes_parallel
  hook API: support passing stdin to hooks, convert am's 'post-rewrite'
  sequencer: use the new hook API for the simpler "post-rewrite" call
  hook: support a --to-stdin=<path> option for testing

Ævar Arnfjörð Bjarmason (1):
  run-command.c: remove dead assignment in while-loop

 Documentation/git-hook.txt |  7 ++++++-
 builtin/am.c               | 20 ++++----------------
 builtin/hook.c             |  4 +++-
 hook.c                     |  8 +++++++-
 hook.h                     |  5 +++++
 run-command.c              | 13 +++++++++----
 sequencer.c                | 18 ++++--------------
 t/t1800-hook.sh            | 18 ++++++++++++++++++
 8 files changed, 56 insertions(+), 37 deletions(-)

Range-diff:
 1:  ac419613fdc <  -:  ----------- Makefile: mark "check" target as .PHONY
 2:  a161b7f0a5c <  -:  ----------- Makefile: stop hardcoding {command,config}-list.h
 3:  ffef1d3257e <  -:  ----------- Makefile: remove an out-of-date comment
 4:  545e16c6f04 <  -:  ----------- hook.[ch]: move find_hook() from run-command.c to hook.c
 5:  a9bc4519e9a <  -:  ----------- hook.c: add a hook_exists() wrapper and use it in bugreport.c
 6:  e99ec2e6f8f <  -:  ----------- hook.c users: use "hook_exists()" instead of "find_hook()"
 7:  2ffb2332c8a <  -:  ----------- hook-list.h: add a generated list of hooks, like config-list.h
 8:  72dd1010f5b <  -:  ----------- hook: add 'run' subcommand
 9:  821cc9bf11e <  -:  ----------- gc: use hook library for pre-auto-gc hook
10:  d71c90254ea <  -:  ----------- rebase: convert pre-rebase to use hook.h
11:  ea3af2ccc4d <  -:  ----------- am: convert applypatch to use hook.h
12:  fed0b52f88f <  -:  ----------- hooks: convert 'post-checkout' hook to hook library
13:  53d8721a0e3 <  -:  ----------- merge: convert post-merge to use hook.h
14:  d60827a2856 <  -:  ----------- git hook run: add an --ignore-missing flag
15:  d4976a0821f <  -:  ----------- send-email: use 'git hook run' for 'sendemail-validate'
16:  99f3dcd1945 <  -:  ----------- git-p4: use 'git hook' to run hooks
17:  509761454e6 <  -:  ----------- commit: convert {pre-commit,prepare-commit-msg} hook to hook.h
18:  e2c94d95427 <  -:  ----------- read-cache: convert post-index-change to use hook.h
19:  fa7d0d24ea2 <  -:  ----------- receive-pack: convert push-to-checkout hook to hook.h
20:  428bb5a6792 <  -:  ----------- run-command: remove old run_hook_{le,ve}() hook API
 -:  ----------- >  1:  351c6a55a41 run-command.c: remove dead assignment in while-loop
21:  994f6ad8602 !  2:  81eef2f60a0 run-command: allow stdin for run_processes_parallel
    @@ Commit message
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## run-command.c ##
    -@@ run-command.c: static int pp_start_one(struct parallel_processes *pp)
    - 	if (i == pp->max_processes)
    +@@ run-command.c: static int pp_start_one(struct parallel_processes *pp,
    + 	if (i == opts->processes)
      		BUG("bookkeeping is hard");
      
     +	/*
    @@ run-command.c: static int pp_start_one(struct parallel_processes *pp)
     +	 */
     +	pp->children[i].process.no_stdin = 1;
     +
    - 	code = pp->get_next_task(&pp->children[i].process,
    - 				 &pp->children[i].err,
    - 				 pp->data,
    -@@ run-command.c: static int pp_start_one(struct parallel_processes *pp)
    + 	code = opts->get_next_task(&pp->children[i].process,
    + 				   opts->ungroup ? NULL : &pp->children[i].err,
    + 				   opts->data,
    +@@ run-command.c: static int pp_start_one(struct parallel_processes *pp,
    + 		pp->children[i].process.err = -1;
    + 		pp->children[i].process.stdout_to_stderr = 1;
      	}
    - 	pp->children[i].process.err = -1;
    - 	pp->children[i].process.stdout_to_stderr = 1;
     -	pp->children[i].process.no_stdin = 1;
      
      	if (start_command(&pp->children[i].process)) {
    - 		code = pp->start_failure(&pp->children[i].err,
    + 		if (opts->start_failure)
23:  f548e3d15e7 !  3:  c6b9b69c516 am: convert 'post-rewrite' hook to hook.h
    @@ Metadata
     Author: Emily Shaffer <emilyshaffer@google.com>
     
      ## Commit message ##
    -    am: convert 'post-rewrite' hook to hook.h
    +    hook API: support passing stdin to hooks, convert am's 'post-rewrite'
    +
    +    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: static int run_applypatch_msg_hook(struct am_state *state)
     -
     -	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";
    -+	struct run_hooks_opt opt = RUN_HOOKS_OPT_INIT;
    - 
    +-
     -	ret = run_command(&cp);
    -+	strvec_push(&opt.args, "rebase");
    -+	opt.path_to_stdin = am_path(state, "rewritten");
    - 
    +-
     -	close(cp.in);
     -	return ret;
    -+	return run_hooks_oneshot("post-rewrite", &opt);
    ++	return run_hooks_opt("post-rewrite", &opt);
      }
      
      /**
    +
    + ## hook.c ##
    +@@ hook.c: 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;
    +
    + ## hook.h ##
    +@@ hook.h: 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 { \
 -:  ----------- >  4:  7a55c95f60f sequencer: use the new hook API for the simpler "post-rewrite" call
22:  3ccc654a664 !  5:  cb9ef7a89c4 hook: support passing stdin to hooks
    @@ Metadata
     Author: Emily Shaffer <emilyshaffer@google.com>
     
      ## Commit message ##
    -    hook: support passing stdin to hooks
    +    hook: support a --to-stdin=<path> option for testing
     
    -    Some hooks (such as post-rewrite) need to take input via stdin.
    -    Previously, callers provided stdin to hooks by setting
    -    run-command.h:child_process.in, which takes a FD. Callers would open the
    -    file in question themselves before calling run-command(). However, since
    -    we will now need to seek to the front of the file and read it again for
    -    every hook which runs, hook.h:run_command() takes a path and handles FD
    -    management itself. Since this file is opened for read only, it should
    -    not prevent later parallel execution support.
    -
    -    On the frontend, 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) and reading it back from the
    -    beginning to each hook. We'd want to support cases like insufficient
    -    memory or storage for the file. While this may prove useful later, for
    -    now the path of least resistance is to just ask the user to make this
    -    interim file themselves.
    +    Expose the "path_to_stdin" API added in the preceding commit in the
    +    "git hook run" command. For now we won't be using this command
    +    interface outside of the tests, but exposing this functionality makes
    +    it easier to test the hook API.
     
         Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Documentation/git-hook.txt ##
    -@@ Documentation/git-hook.txt: git-hook - run git hooks
    +@@ Documentation/git-hook.txt: git-hook - Run git hooks
      SYNOPSIS
      --------
      [verse]
     -'git hook' run [--ignore-missing] <hook-name> [-- <hook-args>]
    -+'git hook' run [--to-stdin=<path>] [--ignore-missing] <hook-name> [-- <hook-args>]
    ++'git hook' run [--ignore-missing] [--to-stdin=<path>] <hook-name> [-- <hook-args>]
      
      DESCRIPTION
      -----------
    -@@ Documentation/git-hook.txt: what those are.
    +@@ Documentation/git-hook.txt: linkgit:githooks[5] for arguments hooks might expect (if any).
      OPTIONS
      -------
      
    @@ builtin/hook.c
     @@ builtin/hook.c: static int run(int argc, const char **argv, const char *prefix)
      	struct option run_options[] = {
      		OPT_BOOL(0, "ignore-missing", &ignore_missing,
    - 			 N_("exit quietly with a zero exit code if the requested hook cannot be found")),
    + 			 N_("silently ignore missing requested <hook-name>")),
     +		OPT_STRING(0, "to-stdin", &opt.path_to_stdin, N_("path"),
     +			   N_("file to read into hooks' stdin")),
      		OPT_END(),
      	};
      	int ret;
     
    - ## hook.c ##
    -@@ hook.c: static int pick_next_hook(struct child_process *cp,
    - 	if (!run_me)
    - 		return 0;
    - 
    --	cp->no_stdin = 1;
    -+	/* 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->env = hook_cb->options->env.v;
    - 	cp->stdout_to_stderr = 1;
    - 	cp->trace2_hook_name = hook_cb->hook_name;
    -
    - ## hook.h ##
    -@@ hook.h: struct run_hooks_opt
    - 
    - 	/* Path to initial working directory for subprocess */
    - 	const char *dir;
    -+
    -+	/* Path to file which should be piped to stdin for each hook */
    -+	const char *path_to_stdin;
    - };
    - 
    - #define RUN_HOOKS_OPT_INIT { \
    -
      ## t/t1800-hook.sh ##
    -@@ t/t1800-hook.sh: test_expect_success 'git -c core.hooksPath=<PATH> hook run' '
    +@@ t/t1800-hook.sh: test_expect_success 'git hook run a hook with a bad shebang' '
      	test_cmp expect actual
      '
      
24:  bb119fa7cc0 <  -:  ----------- run-command: add stdin callback for parallelization
25:  2439f7752b8 <  -:  ----------- hook: provide stdin by string_list or callback
26:  48a380b3a91 <  -:  ----------- hook: convert 'post-rewrite' hook in sequencer.c to hook.h
27:  af6b9292aaa <  -:  ----------- transport: convert pre-push hook to hook.h
28:  957691f0b6d <  -:  ----------- hook tests: test for exact "pre-push" hook input
29:  88fe2621549 <  -:  ----------- hook tests: use a modern style for "pre-push" tests
30:  1d905e81779 <  -:  ----------- reference-transaction: use hook.h to run hooks
31:  fac56a9d8af <  -:  ----------- run-command: allow capturing of collated output
32:  7d185cdf9d1 <  -:  ----------- hooks: allow callers to capture output
33:  c8150e1239f <  -:  ----------- receive-pack: convert 'update' hook to hook.h
34:  a20ad847c14 <  -:  ----------- post-update: use hook.h library
35:  79c380be6ed <  -:  ----------- receive-pack: convert receive hooks to hook.h
36:  fe056098534 <  -:  ----------- hooks: fix a TOCTOU in "did we run a hook?" heuristic
-- 
2.39.1.1301.gffb37c08dee


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

* [PATCH 1/5] run-command.c: remove dead assignment in while-loop
  2023-01-23 17:15 [PATCH 0/5] hook API: support stdin, convert post-rewrite Ævar Arnfjörð Bjarmason
@ 2023-01-23 17:15 ` Ævar Arnfjörð Bjarmason
  2023-01-23 22:48   ` Junio C Hamano
  2023-01-23 17:15 ` [PATCH 2/5] run-command: allow stdin for run_processes_parallel Ævar Arnfjörð Bjarmason
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-23 17:15 UTC (permalink / raw)
  To: git
  Cc: Emily Shaffer, Junio C Hamano, Eric Sunshine, Felipe Contreras,
	Taylor Blau, Michael Strawbridge,
	Ævar Arnfjörð Bjarmason

Remove code that's been unused since it was added in
c553c72eed6 (run-command: add an asynchronous parallel child
processor, 2015-12-15), the next use of "i" in this function is:

	for (i = 0; ...

So we'll always clobber the "i" that's set here. Presumably the "i"
assignment is an artifact of WIP code that made it into our tree.

A subsequent commit will need to adjust the type of the "i" variable
in the otherwise unrelated for-loop, which is why this is being
removed now.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/run-command.c b/run-command.c
index 50cc011654e..b439c7974ca 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1632,9 +1632,7 @@ static void pp_buffer_stderr(struct parallel_processes *pp,
 			     const struct run_process_parallel_opts *opts,
 			     int output_timeout)
 {
-	int i;
-
-	while ((i = poll(pp->pfd, opts->processes, output_timeout) < 0)) {
+	while (poll(pp->pfd, opts->processes, output_timeout) < 0) {
 		if (errno == EINTR)
 			continue;
 		pp_cleanup(pp, opts);
-- 
2.39.1.1301.gffb37c08dee


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

* [PATCH 2/5] run-command: allow stdin for run_processes_parallel
  2023-01-23 17:15 [PATCH 0/5] hook API: support stdin, convert post-rewrite Ævar Arnfjörð Bjarmason
  2023-01-23 17:15 ` [PATCH 1/5] run-command.c: remove dead assignment in while-loop Ævar Arnfjörð Bjarmason
@ 2023-01-23 17:15 ` Ævar Arnfjörð Bjarmason
  2023-01-23 22:52   ` Junio C Hamano
  2023-01-23 17:15 ` [PATCH 3/5] hook API: support passing stdin to hooks, convert am's 'post-rewrite' Ævar Arnfjörð Bjarmason
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-23 17:15 UTC (permalink / raw)
  To: git
  Cc: Emily Shaffer, Junio C Hamano, Eric Sunshine, Felipe Contreras,
	Taylor Blau, Michael Strawbridge,
	Ævar Arnfjörð Bjarmason

From: Emily Shaffer <emilyshaffer@google.com>

While it makes sense not to inherit stdin from the parent process to
avoid deadlocking, it's not necessary to completely ban stdin to
children. An informed user should be able to configure stdin safely. By
setting `some_child.process.no_stdin=1` before calling `get_next_task()`
we provide a reasonable default behavior but enable users to set up
stdin streaming for themselves during the callback.

`some_child.process.stdout_to_stderr`, however, remains unmodifiable by
`get_next_task()` - the rest of the run_processes_parallel() API depends
on child output in stderr.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index b439c7974ca..6bd16acb060 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1586,6 +1586,14 @@ static int pp_start_one(struct parallel_processes *pp,
 	if (i == opts->processes)
 		BUG("bookkeeping is hard");
 
+	/*
+	 * By default, do not inherit stdin from the parent process - otherwise,
+	 * all children would share stdin! Users may overwrite this to provide
+	 * something to the child's stdin by having their 'get_next_task'
+	 * callback assign 0 to .no_stdin and an appropriate integer to .in.
+	 */
+	pp->children[i].process.no_stdin = 1;
+
 	code = opts->get_next_task(&pp->children[i].process,
 				   opts->ungroup ? NULL : &pp->children[i].err,
 				   opts->data,
@@ -1601,7 +1609,6 @@ static int pp_start_one(struct parallel_processes *pp,
 		pp->children[i].process.err = -1;
 		pp->children[i].process.stdout_to_stderr = 1;
 	}
-	pp->children[i].process.no_stdin = 1;
 
 	if (start_command(&pp->children[i].process)) {
 		if (opts->start_failure)
-- 
2.39.1.1301.gffb37c08dee


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

* [PATCH 3/5] hook API: support passing stdin to hooks, convert am's 'post-rewrite'
  2023-01-23 17:15 [PATCH 0/5] hook API: support stdin, convert post-rewrite Ævar Arnfjörð Bjarmason
  2023-01-23 17:15 ` [PATCH 1/5] run-command.c: remove dead assignment in while-loop Ævar Arnfjörð Bjarmason
  2023-01-23 17:15 ` [PATCH 2/5] run-command: allow stdin for run_processes_parallel Ævar Arnfjörð Bjarmason
@ 2023-01-23 17:15 ` Ævar Arnfjörð Bjarmason
  2023-01-23 23:10   ` Junio C Hamano
  2023-01-23 23:13   ` Junio C Hamano
  2023-01-23 17:15 ` [PATCH 4/5] sequencer: use the new hook API for the simpler "post-rewrite" call Ævar Arnfjörð Bjarmason
  2023-01-23 17:15 ` [PATCH 5/5] hook: support a --to-stdin=<path> option for testing Ævar Arnfjörð Bjarmason
  4 siblings, 2 replies; 14+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-23 17:15 UTC (permalink / raw)
  To: git
  Cc: Emily Shaffer, Junio C Hamano, Eric Sunshine, Felipe Contreras,
	Taylor Blau, Michael Strawbridge,
	Ævar Arnfjörð Bjarmason

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

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 { \
-- 
2.39.1.1301.gffb37c08dee


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

* [PATCH 4/5] sequencer: use the new hook API for the simpler "post-rewrite" call
  2023-01-23 17:15 [PATCH 0/5] hook API: support stdin, convert post-rewrite Ævar Arnfjörð Bjarmason
                   ` (2 preceding siblings ...)
  2023-01-23 17:15 ` [PATCH 3/5] hook API: support passing stdin to hooks, convert am's 'post-rewrite' Ævar Arnfjörð Bjarmason
@ 2023-01-23 17:15 ` Ævar Arnfjörð Bjarmason
  2023-01-24 14:46   ` Phillip Wood
  2023-01-23 17:15 ` [PATCH 5/5] hook: support a --to-stdin=<path> option for testing Ævar Arnfjörð Bjarmason
  4 siblings, 1 reply; 14+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-23 17:15 UTC (permalink / raw)
  To: git
  Cc: Emily Shaffer, Junio C Hamano, Eric Sunshine, Felipe Contreras,
	Taylor Blau, Michael Strawbridge,
	Ævar Arnfjörð Bjarmason

From: Emily Shaffer <emilyshaffer@google.com>

Change the invocation of the "post-rewrite" hook added in
795160457db (sequencer (rebase -i): run the post-rewrite hook, if
needed, 2017-01-02) to use the new hook API.

This leaves the more complex "post-rewrite" invocation added in
a87a6f3c98e (commit: move post-rewrite code to libgit, 2017-11-17)
here in sequencer.c unconverted. That'll be done in a subsequent
commit.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sequencer.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 3e4a1972897..d8d59d05dd4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -4834,8 +4834,7 @@ static int pick_commits(struct repository *r,
 		if (!stat(rebase_path_rewritten_list(), &st) &&
 				st.st_size > 0) {
 			struct child_process child = CHILD_PROCESS_INIT;
-			const char *post_rewrite_hook =
-				find_hook("post-rewrite");
+			struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT;
 
 			child.in = open(rebase_path_rewritten_list(), O_RDONLY);
 			child.git_cmd = 1;
@@ -4845,18 +4844,9 @@ static int pick_commits(struct repository *r,
 			/* we don't care if this copying failed */
 			run_command(&child);
 
-			if (post_rewrite_hook) {
-				struct child_process hook = CHILD_PROCESS_INIT;
-
-				hook.in = open(rebase_path_rewritten_list(),
-					O_RDONLY);
-				hook.stdout_to_stderr = 1;
-				hook.trace2_hook_name = "post-rewrite";
-				strvec_push(&hook.args, post_rewrite_hook);
-				strvec_push(&hook.args, "rebase");
-				/* we don't care if this hook failed */
-				run_command(&hook);
-			}
+			hook_opt.path_to_stdin = rebase_path_rewritten_list();
+			strvec_push(&hook_opt.args, "rebase");
+			run_hooks_opt("post-rewrite", &hook_opt);
 		}
 		apply_autostash(rebase_path_autostash());
 
-- 
2.39.1.1301.gffb37c08dee


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

* [PATCH 5/5] hook: support a --to-stdin=<path> option for testing
  2023-01-23 17:15 [PATCH 0/5] hook API: support stdin, convert post-rewrite Ævar Arnfjörð Bjarmason
                   ` (3 preceding siblings ...)
  2023-01-23 17:15 ` [PATCH 4/5] sequencer: use the new hook API for the simpler "post-rewrite" call Ævar Arnfjörð Bjarmason
@ 2023-01-23 17:15 ` Ævar Arnfjörð Bjarmason
  2023-01-24  0:55   ` Junio C Hamano
  4 siblings, 1 reply; 14+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2023-01-23 17:15 UTC (permalink / raw)
  To: git
  Cc: Emily Shaffer, Junio C Hamano, Eric Sunshine, Felipe Contreras,
	Taylor Blau, Michael Strawbridge,
	Ævar Arnfjörð Bjarmason

From: Emily Shaffer <emilyshaffer@google.com>

Expose the "path_to_stdin" API added in the preceding commit in the
"git hook run" command. For now we won't be using this command
interface outside of the tests, but exposing this functionality makes
it easier to test the hook API.

Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-hook.txt |  7 ++++++-
 builtin/hook.c             |  4 +++-
 t/t1800-hook.sh            | 18 ++++++++++++++++++
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-hook.txt b/Documentation/git-hook.txt
index 77c3a8ad909..3407f3c2c07 100644
--- a/Documentation/git-hook.txt
+++ b/Documentation/git-hook.txt
@@ -8,7 +8,7 @@ git-hook - Run git hooks
 SYNOPSIS
 --------
 [verse]
-'git hook' run [--ignore-missing] <hook-name> [-- <hook-args>]
+'git hook' run [--ignore-missing] [--to-stdin=<path>] <hook-name> [-- <hook-args>]
 
 DESCRIPTION
 -----------
@@ -31,6 +31,11 @@ linkgit:githooks[5] for arguments hooks might expect (if any).
 OPTIONS
 -------
 
+--to-stdin::
+	For "run"; Specify a file which will be streamed into the
+	hook's stdin. The hook will receive the entire file from
+	beginning to EOF.
+
 --ignore-missing::
 	Ignore any missing hook by quietly returning zero. Used for
 	tools that want to do a blind one-shot run of a hook that may
diff --git a/builtin/hook.c b/builtin/hook.c
index b6530d189ad..f95b7965c58 100644
--- a/builtin/hook.c
+++ b/builtin/hook.c
@@ -7,7 +7,7 @@
 #include "strvec.h"
 
 #define BUILTIN_HOOK_RUN_USAGE \
-	N_("git hook run [--ignore-missing] <hook-name> [-- <hook-args>]")
+	N_("git hook run [--ignore-missing] [--to-stdin=<path>] <hook-name> [-- <hook-args>]")
 
 static const char * const builtin_hook_usage[] = {
 	BUILTIN_HOOK_RUN_USAGE,
@@ -28,6 +28,8 @@ static int run(int argc, const char **argv, const char *prefix)
 	struct option run_options[] = {
 		OPT_BOOL(0, "ignore-missing", &ignore_missing,
 			 N_("silently ignore missing requested <hook-name>")),
+		OPT_STRING(0, "to-stdin", &opt.path_to_stdin, N_("path"),
+			   N_("file to read into hooks' stdin")),
 		OPT_END(),
 	};
 	int ret;
diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
index 2ef3579fa7c..3506f627b6c 100755
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -177,4 +177,22 @@ test_expect_success 'git hook run a hook with a bad shebang' '
 	test_cmp expect actual
 '
 
+test_expect_success 'stdin to hooks' '
+	write_script .git/hooks/test-hook <<-\EOF &&
+	echo BEGIN stdin
+	cat
+	echo END stdin
+	EOF
+
+	cat >expect <<-EOF &&
+	BEGIN stdin
+	hello
+	END stdin
+	EOF
+
+	echo hello >input &&
+	git hook run --to-stdin=input test-hook 2>actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.39.1.1301.gffb37c08dee


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

* Re: [PATCH 1/5] run-command.c: remove dead assignment in while-loop
  2023-01-23 17:15 ` [PATCH 1/5] run-command.c: remove dead assignment in while-loop Ævar Arnfjörð Bjarmason
@ 2023-01-23 22:48   ` Junio C Hamano
  0 siblings, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2023-01-23 22:48 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Emily Shaffer, Eric Sunshine, Felipe Contreras, Taylor Blau,
	Michael Strawbridge

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Remove code that's been unused since it was added in
> c553c72eed6 (run-command: add an asynchronous parallel child
> processor, 2015-12-15), the next use of "i" in this function is:
>
> 	for (i = 0; ...

And it has been updated to a different type, i.e.

	for (size_t i = 0; ...

so it doubly makes sense to kill that unused variable.

Makes sense.

> So we'll always clobber the "i" that's set here. Presumably the "i"
> assignment is an artifact of WIP code that made it into our tree.
>
> A subsequent commit will need to adjust the type of the "i" variable
> in the otherwise unrelated for-loop, which is why this is being
> removed now.

That, together with the earlier mention of the other i (which I
think came from the same source---perhaps the original topic this
was taken from had int->size_t change in it) are both stale.

Please proofread what you send out before submitting.

Thanks.


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

* Re: [PATCH 2/5] run-command: allow stdin for run_processes_parallel
  2023-01-23 17:15 ` [PATCH 2/5] run-command: allow stdin for run_processes_parallel Ævar Arnfjörð Bjarmason
@ 2023-01-23 22:52   ` Junio C Hamano
  0 siblings, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2023-01-23 22:52 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Emily Shaffer, Eric Sunshine, Felipe Contreras, Taylor Blau,
	Michael Strawbridge

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> +	/*
> +	 * By default, do not inherit stdin from the parent process - otherwise,
> +	 * all children would share stdin! Users may overwrite this to provide
> +	 * something to the child's stdin by having their 'get_next_task'
> +	 * callback assign 0 to .no_stdin and an appropriate integer to .in.
> +	 */
> +	pp->children[i].process.no_stdin = 1;
> +
>  	code = opts->get_next_task(&pp->children[i].process,
>  				   opts->ungroup ? NULL : &pp->children[i].err,
>  				   opts->data,
> @@ -1601,7 +1609,6 @@ static int pp_start_one(struct parallel_processes *pp,
>  		pp->children[i].process.err = -1;
>  		pp->children[i].process.stdout_to_stderr = 1;
>  	}
> -	pp->children[i].process.no_stdin = 1;

For this single process, by default .no_stdin is set before it is
passed to start_command(), so the default behaviour does not change.

This needs a new safety to ensure that processes that have .no_stdin
turned off do not share the same value in their .in member, doesn't
it?  Hopefully that will be added in a later step in the series?

Provided that such a safety will appear in the end result, this
conversion does make sense to me.  Let's read on.

>  	if (start_command(&pp->children[i].process)) {
>  		if (opts->start_failure)

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

* Re: [PATCH 3/5] hook API: support passing stdin to hooks, convert am's 'post-rewrite'
  2023-01-23 17:15 ` [PATCH 3/5] hook API: support passing stdin to hooks, convert am's 'post-rewrite' Ævar Arnfjörð Bjarmason
@ 2023-01-23 23:10   ` Junio C Hamano
  2023-01-23 23:13   ` Junio C Hamano
  1 sibling, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2023-01-23 23:10 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Emily Shaffer, Eric Sunshine, Felipe Contreras, Taylor Blau,
	Michael Strawbridge

Æ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 { \

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

* Re: [PATCH 3/5] hook API: support passing stdin to hooks, convert am's 'post-rewrite'
  2023-01-23 17:15 ` [PATCH 3/5] hook API: support passing stdin to hooks, convert am's 'post-rewrite' Ævar Arnfjörð Bjarmason
  2023-01-23 23:10   ` Junio C Hamano
@ 2023-01-23 23:13   ` Junio C Hamano
  1 sibling, 0 replies; 14+ messages in thread
From: Junio C Hamano @ 2023-01-23 23:13 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Emily Shaffer, Eric Sunshine, Felipe Contreras, Taylor Blau,
	Michael Strawbridge

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

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

* Re: [PATCH 5/5] hook: support a --to-stdin=<path> option for testing
  2023-01-23 17:15 ` [PATCH 5/5] hook: support a --to-stdin=<path> option for testing Ævar Arnfjörð Bjarmason
@ 2023-01-24  0:55   ` Junio C Hamano
  2023-01-24  0:59     ` Michael Strawbridge
  0 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2023-01-24  0:55 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Emily Shaffer, Eric Sunshine, Felipe Contreras, Taylor Blau,
	Michael Strawbridge

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> From: Emily Shaffer <emilyshaffer@google.com>
>
> Expose the "path_to_stdin" API added in the preceding commit in the
> "git hook run" command. For now we won't be using this command
> interface outside of the tests, but exposing this functionality makes
> it easier to test the hook API.

Presumably, the send-email validation topic would be using this
immediately once it becomes available, no?

When "git hook" finds and runs more than one hook script, do they
get the same input?  What I am wondering is if "to-stdin" should be
exposed like this interface (which may be sufficient for testing
purposes).  I imagine that scripters (e.g. send-email developers)
would find it more convenient if they do not have to come up with a
temporary file and they can just run "git hook" and feed whatever
they want to give to the hook from its standard input.  "git hook"
command, upon startup, should do the reading of its standard input
and spooling it to a temporary file it uses to pass the contents to
the hook scripts, in other words.

Other than that, it surely looks not just handy for tests, but has
immediate uses.

Thanks.

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

* Re: [PATCH 5/5] hook: support a --to-stdin=<path> option for testing
  2023-01-24  0:55   ` Junio C Hamano
@ 2023-01-24  0:59     ` Michael Strawbridge
  0 siblings, 0 replies; 14+ messages in thread
From: Michael Strawbridge @ 2023-01-24  0:59 UTC (permalink / raw)
  To: Junio C Hamano, Ævar Arnfjörð Bjarmason
  Cc: git, Emily Shaffer, Eric Sunshine, Felipe Contreras, Taylor Blau


On 2023-01-23 19:55, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> From: Emily Shaffer <emilyshaffer@google.com>
>>
>> Expose the "path_to_stdin" API added in the preceding commit in the
>> "git hook run" command. For now we won't be using this command
>> interface outside of the tests, but exposing this functionality makes
>> it easier to test the hook API.
> Presumably, the send-email validation topic would be using this
> immediately once it becomes available, no?

Yes I would be trying to use it for my send-email header patch right away.

>
> When "git hook" finds and runs more than one hook script, do they
> get the same input?  What I am wondering is if "to-stdin" should be
> exposed like this interface (which may be sufficient for testing
> purposes).  I imagine that scripters (e.g. send-email developers)
> would find it more convenient if they do not have to come up with a
> temporary file and they can just run "git hook" and feed whatever
> they want to give to the hook from its standard input.  "git hook"
> command, upon startup, should do the reading of its standard input
> and spooling it to a temporary file it uses to pass the contents to
> the hook scripts, in other words.
>
> Other than that, it surely looks not just handy for tests, but has
> immediate uses.
>
> Thanks.

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

* Re: [PATCH 4/5] sequencer: use the new hook API for the simpler "post-rewrite" call
  2023-01-23 17:15 ` [PATCH 4/5] sequencer: use the new hook API for the simpler "post-rewrite" call Ævar Arnfjörð Bjarmason
@ 2023-01-24 14:46   ` Phillip Wood
  2023-01-27 15:08     ` Phillip Wood
  0 siblings, 1 reply; 14+ messages in thread
From: Phillip Wood @ 2023-01-24 14:46 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Emily Shaffer, Junio C Hamano, Eric Sunshine, Felipe Contreras,
	Taylor Blau, Michael Strawbridge

Hi Ævar

On 23/01/2023 17:15, Ævar Arnfjörð Bjarmason wrote:
> From: Emily Shaffer <emilyshaffer@google.com>
> 
> Change the invocation of the "post-rewrite" hook added in
> 795160457db (sequencer (rebase -i): run the post-rewrite hook, if
> needed, 2017-01-02) to use the new hook API.
> 
> This leaves the more complex "post-rewrite" invocation added in
> a87a6f3c98e (commit: move post-rewrite code to libgit, 2017-11-17)
> here in sequencer.c unconverted. That'll be done in a subsequent
> commit.

As a reader I'd find it more helpful to explain why the conversion isn't 
done here rather than leaving be to run "git show" to figure it out. If 
you re-roll perhaps we could replace the commit citation with something like

sequencer.c also contains an invocation of the "post-rewrite" hook in 
run_rewrite_hook() that is not converted as the hook API does not allow 
us to pass the hook input as a string yet.

Best Wishes

Phillip

> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   sequencer.c | 18 ++++--------------
>   1 file changed, 4 insertions(+), 14 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 3e4a1972897..d8d59d05dd4 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4834,8 +4834,7 @@ static int pick_commits(struct repository *r,
>   		if (!stat(rebase_path_rewritten_list(), &st) &&
>   				st.st_size > 0) {
>   			struct child_process child = CHILD_PROCESS_INIT;
> -			const char *post_rewrite_hook =
> -				find_hook("post-rewrite");
> +			struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT;
>   
>   			child.in = open(rebase_path_rewritten_list(), O_RDONLY);
>   			child.git_cmd = 1;
> @@ -4845,18 +4844,9 @@ static int pick_commits(struct repository *r,
>   			/* we don't care if this copying failed */
>   			run_command(&child);
>   
> -			if (post_rewrite_hook) {
> -				struct child_process hook = CHILD_PROCESS_INIT;
> -
> -				hook.in = open(rebase_path_rewritten_list(),
> -					O_RDONLY);
> -				hook.stdout_to_stderr = 1;
> -				hook.trace2_hook_name = "post-rewrite";
> -				strvec_push(&hook.args, post_rewrite_hook);
> -				strvec_push(&hook.args, "rebase");
> -				/* we don't care if this hook failed */
> -				run_command(&hook);
> -			}
> +			hook_opt.path_to_stdin = rebase_path_rewritten_list();
> +			strvec_push(&hook_opt.args, "rebase");
> +			run_hooks_opt("post-rewrite", &hook_opt);
>   		}
>   		apply_autostash(rebase_path_autostash());
>   

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

* Re: [PATCH 4/5] sequencer: use the new hook API for the simpler "post-rewrite" call
  2023-01-24 14:46   ` Phillip Wood
@ 2023-01-27 15:08     ` Phillip Wood
  0 siblings, 0 replies; 14+ messages in thread
From: Phillip Wood @ 2023-01-27 15:08 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Emily Shaffer, Junio C Hamano, Eric Sunshine, Felipe Contreras,
	Taylor Blau, Michael Strawbridge

Hi Ævar

On 24/01/2023 14:46, Phillip Wood wrote:
> Hi Ævar
> 
> On 23/01/2023 17:15, Ævar Arnfjörð Bjarmason wrote:
>> From: Emily Shaffer <emilyshaffer@google.com>
>>
>> Change the invocation of the "post-rewrite" hook added in
>> 795160457db (sequencer (rebase -i): run the post-rewrite hook, if
>> needed, 2017-01-02) to use the new hook API.
>>
>> This leaves the more complex "post-rewrite" invocation added in
>> a87a6f3c98e (commit: move post-rewrite code to libgit, 2017-11-17)
>> here in sequencer.c unconverted. That'll be done in a subsequent
>> commit.
> 
> As a reader I'd find it more helpful to explain why the conversion isn't 
> done here rather than leaving be to run "git show" to figure it out. If 
> you re-roll perhaps we could replace the commit citation with something 
> like
> 
> sequencer.c also contains an invocation of the "post-rewrite" hook in 
> run_rewrite_hook() that is not converted as the hook API does not allow 
> us to pass the hook input as a string yet.

Sorry, I forgot to say in my previous reply that I like the code change 
here - it is a nice simplification for callers. builtin/am.c has a 
similar function to the one that is converted here.

Best Wishes

Phillip

> Best Wishes
> 
> Phillip
> 
>> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>   sequencer.c | 18 ++++--------------
>>   1 file changed, 4 insertions(+), 14 deletions(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index 3e4a1972897..d8d59d05dd4 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -4834,8 +4834,7 @@ static int pick_commits(struct repository *r,
>>           if (!stat(rebase_path_rewritten_list(), &st) &&
>>                   st.st_size > 0) {
>>               struct child_process child = CHILD_PROCESS_INIT;
>> -            const char *post_rewrite_hook =
>> -                find_hook("post-rewrite");
>> +            struct run_hooks_opt hook_opt = RUN_HOOKS_OPT_INIT;
>>               child.in = open(rebase_path_rewritten_list(), O_RDONLY);
>>               child.git_cmd = 1;
>> @@ -4845,18 +4844,9 @@ static int pick_commits(struct repository *r,
>>               /* we don't care if this copying failed */
>>               run_command(&child);
>> -            if (post_rewrite_hook) {
>> -                struct child_process hook = CHILD_PROCESS_INIT;
>> -
>> -                hook.in = open(rebase_path_rewritten_list(),
>> -                    O_RDONLY);
>> -                hook.stdout_to_stderr = 1;
>> -                hook.trace2_hook_name = "post-rewrite";
>> -                strvec_push(&hook.args, post_rewrite_hook);
>> -                strvec_push(&hook.args, "rebase");
>> -                /* we don't care if this hook failed */
>> -                run_command(&hook);
>> -            }
>> +            hook_opt.path_to_stdin = rebase_path_rewritten_list();
>> +            strvec_push(&hook_opt.args, "rebase");
>> +            run_hooks_opt("post-rewrite", &hook_opt);
>>           }
>>           apply_autostash(rebase_path_autostash());

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

end of thread, other threads:[~2023-01-27 15:08 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-23 17:15 [PATCH 0/5] hook API: support stdin, convert post-rewrite Ævar Arnfjörð Bjarmason
2023-01-23 17:15 ` [PATCH 1/5] run-command.c: remove dead assignment in while-loop Ævar Arnfjörð Bjarmason
2023-01-23 22:48   ` Junio C Hamano
2023-01-23 17:15 ` [PATCH 2/5] run-command: allow stdin for run_processes_parallel Ævar Arnfjörð Bjarmason
2023-01-23 22:52   ` Junio C Hamano
2023-01-23 17:15 ` [PATCH 3/5] hook API: support passing stdin to hooks, convert am's 'post-rewrite' Ævar Arnfjörð Bjarmason
2023-01-23 23:10   ` Junio C Hamano
2023-01-23 23:13   ` Junio C Hamano
2023-01-23 17:15 ` [PATCH 4/5] sequencer: use the new hook API for the simpler "post-rewrite" call Ævar Arnfjörð Bjarmason
2023-01-24 14:46   ` Phillip Wood
2023-01-27 15:08     ` Phillip Wood
2023-01-23 17:15 ` [PATCH 5/5] hook: support a --to-stdin=<path> option for testing Ævar Arnfjörð Bjarmason
2023-01-24  0:55   ` Junio C Hamano
2023-01-24  0:59     ` Michael Strawbridge

Code repositories for project(s) associated with this public inbox

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).