| Message ID | 3bd6024a236b061c89bb6b60daf3dc15ef1e32ca.1605819390.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hooks: allow input from stdin for commit-related hooks | expand |
On Thu, Nov 19, 2020 at 3:57 PM Orgad Shaneh via GitGitGadget <gitgitgadget@gmail.com> wrote: > Let hooks receive user input if applicable. > [...] > This allows for example prompting the user to choose an issue > in prepare-commit-msg, and add "Fixes #123" to the commit message. > > Another possible use-case is running sanity test on pre-commit, > and having a prompt like "This and that issue were found in your > changes. Are you sure you want to commit? [Y/N]". These use-cases really help readers understand the motivation for this change. Good. > Allow stdin only for commit-related hooks. Some of the other > hooks pass their own input to the hook, so don't change them. > > Note: If pre-commit reads from stdin, and git commit is executed > with -F - (read message from stdin), the message is not read > correctly. This is fixed in the follow-up commit. Rather than making such a fundamental change and having to deal with the fallout by introducing complexity to handle various special-cases which pop up now and in the future, I wonder if it makes more sense to instead just update documentation to tell hook authors to read explicitly from the console rather than expecting stdin to be available (since stdin may already be consumed for other purposes when dealing with hooks or commands which invoke the hooks).
Eric Sunshine <sunshine@sunshineco.com> writes: > Rather than making such a fundamental change and having to deal with > the fallout by introducing complexity to handle various special-cases > which pop up now and in the future, I wonder if it makes more sense to > instead just update documentation to tell hook authors to read > explicitly from the console rather than expecting stdin to be > available (since stdin may already be consumed for other purposes when > dealing with hooks or commands which invoke the hooks). ;-) Thanks for saying this.
On Thu, Nov 19, 2020 at 11:23 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Thu, Nov 19, 2020 at 3:57 PM Orgad Shaneh via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > Let hooks receive user input if applicable. > > [...] > > This allows for example prompting the user to choose an issue > > in prepare-commit-msg, and add "Fixes #123" to the commit message. > > > > Another possible use-case is running sanity test on pre-commit, > > and having a prompt like "This and that issue were found in your > > changes. Are you sure you want to commit? [Y/N]". > > These use-cases really help readers understand the motivation for this > change. Good. > > > Allow stdin only for commit-related hooks. Some of the other > > hooks pass their own input to the hook, so don't change them. > > > > Note: If pre-commit reads from stdin, and git commit is executed > > with -F - (read message from stdin), the message is not read > > correctly. This is fixed in the follow-up commit. > > Rather than making such a fundamental change and having to deal with > the fallout by introducing complexity to handle various special-cases > which pop up now and in the future, I wonder if it makes more sense to > instead just update documentation to tell hook authors to read > explicitly from the console rather than expecting stdin to be > available (since stdin may already be consumed for other purposes when > dealing with hooks or commands which invoke the hooks). On the first revision I had several links in the commit message to users who solved it this way. This solution however is not optimal. I have a prepare-commit-msg hook that requires user interaction for choosing an issue. This hook must work from the terminal and also from GUI applications like IDE. Currently the hook always pops a GUI window, but when using it from the terminal this is inconvenient (and when running over remote SSH without X forwarding it can't work), so I'd like it to be usable also from the terminal. To achieve that, I created 2 classes - one for terminal and one for GUI, and trying to choose the correct class by checking if stdin is a tty. The condition looks like this (Ruby): client = STDIN.tty? ? Terminal.new : GUI.new At this point I was surprised to discover that Git closes stdin, so the condition is never satisfied, and I always end up with GUI. As I mentioned, I need it to work also when executed from GUI applications, so just reading from the console will not work in my case. I tried other ways to detect "running from terminal" without the tty condition, but couldn't. The environment variables are identical when running in a GUI terminal and in the IDE. Can you suggest an alternative way to determine if I can accept user input from the console or not? - Orgad
On Fri, Nov 20, 2020 at 12:23 AM Orgad Shaneh <orgads@gmail.com> wrote: > On Thu, Nov 19, 2020 at 11:23 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > [...] I wonder if it makes more sense to > > instead just update documentation to tell hook authors to read > > explicitly from the console rather than expecting stdin to be > > available [...] > > I have a prepare-commit-msg hook that requires user interaction for > choosing an issue. This hook must work from the terminal and also > from GUI applications like IDE. > [...] > As I mentioned, I need it to work also when executed from > GUI applications, so just reading from the console will not work > in my case. I tried other ways to detect "running from terminal" > without the tty condition, but couldn't. The environment variables > are identical when running in a GUI terminal and in the IDE. > > Can you suggest an alternative way to determine if I can accept user > input from the console or not? Not at present, and I expect that the answer is that any such mechanism for determining this would be IDE-dependent. (That is, although your IDE doesn't distinguish itself in any way which your hook can detect, other IDE's might, but that doesn't help in the general case.) What I can say, though, is that the additional information you supplied in your response should be part of the commit message to help reviewers and future readers better understand why this change is wanted. The use-cases presented in the v4 commit message, although helpful, didn't provide sufficient explanation considering that the first question which popped into this reviewer's mind was "why not have the hook read from the console explicitly?". (It is an unfortunate fact that reviewer time is a limited resource, so many reviewers on this list don't bother chasing down links like those you included in the commit message of v1 -- which would have helped justify the change -- but instead base their reviews only on the information presented in the commit message itself. In my case, I was only lightly skimming this series, thus didn't even bother chasing down those links -- but have done so now for this reply.) I do find it quite concerning that the way this series handles the stdin conflict between the hook and `-F -` can break the hook silently and mysteriously. How confusing for a user to write a hook which works with `git commit -m msg` and `git commit -F file` but breaks silently with `git commit -F -`. What is worse is that this breakage may be outside the user's control. For instance, it is easy to imagine some IDE passing the commit message to git-commit via stdin (using `-F -`) rather than via a file (using `-F file`). At the very least, this change deserves a documentation update, both to explain that the prepare-commit-msg hook has a valid stdin, and (importantly) that it won't be able to rely upon stdin in conjunction with `-F -`. (This also makes me wonder if it would be possible to signal to the hook whether or not stdin is available. Perhaps this could be done by passing an additional argument to the hook.) Finally, I realize that you followed Junio's suggestion for organizing the series, however, it feels undesirable for patch [1/2] to leave the command in a somewhat broken state, by which I'm referring to the indeterminate outcome of the hook and `-F -` competing for stdin; a situation which is only resolved in [2/2]. To me, a cleaner organization would be for [1/2] to introduce the underlying mechanism and support by adding `flags` to run_hook_ve() (and perhaps to run_commit_hook()) but not to turn on RUN_HOOK_ALLOW_STDIN, and then have patch [2/2] actually enable RUN_HOOK_ALLOW_STDIN where appropriate _and_ deal with the `-F -` conflict all at the same time. (And the commit message should mention the conflict and how it is handled.)
On Fri, Nov 20, 2020 at 1:38 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > I do find it quite concerning that the way this series handles the > stdin conflict between the hook and `-F -` can break the hook silently > and mysteriously. How confusing for a user to write a hook which works > with `git commit -m msg` and `git commit -F file` but breaks silently > with `git commit -F -`. What is worse is that this breakage may be > outside the user's control. For instance, it is easy to imagine some > IDE passing the commit message to git-commit via stdin (using `-F -`) > rather than via a file (using `-F file`). > > At the very least, this change deserves a documentation update, both > to explain that the prepare-commit-msg hook has a valid stdin, and > (importantly) that it won't be able to rely upon stdin in conjunction > with `-F -`. What I forgot to say here was that this patch series doesn't help users at all if their IDE passes the commit message to git-commit via stdin using `-F -`. In such a case, their hook will _never_ see a valid stdin coming from Git, no matter what their script does. So, the change made by this patch series may help some users but not others, and this is a limitation that should be stated in the commit message (and perhaps mentioned in the documentation, though that may be difficult to do in a general way).
On Fri, Nov 20, 2020 at 8:49 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Fri, Nov 20, 2020 at 1:38 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > I do find it quite concerning that the way this series handles the > > stdin conflict between the hook and `-F -` can break the hook silently > > and mysteriously. How confusing for a user to write a hook which works > > with `git commit -m msg` and `git commit -F file` but breaks silently > > with `git commit -F -`. What is worse is that this breakage may be > > outside the user's control. For instance, it is easy to imagine some > > IDE passing the commit message to git-commit via stdin (using `-F -`) > > rather than via a file (using `-F file`). > > > > At the very least, this change deserves a documentation update, both > > to explain that the prepare-commit-msg hook has a valid stdin, and > > (importantly) that it won't be able to rely upon stdin in conjunction > > with `-F -`. > > What I forgot to say here was that this patch series doesn't help > users at all if their IDE passes the commit message to git-commit via > stdin using `-F -`. In such a case, their hook will _never_ see a > valid stdin coming from Git, no matter what their script does. So, the > change made by this patch series may help some users but not others, > and this is a limitation that should be stated in the commit message > (and perhaps mentioned in the documentation, though that may be > difficult to do in a general way). At least in my case, I never expect stdin to be available when running in the IDE, so my hook is expected to use GUI anyway. I only need stdin when the user is running git from the terminal. So all I need from the IDE is that it doesn't pretend to be a tty while running Git. And regarding the hook itself - the hook author should be aware that stdin is not always a tty, and sometimes can be closed or pipe, and write the hook in a way that handles special cases. - Orgad
On Fri, Nov 20 2020, Orgad Shaneh wrote: > On Thu, Nov 19, 2020 at 11:23 PM Eric Sunshine <sunshine@sunshineco.com> wrote: >> >> On Thu, Nov 19, 2020 at 3:57 PM Orgad Shaneh via GitGitGadget >> <gitgitgadget@gmail.com> wrote: >> > Let hooks receive user input if applicable. >> > [...] >> > This allows for example prompting the user to choose an issue >> > in prepare-commit-msg, and add "Fixes #123" to the commit message. >> > >> > Another possible use-case is running sanity test on pre-commit, >> > and having a prompt like "This and that issue were found in your >> > changes. Are you sure you want to commit? [Y/N]". >> >> These use-cases really help readers understand the motivation for this >> change. Good. >> >> > Allow stdin only for commit-related hooks. Some of the other >> > hooks pass their own input to the hook, so don't change them. >> > >> > Note: If pre-commit reads from stdin, and git commit is executed >> > with -F - (read message from stdin), the message is not read >> > correctly. This is fixed in the follow-up commit. >> >> Rather than making such a fundamental change and having to deal with >> the fallout by introducing complexity to handle various special-cases >> which pop up now and in the future, I wonder if it makes more sense to >> instead just update documentation to tell hook authors to read >> explicitly from the console rather than expecting stdin to be >> available (since stdin may already be consumed for other purposes when >> dealing with hooks or commands which invoke the hooks). > > On the first revision I had several links in the commit message to > users who solved it this way. This solution however is not optimal. > I have a prepare-commit-msg hook that requires user interaction for > choosing an issue. This hook must work from the terminal and also > from GUI applications like IDE. > > Currently the hook always pops a GUI window, but when using it > from the terminal this is inconvenient (and when running over > remote SSH without X forwarding it can't work), so I'd like it to be > usable also from the terminal. > > To achieve that, I created 2 classes - one for terminal and one > for GUI, and trying to choose the correct class by checking if > stdin is a tty. The condition looks like this (Ruby): > client = STDIN.tty? ? Terminal.new : GUI.new > > At this point I was surprised to discover that Git closes stdin, > so the condition is never satisfied, and I always end up with GUI. > > As I mentioned, I need it to work also when executed from > GUI applications, so just reading from the console will not work > in my case. I tried other ways to detect "running from terminal" > without the tty condition, but couldn't. The environment variables > are identical when running in a GUI terminal and in the IDE. > > Can you suggest an alternative way to determine if I can accept user > input from the console or not? Like Eric noted in his reply I can't think of a way to do that particular thing reliably either, and agree with his comments that if such a way is found / some aspect of this change is kept having this explanation in the patch/commit message is really helpful. I think what you're trying to do here isn't a good fit for most git workflows. Instead of trying to interactively compose a commit message why not change the commit template to start with e.g.: # You must replace XXX with an issue number here!: Issue #XXX: That gives the user the same thing to fill out, but in their editor instead of via some terminal/GUI prompt. They need to write the rest of the commit message anyway in the editor, so even if you could why open up two UIs? Projects that have these conventions also typically settle on just not trying to solve this problem on the client-side, but e.g. having a pre-receive hook that does the validation, or do it via CI / before a merge to master happens etc.
On Fri, Nov 20, 2020 at 12:59 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Fri, Nov 20 2020, Orgad Shaneh wrote: > > > Can you suggest an alternative way to determine if I can accept user > > input from the console or not? > > Like Eric noted in his reply I can't think of a way to do that > particular thing reliably either, and agree with his comments that if > such a way is found / some aspect of this change is kept having this > explanation in the patch/commit message is really helpful. I'll reword. > I think what you're trying to do here isn't a good fit for most git > workflows. Instead of trying to interactively compose a commit message > why not change the commit template to start with e.g.: > > # You must replace XXX with an issue number here!: > Issue #XXX: > > That gives the user the same thing to fill out, but in their editor > instead of via some terminal/GUI prompt. They need to write the rest of > the commit message anyway in the editor, so even if you could why open > up two UIs? We do have a template. The hook pops a listbox with all the open issues assigned to the user, which he/she can easily pick from, instead of searching for them in the browser and copying the issue id. This is only done if the user doesn't write an issue in the commit message. > Projects that have these conventions also typically settle on just not > trying to solve this problem on the client-side, but e.g. having a > pre-receive hook that does the validation, or do it via CI / before a > merge to master happens etc. We have validation on the server too. The hook is there for convenience. - Orgad
Eric Sunshine <sunshine@sunshineco.com> writes: > At the very least, this change deserves a documentation update, both > to explain that the prepare-commit-msg hook has a valid stdin, and > (importantly) that it won't be able to rely upon stdin in conjunction > with `-F -`. (This also makes me wonder if it would be possible to > signal to the hook whether or not stdin is available. Perhaps this > could be done by passing an additional argument to the hook.) > > Finally, I realize that you followed Junio's suggestion for organizing > the series, however, it feels undesirable for patch [1/2] to leave the > command in a somewhat broken state, ... True. The split you suggest sounds saner, if we were to still move forward with this change. I originally threw "commit -F -" in the "don't do it if it hurts" category, but I agree with you that it is quite plausible that IDE would want to use the feature to feed the log message to the command (that way they do not need to worry about a temporary file at all), so it can become a real issue. Thanks.
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/run-command.c b/run-command.c index 2ee59acdc8..38ce53bee5 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, unsigned flags, const char *name, va_list args) { struct child_process hook = CHILD_PROCESS_INIT; const char *p; @@ -1356,7 +1356,7 @@ 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; + hook.no_stdin = !(flags & RUN_HOOK_ALLOW_STDIN); hook.stdout_to_stderr = 1; hook.trace2_hook_name = name; @@ -1369,7 +1369,7 @@ int run_hook_le(const char *const *env, const char *name, ...) int ret; va_start(args, name); - ret = run_hook_ve(env, name, args); + ret = run_hook_ve(env, 0, name, args); va_end(args); return ret; diff --git a/run-command.h b/run-command.h index 6472b38bde..e613e5e3f9 100644 --- a/run-command.h +++ b/run-command.h @@ -201,22 +201,29 @@ 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 env argument is an array of environment variables, or NULL + * if the hook uses the default environment and doesn't require + * additional variables. + * The flags argument is an OR'ed collection of feature bits like + * RUN_HOOK_ALLOW_STDIN defined above, which enables + * stdin for the child process (the default is no_stdin). + * The name 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 * value will be zero. * If it is executable, the hook will be executed and the exit * status of the hook is returned. - * On execution, .stdout_to_stderr and .no_stdin will be set. + * On execution, .stdout_to_stderr will be set, and .no_stdin will be + * set unless RUN_HOOK_ALLOW_STDIN flag is requested. */ 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_ve(const char *const *env, unsigned flags, 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..7bfb7435c6 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,14 @@ 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 + EOF ' test_expect_success 'root commit' ' @@ -278,4 +282,44 @@ 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_failure 'with user input combined with -F -' ' + 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 -F - <user_input && + ! test_path_is_file 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..aa76eb7e1f 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..aa9c9375e6 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 &&