| Message ID | pull.790.v3.git.1605801376577.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] hooks: allow input from stdin for commit-related hooks | expand |
"Orgad Shaneh via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Orgad Shaneh <orgads@gmail.com> > > Let hooks receive user input if applicable. > > Closing stdin originates in f5bbc3225 (Port git commit to C, > 2007). Looks like the original shell implementation did have > stdin open. Not clear why the author chose to close it on > the C port (maybe copy&paste). Please drop unsubstanciated guess in the parentheses. If anything, we've learned during the discussion thread that it is a bad idea to leave the standard input open when spawning hooks in general, and to me it looks like a lot more plausible reason why we tightened the interface when it was made from "only to run hooks for 'git commit'" to an interface that more widely usable to run any hook. If we are not going to record that finding in the log message to help people to find out what we knew at the time when the commit was created, then we shouldn't mislead them with "maybe copy&paste" that is not backed by anything other than a hunch. > Allow stdin only for commit-related hooks. Some of the other > hooks pass their own input to the hook, so don't change them. Before this paragraph that gives orders to the code to "be like so", the log message needs to explain why it is a good idea to make such a change. Which hook benefits by being able to read the standard input? Describe what becomes possible in terms of end-user visible effects (i.e. "now reading standard input becomes possible for pre-commit hook" is *not* an answer. What new things a pre-commit hook that now can read from the standard input do for the end user?) to justify why such a change is a good thing to have, before this paragraph to justify why leaving the standard input open for hooks run by "git commit" is a good idea and is a safe thing to do. Note that even "git commit" may compete for its standard input with hooks. "git commit -F - <message" currently may read the message to EOF before doing anything interesting like spawning a hook, but it is not implausible that the reading of the message may want to happen much later in a future codebase, at which point the hook may end up stealing the beginning of the message by reading from the standard input. So ideally, if we can find a way to selectively close the standard input for the hooks if "git commit" itself uses the standard input, that would be better than unconditionally leaving it open. Let's reorder the patch hunks to see the bottom layer first, as the callers are mostly the same. > diff --git a/run-command.h b/run-command.h > index 6472b38bde..e6a850c6fe 100644 > --- a/run-command.h > +++ b/run-command.h > @@ -201,11 +201,16 @@ int run_command(struct child_process *); > */ > const char *find_hook(const char *name); > > +#define RUN_HOOK_ALLOW_STDIN 1 > + > /** > * Run a hook. > - * The first argument is a pathname to an index file, or NULL > - * if the hook uses the default index file or no index is needed. > - * The second argument is the name of the hook. > + * The first argument is an array of environment variables, or NULL > + * if the hook uses the default environment and doesn't require > + * additional variables. > + * The second argument is zero or RUN_HOOK_ALLOW_STDIN, which enables > + * stdin for the child process (the default is no_stdin). > + * The third argument is the name of the hook. > * The further arguments correspond to the hook arguments. > * The last argument has to be NULL to terminate the arguments list. > * If the hook does not exist or is not executable, the return > @@ -215,8 +220,8 @@ const char *find_hook(const char *name); > * On execution, .stdout_to_stderr and .no_stdin will be set. > */ > LAST_ARG_MUST_BE_NULL > -int run_hook_le(const char *const *env, const char *name, ...); > -int run_hook_ve(const char *const *env, const char *name, va_list args); > +int run_hook_le(const char *const *env, int opt, const char *name, ...); > +int run_hook_ve(const char *const *env, int opt, const char *name, va_list args); Is this new parameter meant to be used as an enum? When the run_hook interface gets extended the next time and we want a new option, is the option expected to be mutually incompatible with allow-stdin? I suspect that it would make this a more useful API if this new parameter is not used as an enum but as a collection of flag bits. If so, a few things must change in the above: - The description of the second parameter in the comment shouldn't say "zero or RUN_HOOK_ALLOW_STDIN"; it should rather say "an OR'ed collection of feature bits like RUN_HOOK_ALLOW_STDIN defined above" - The second parameter should be 'unsigned flags', not 'int opt'. It is my understanding that "git commit" only needs run_hook_ve() to drive its hook scripts. Isn't it premature to touch run_hook_le(), in which nobody wants to leave the standard input open while running hooks? It _might_ be a better idea to allow users of _le() to do the same eventually, but then perhaps it is a good idea to do so in a separate step at the end, as "only to be complete" patch. That is, the structure of the topic ought to be something like: - [PATCH 1/2] add the "unsigned flags" word to _ve(), assign the RUN_HOOK_ALLOW_STDIN bit, and update commit.c::run_commit_hook() to pass RUN_HOOK_ALLOW_STDIN to it. - [PATCH 2/2] after surveying the options "git commit" takes, find out the condition where "git commit" itself would want to consume the standard input (e.g. "commit -F -", there may be others), and tell run_commit_hook() *not* to pass RUN_HOOK_ALLOW_STDIN when we use the standard input ourselves (i.e. forbid hooks to read from it). - [PATCH 3/2] add the same "unsigned flags" word to _le(), and teach all callers to pass 0, as a "just for completeness" step. Personally, I think we should stop at [2/2], and do not do [3/2], as there is no real demonstrated use of the standard input for hooks. Especially because users of the _le() interface includes programs like receive-pack whose standard input should not be molested, I'd feel safer not to see [3/2] done at all (for that matter, I'm not happy with [1/2] unless it comes with [2/2], either). > diff --git a/run-command.c b/run-command.c > index 2ee59acdc8..21b1f0a5e9 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -1343,7 +1343,7 @@ const char *find_hook(const char *name) > return path.buf; > } > > -int run_hook_ve(const char *const *env, const char *name, va_list args) > +int run_hook_ve(const char *const *env, int opt, const char *name, va_list args) > { > struct child_process hook = CHILD_PROCESS_INIT; > const char *p; > @@ -1356,20 +1356,21 @@ int run_hook_ve(const char *const *env, const char *name, va_list args) > while ((p = va_arg(args, const char *))) > strvec_push(&hook.args, p); > hook.env = env; > - hook.no_stdin = 1; > + if (!(opt & RUN_HOOK_ALLOW_STDIN)) > + hook.no_stdin = 1; OK, so you are using the parameter as a flag word after all. Then "int opt" should definitely be "unsigned flags". And these two lines would be more readable, when written like so: hook.no_stdin = !(flags & RUN_HOOK_ALLOW_STDIN); I would think. > hook.stdout_to_stderr = 1; > hook.trace2_hook_name = name; > > return run_command(&hook); > } > > -int run_hook_le(const char *const *env, const char *name, ...) > +int run_hook_le(const char *const *env, int opt, const char *name, ...) > { > va_list args; > int ret; > > va_start(args, name); > - ret = run_hook_ve(env, name, args); > + ret = run_hook_ve(env, opt, name, args); I'd rather not to see the function signature of _le() changed in patches [1/2] and [2/2]; instead we can just pass hardcoded 0 from here to the underlying _ve(). Now, what is left is individual commands that use the run_hook interface. > diff --git a/commit.c b/commit.c > index fe1fa3dc41..775019ec9d 100644 > --- a/commit.c > +++ b/commit.c > @@ -1646,7 +1646,7 @@ 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); > + ret = run_hook_ve(hook_env.v, RUN_HOOK_ALLOW_STDIN, name, args); > va_end(args); > strvec_clear(&hook_env); > This is good for [1/2]. We should avoid "git commit" from competing with hooks for its standard input by conditionally passing ALLOW_STDIN from here---only when the program itself does not use the standard input in [2/2]. > diff --git a/builtin/am.c b/builtin/am.c "git am <mbox" reads from the mailbox. "git am -i" interacts with the end user via its stdin/stdout. There may be other situations where the hooks should not touch the standard input. > diff --git a/builtin/checkout.c b/builtin/checkout.c I do not offhand think of a reason why "git checkout" would compete with its hooks for the standard input. If we were to allow hooks to read from the standard input, that should come as an independent patch for each program after patch [3/2], I think. The ones I don't mention below should never leave the standard input open for hooks. > diff --git a/builtin/clone.c b/builtin/clone.c > diff --git a/builtin/gc.c b/builtin/gc.c > diff --git a/builtin/merge.c b/builtin/merge.c > diff --git a/builtin/rebase.c b/builtin/rebase.c > diff --git a/reset.c b/reset.c Ditto. > 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..e915ffe546 100755 > --- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh > +++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh > @@ -7,6 +7,7 @@ test_description='pre-commit and pre-merge-commit hooks' > HOOKDIR="$(git rev-parse --git-dir)/hooks" > PRECOMMIT="$HOOKDIR/pre-commit" > PREMERGE="$HOOKDIR/pre-merge-commit" > +POSTCOMMIT="$HOOKDIR/post-commit" > > # Prepare sample scripts that write their $0 to actual_hooks > test_expect_success 'sample script setup' ' > @@ -28,11 +29,15 @@ test_expect_success 'sample script setup' ' > echo $0 >>actual_hooks > test $GIT_PREFIX = "success/" > EOF > - write_script "$HOOKDIR/check-author.sample" <<-\EOF > + write_script "$HOOKDIR/check-author.sample" <<-\EOF && > echo $0 >>actual_hooks > test "$GIT_AUTHOR_NAME" = "New Author" && > test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com" > EOF > + write_script "$HOOKDIR/user-input.sample" <<-\EOF > + ! read -r line || echo "$line" > hook_input > + exit 0 Style (Documentation/CodingGuidelines) - Redirection operators should be written with space before, but no space after them. In other words, write 'echo test >"$file"' instead of 'echo test> $file' or 'echo test > $file'. So, when our "read" immediately hits EOF (or I/O error, but let's not worry about that case for now), we leave hook_input file alone, but otherwise we write the single line we read to hook_input, and regardless of an error, we report success to the invoking "git commit". Which makes sense. We report success even when we had trouble writing into hook_input file, though. Perhaps you should lose the "exit 0" at the end? > + EOF > ' > > test_expect_success 'root commit' ' > @@ -278,4 +283,34 @@ test_expect_success 'check the author in hook' ' > test_cmp expected_hooks actual_hooks > ' > > +test_expect_success 'with user input' ' > + test_when_finished "rm -f \"$PRECOMMIT\" user_input hook_input" && > + cp "$HOOKDIR/user-input.sample" "$PRECOMMIT" && > + echo "user input" > user_input && Style (I won't repeat from here on). > + echo "more" >>file && > + git add file && > + git commit -m "more" < user_input && > + test_cmp user_input hook_input > +' This is probably a good place to also test git commit -F - <user_input and see what happens. Thanks.
On Thu, Nov 19, 2020 at 9:16 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Orgad Shaneh via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Orgad Shaneh <orgads@gmail.com> > > > > Let hooks receive user input if applicable. > > > > Closing stdin originates in f5bbc3225 (Port git commit to C, > > 2007). Looks like the original shell implementation did have > > stdin open. Not clear why the author chose to close it on > > the C port (maybe copy&paste). > > Please drop unsubstanciated guess in the parentheses. If anything, > we've learned during the discussion thread that it is a bad idea to > leave the standard input open when spawning hooks in general, and to > me it looks like a lot more plausible reason why we tightened the > interface when it was made from "only to run hooks for 'git commit'" > to an interface that more widely usable to run any hook. If we are > not going to record that finding in the log message to help people > to find out what we knew at the time when the commit was created, > then we shouldn't mislead them with "maybe copy&paste" that is not > backed by anything other than a hunch. Done > > Allow stdin only for commit-related hooks. Some of the other > > hooks pass their own input to the hook, so don't change them. > > Before this paragraph that gives orders to the code to "be like so", > the log message needs to explain why it is a good idea to make such > a change. Which hook benefits by being able to read the standard > input? Describe what becomes possible in terms of end-user visible > effects (i.e. "now reading standard input becomes possible for > pre-commit hook" is *not* an answer. What new things a pre-commit > hook that now can read from the standard input do for the end user?) > to justify why such a change is a good thing to have, before this > paragraph to justify why leaving the standard input open for hooks > run by "git commit" is a good idea and is a safe thing to do. Added justification. > Note that even "git commit" may compete for its standard input with > hooks. "git commit -F - <message" currently may read the message to > EOF before doing anything interesting like spawning a hook, but it > is not implausible that the reading of the message may want to > happen much later in a future codebase, at which point the hook may > end up stealing the beginning of the message by reading from the > standard input. So ideally, if we can find a way to selectively > close the standard input for the hooks if "git commit" itself uses > the standard input, that would be better than unconditionally > leaving it open. Good catch! I found that pre-commit is called before reading stdin, so -F - is broken if pre-commit reads the input. Not sure what's the best way to solve this. Should I pass the flag to run_commit_hook everywhere? Or maybe add a new opt-out flag for run_commit_hook, and pass 0 on most calls? > Let's reorder the patch hunks to see the bottom layer first, as the > callers are mostly the same. > > > diff --git a/run-command.h b/run-command.h > > index 6472b38bde..e6a850c6fe 100644 > > --- a/run-command.h > > +++ b/run-command.h > > @@ -201,11 +201,16 @@ int run_command(struct child_process *); > > */ > > const char *find_hook(const char *name); > > > > +#define RUN_HOOK_ALLOW_STDIN 1 > > + > > /** > > * Run a hook. > > - * The first argument is a pathname to an index file, or NULL > > - * if the hook uses the default index file or no index is needed. > > - * The second argument is the name of the hook. > > + * The first argument is an array of environment variables, or NULL > > + * if the hook uses the default environment and doesn't require > > + * additional variables. > > + * The second argument is zero or RUN_HOOK_ALLOW_STDIN, which enables > > + * stdin for the child process (the default is no_stdin). > > + * The third argument is the name of the hook. > > * The further arguments correspond to the hook arguments. > > * The last argument has to be NULL to terminate the arguments list. > > * If the hook does not exist or is not executable, the return > > @@ -215,8 +220,8 @@ const char *find_hook(const char *name); > > * On execution, .stdout_to_stderr and .no_stdin will be set. > > */ > > LAST_ARG_MUST_BE_NULL > > -int run_hook_le(const char *const *env, const char *name, ...); > > -int run_hook_ve(const char *const *env, const char *name, va_list args); > > +int run_hook_le(const char *const *env, int opt, const char *name, ...); > > +int run_hook_ve(const char *const *env, int opt, const char *name, va_list args); > > Is this new parameter meant to be used as an enum? When the > run_hook interface gets extended the next time and we want a new > option, is the option expected to be mutually incompatible with > allow-stdin? You found it :) > I suspect that it would make this a more useful API if this new > parameter is not used as an enum but as a collection of flag bits. > > If so, a few things must change in the above: > > - The description of the second parameter in the comment shouldn't > say "zero or RUN_HOOK_ALLOW_STDIN"; it should rather say "an > OR'ed collection of feature bits like RUN_HOOK_ALLOW_STDIN > defined above" Done, thanks. > - The second parameter should be 'unsigned flags', not 'int opt'. I copied from run_command_v_opt*, which have int opt for flags. Changed anyway. > It is my understanding that "git commit" only needs run_hook_ve() to > drive its hook scripts. Isn't it premature to touch run_hook_le(), > in which nobody wants to leave the standard input open while running > hooks? It _might_ be a better idea to allow users of _le() to do > the same eventually, but then perhaps it is a good idea to do so in > a separate step at the end, as "only to be complete" patch. That > is, the structure of the topic ought to be something like: > > - [PATCH 1/2] add the "unsigned flags" word to _ve(), assign the > RUN_HOOK_ALLOW_STDIN bit, and update commit.c::run_commit_hook() > to pass RUN_HOOK_ALLOW_STDIN to it. > > - [PATCH 2/2] after surveying the options "git commit" takes, find > out the condition where "git commit" itself would want to consume > the standard input (e.g. "commit -F -", there may be others), and > tell run_commit_hook() *not* to pass RUN_HOOK_ALLOW_STDIN when we > use the standard input ourselves (i.e. forbid hooks to read from > it). Accepted. Waiting for your feedback to implement this part. > - [PATCH 3/2] add the same "unsigned flags" word to _le(), and > teach all callers to pass 0, as a "just for completeness" step. > > Personally, I think we should stop at [2/2], and do not do [3/2], as > there is no real demonstrated use of the standard input for hooks. > Especially because users of the _le() interface includes programs > like receive-pack whose standard input should not be molested, I'd > feel safer not to see [3/2] done at all (for that matter, I'm not > happy with [1/2] unless it comes with [2/2], either). Agreed. > > + if (!(opt & RUN_HOOK_ALLOW_STDIN)) > > + hook.no_stdin = 1; > > OK, so you are using the parameter as a flag word after all. Then > "int opt" should definitely be "unsigned flags". And these two > lines would be more readable, when written like so: > > hook.no_stdin = !(flags & RUN_HOOK_ALLOW_STDIN); > > I would think. Done. > > hook.stdout_to_stderr = 1; > > hook.trace2_hook_name = name; > > > > return run_command(&hook); > > } > > > > -int run_hook_le(const char *const *env, const char *name, ...) > > +int run_hook_le(const char *const *env, int opt, const char *name, ...) > > { > > va_list args; > > int ret; > > > > va_start(args, name); > > - ret = run_hook_ve(env, name, args); > > + ret = run_hook_ve(env, opt, name, args); > > I'd rather not to see the function signature of _le() changed in > patches [1/2] and [2/2]; instead we can just pass hardcoded 0 from > here to the underlying _ve(). > > Now, what is left is individual commands that use the run_hook > interface. > > > diff --git a/commit.c b/commit.c > > index fe1fa3dc41..775019ec9d 100644 > > --- a/commit.c > > +++ b/commit.c > > @@ -1646,7 +1646,7 @@ 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); > > + ret = run_hook_ve(hook_env.v, RUN_HOOK_ALLOW_STDIN, name, args); > > va_end(args); > > strvec_clear(&hook_env); > > > > This is good for [1/2]. We should avoid "git commit" from competing > with hooks for its standard input by conditionally passing > ALLOW_STDIN from here---only when the program itself does not use > the standard input in [2/2]. > > > diff --git a/builtin/am.c b/builtin/am.c > > "git am <mbox" reads from the mailbox. "git am -i" interacts with > the end user via its stdin/stdout. There may be other situations > where the hooks should not touch the standard input. > > > diff --git a/builtin/checkout.c b/builtin/checkout.c > > I do not offhand think of a reason why "git checkout" would compete > with its hooks for the standard input. If we were to allow hooks to > read from the standard input, that should come as an independent > patch for each program after patch [3/2], I think. The ones I don't > mention below should never leave the standard input open for hooks. > > > diff --git a/builtin/clone.c b/builtin/clone.c > > diff --git a/builtin/gc.c b/builtin/gc.c > > diff --git a/builtin/merge.c b/builtin/merge.c > > diff --git a/builtin/rebase.c b/builtin/rebase.c > > diff --git a/reset.c b/reset.c > > Ditto. > > > > 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..e915ffe546 100755 > > --- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh > > +++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh > > @@ -7,6 +7,7 @@ test_description='pre-commit and pre-merge-commit hooks' > > HOOKDIR="$(git rev-parse --git-dir)/hooks" > > PRECOMMIT="$HOOKDIR/pre-commit" > > PREMERGE="$HOOKDIR/pre-merge-commit" > > +POSTCOMMIT="$HOOKDIR/post-commit" > > > > # Prepare sample scripts that write their $0 to actual_hooks > > test_expect_success 'sample script setup' ' > > @@ -28,11 +29,15 @@ test_expect_success 'sample script setup' ' > > echo $0 >>actual_hooks > > test $GIT_PREFIX = "success/" > > EOF > > - write_script "$HOOKDIR/check-author.sample" <<-\EOF > > + write_script "$HOOKDIR/check-author.sample" <<-\EOF && > > echo $0 >>actual_hooks > > test "$GIT_AUTHOR_NAME" = "New Author" && > > test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com" > > EOF > > + write_script "$HOOKDIR/user-input.sample" <<-\EOF > > + ! read -r line || echo "$line" > hook_input > > + exit 0 > > Style (Documentation/CodingGuidelines) Fixed. > - Redirection operators should be written with space before, but no > space after them. In other words, write 'echo test >"$file"' > instead of 'echo test> $file' or 'echo test > $file'. > > So, when our "read" immediately hits EOF (or I/O error, but let's > not worry about that case for now), we leave hook_input file alone, > but otherwise we write the single line we read to hook_input, and > regardless of an error, we report success to the invoking "git > commit". Which makes sense. > > We report success even when we had trouble writing into hook_input > file, though. Perhaps you should lose the "exit 0" at the end? Removed. > > + EOF > > ' > > > > test_expect_success 'root commit' ' > > @@ -278,4 +283,34 @@ test_expect_success 'check the author in hook' ' > > test_cmp expected_hooks actual_hooks > > ' > > > > +test_expect_success 'with user input' ' > > + test_when_finished "rm -f \"$PRECOMMIT\" user_input hook_input" && > > + cp "$HOOKDIR/user-input.sample" "$PRECOMMIT" && > > + echo "user input" > user_input && > > Style (I won't repeat from here on). Fixed. > > + echo "more" >>file && > > + git add file && > > + git commit -m "more" < user_input && > > + test_cmp user_input hook_input > > +' > > This is probably a good place to also test > > git commit -F - <user_input > > and see what happens. Added a test (currently failing). > Thanks. Thank you! Your feedback is thorough and helpful. - Orgad
diff --git a/builtin/am.c b/builtin/am.c index f22c73a05b..1946569d5b 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -428,7 +428,7 @@ static int run_applypatch_msg_hook(struct am_state *state) int ret; assert(state->msg); - ret = run_hook_le(NULL, "applypatch-msg", am_path(state, "final-commit"), NULL); + ret = run_hook_le(NULL, 0, "applypatch-msg", am_path(state, "final-commit"), NULL); if (!ret) { FREE_AND_NULL(state->msg); @@ -1559,7 +1559,7 @@ static void do_commit(const struct am_state *state) const char *reflog_msg, *author, *committer = NULL; struct strbuf sb = STRBUF_INIT; - if (run_hook_le(NULL, "pre-applypatch", NULL)) + if (run_hook_le(NULL, 0, "pre-applypatch", NULL)) exit(1); if (write_cache_as_tree(&tree, 0, NULL)) @@ -1611,7 +1611,7 @@ static void do_commit(const struct am_state *state) fclose(fp); } - run_hook_le(NULL, "post-applypatch", NULL); + run_hook_le(NULL, 0, "post-applypatch", NULL); strbuf_release(&sb); } diff --git a/builtin/checkout.c b/builtin/checkout.c index 9b82119129..293e8ebd76 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -104,7 +104,7 @@ struct branch_info { static int post_checkout_hook(struct commit *old_commit, struct commit *new_commit, int changed) { - return run_hook_le(NULL, "post-checkout", + return run_hook_le(NULL, 0, "post-checkout", oid_to_hex(old_commit ? &old_commit->object.oid : &null_oid), oid_to_hex(new_commit ? &new_commit->object.oid : &null_oid), changed ? "1" : "0", NULL); diff --git a/builtin/clone.c b/builtin/clone.c index a0841923cf..6cfd2f23be 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -816,7 +816,7 @@ static int checkout(int submodule_progress) if (write_locked_index(&the_index, &lock_file, COMMIT_LOCK)) die(_("unable to write new index file")); - err |= run_hook_le(NULL, "post-checkout", oid_to_hex(&null_oid), + err |= run_hook_le(NULL, 0, "post-checkout", oid_to_hex(&null_oid), oid_to_hex(&oid), "1", NULL); if (!err && (option_recurse_submodules.nr > 0)) { diff --git a/builtin/gc.c b/builtin/gc.c index 5cd2a43f9f..c790a362df 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -385,7 +385,7 @@ static int need_to_gc(void) else return 0; - if (run_hook_le(NULL, "pre-auto-gc", NULL)) + if (run_hook_le(NULL, 0, "pre-auto-gc", NULL)) return 0; return 1; } diff --git a/builtin/merge.c b/builtin/merge.c index 4c133402a6..1f1d234879 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -474,7 +474,7 @@ static void finish(struct commit *head_commit, } /* Run a post-merge hook */ - run_hook_le(NULL, "post-merge", squash ? "1" : "0", NULL); + run_hook_le(NULL, 0, "post-merge", squash ? "1" : "0", NULL); apply_autostash(git_path_merge_autostash(the_repository)); strbuf_release(&reflog_message); diff --git a/builtin/rebase.c b/builtin/rebase.c index 7b65525301..c17f7a628b 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -2014,7 +2014,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) /* If a hook exists, give it a chance to interrupt*/ if (!ok_to_skip_pre_rebase && - run_hook_le(NULL, "pre-rebase", options.upstream_arg, + run_hook_le(NULL, 0, "pre-rebase", options.upstream_arg, argc ? argv[0] : NULL, NULL)) die(_("The pre-rebase hook refused to rebase.")); diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index bb9909c52e..c1398af755 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1384,7 +1384,7 @@ static const char *push_to_checkout(unsigned char *hash, const char *work_tree) { strvec_pushf(env, "GIT_WORK_TREE=%s", absolute_path(work_tree)); - if (run_hook_le(env->v, push_to_checkout_hook, + if (run_hook_le(env->v, 0, push_to_checkout_hook, hash_to_hex(hash), NULL)) return "push-to-checkout hook declined"; else diff --git a/commit.c b/commit.c index fe1fa3dc41..775019ec9d 100644 --- a/commit.c +++ b/commit.c @@ -1646,7 +1646,7 @@ 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); + ret = run_hook_ve(hook_env.v, RUN_HOOK_ALLOW_STDIN, name, args); va_end(args); strvec_clear(&hook_env); diff --git a/read-cache.c b/read-cache.c index ecf6f68994..a83beac63e 100644 --- a/read-cache.c +++ b/read-cache.c @@ -3070,7 +3070,7 @@ static int do_write_locked_index(struct index_state *istate, struct lock_file *l else ret = close_lock_file_gently(lock); - run_hook_le(NULL, "post-index-change", + run_hook_le(NULL, 0, "post-index-change", istate->updated_workdir ? "1" : "0", istate->updated_skipworktree ? "1" : "0", NULL); istate->updated_workdir = 0; diff --git a/reset.c b/reset.c index 2f4fbd07c5..33687b0b5b 100644 --- a/reset.c +++ b/reset.c @@ -127,7 +127,7 @@ int reset_head(struct repository *r, struct object_id *oid, const char *action, reflog_head); } if (run_hook) - run_hook_le(NULL, "post-checkout", + run_hook_le(NULL, 0, "post-checkout", oid_to_hex(orig ? orig : &null_oid), oid_to_hex(oid), "1", NULL); diff --git a/run-command.c b/run-command.c index 2ee59acdc8..21b1f0a5e9 100644 --- a/run-command.c +++ b/run-command.c @@ -1343,7 +1343,7 @@ const char *find_hook(const char *name) return path.buf; } -int run_hook_ve(const char *const *env, const char *name, va_list args) +int run_hook_ve(const char *const *env, int opt, const char *name, va_list args) { struct child_process hook = CHILD_PROCESS_INIT; const char *p; @@ -1356,20 +1356,21 @@ int run_hook_ve(const char *const *env, const char *name, va_list args) while ((p = va_arg(args, const char *))) strvec_push(&hook.args, p); hook.env = env; - hook.no_stdin = 1; + if (!(opt & RUN_HOOK_ALLOW_STDIN)) + hook.no_stdin = 1; hook.stdout_to_stderr = 1; hook.trace2_hook_name = name; return run_command(&hook); } -int run_hook_le(const char *const *env, const char *name, ...) +int run_hook_le(const char *const *env, int opt, const char *name, ...) { va_list args; int ret; va_start(args, name); - ret = run_hook_ve(env, name, args); + ret = run_hook_ve(env, opt, name, args); va_end(args); return ret; diff --git a/run-command.h b/run-command.h index 6472b38bde..e6a850c6fe 100644 --- a/run-command.h +++ b/run-command.h @@ -201,11 +201,16 @@ int run_command(struct child_process *); */ const char *find_hook(const char *name); +#define RUN_HOOK_ALLOW_STDIN 1 + /** * Run a hook. - * The first argument is a pathname to an index file, or NULL - * if the hook uses the default index file or no index is needed. - * The second argument is the name of the hook. + * The first argument is an array of environment variables, or NULL + * if the hook uses the default environment and doesn't require + * additional variables. + * The second argument is zero or RUN_HOOK_ALLOW_STDIN, which enables + * stdin for the child process (the default is no_stdin). + * The third argument is the name of the hook. * The further arguments correspond to the hook arguments. * The last argument has to be NULL to terminate the arguments list. * If the hook does not exist or is not executable, the return @@ -215,8 +220,8 @@ const char *find_hook(const char *name); * On execution, .stdout_to_stderr and .no_stdin will be set. */ LAST_ARG_MUST_BE_NULL -int run_hook_le(const char *const *env, const char *name, ...); -int run_hook_ve(const char *const *env, const char *name, va_list args); +int run_hook_le(const char *const *env, int opt, const char *name, ...); +int run_hook_ve(const char *const *env, int opt, const char *name, va_list args); /* * Trigger an auto-gc 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..e915ffe546 100755 --- a/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh +++ b/t/t7503-pre-commit-and-pre-merge-commit-hooks.sh @@ -7,6 +7,7 @@ test_description='pre-commit and pre-merge-commit hooks' HOOKDIR="$(git rev-parse --git-dir)/hooks" PRECOMMIT="$HOOKDIR/pre-commit" PREMERGE="$HOOKDIR/pre-merge-commit" +POSTCOMMIT="$HOOKDIR/post-commit" # Prepare sample scripts that write their $0 to actual_hooks test_expect_success 'sample script setup' ' @@ -28,11 +29,15 @@ test_expect_success 'sample script setup' ' echo $0 >>actual_hooks test $GIT_PREFIX = "success/" EOF - write_script "$HOOKDIR/check-author.sample" <<-\EOF + write_script "$HOOKDIR/check-author.sample" <<-\EOF && echo $0 >>actual_hooks test "$GIT_AUTHOR_NAME" = "New Author" && test "$GIT_AUTHOR_EMAIL" = "newauthor@example.com" EOF + write_script "$HOOKDIR/user-input.sample" <<-\EOF + ! read -r line || echo "$line" > hook_input + exit 0 + EOF ' test_expect_success 'root commit' ' @@ -278,4 +283,34 @@ test_expect_success 'check the author in hook' ' test_cmp expected_hooks actual_hooks ' +test_expect_success 'with user input' ' + test_when_finished "rm -f \"$PRECOMMIT\" user_input hook_input" && + cp "$HOOKDIR/user-input.sample" "$PRECOMMIT" && + echo "user input" > user_input && + echo "more" >>file && + git add file && + git commit -m "more" < user_input && + test_cmp user_input hook_input +' + +test_expect_success 'post-commit with user input' ' + test_when_finished "rm -f \"$POSTCOMMIT\" user_input hook_input" && + cp "$HOOKDIR/user-input.sample" "$POSTCOMMIT" && + echo "user input" > user_input && + echo "more" >>file && + git add file && + git commit -m "more" < user_input && + test_cmp user_input hook_input +' + +test_expect_success 'with user input (merge)' ' + test_when_finished "rm -f \"$PREMERGE\" user_input hook_input" && + cp "$HOOKDIR/user-input.sample" "$PREMERGE" && + echo "user input" > user_input && + git checkout side && + git merge -m "merge master" master < user_input && + git checkout master && + test_cmp user_input hook_input +' + test_done diff --git a/t/t7504-commit-msg-hook.sh b/t/t7504-commit-msg-hook.sh index 31b9c6a2c1..ad467aad86 100755 --- a/t/t7504-commit-msg-hook.sh +++ b/t/t7504-commit-msg-hook.sh @@ -294,5 +294,20 @@ test_expect_success 'hook is called for reword during `rebase -i`' ' ' +# now a hook that accepts input and writes it as the commit message +cat > "$HOOK" <<'EOF' +#!/bin/sh +! read -r line || echo "$line" > "$1" +EOF +chmod +x "$HOOK" + +test_expect_success 'hook with user input' ' + + echo "additional" >> file && + git add file && + echo "user input" | git commit -m "additional" && + commit_msg_is "user input" + +' test_done diff --git a/t/t7505-prepare-commit-msg-hook.sh b/t/t7505-prepare-commit-msg-hook.sh index 94f85cdf83..16a161f129 100755 --- a/t/t7505-prepare-commit-msg-hook.sh +++ b/t/t7505-prepare-commit-msg-hook.sh @@ -91,6 +91,11 @@ else fi test "$GIT_EDITOR" = : && source="$source (no editor)" +if read -r line +then + source="$source $line" +fi + if test $rebasing = 1 then echo "$source $(get_last_cmd)" >"$1" @@ -113,6 +118,15 @@ test_expect_success 'with hook (-m)' ' ' +test_expect_success 'with hook (-m and input)' ' + + echo "more" >> file && + git add file && + echo "user input" | git commit -m "more" && + test "$(git log -1 --pretty=format:%s)" = "message (no editor) user input" + +' + test_expect_success 'with hook (-m editor)' ' echo "more" >> file &&