captainhook icon indicating copy to clipboard operation
captainhook copied to clipboard

[Feature] Allow hooks to prompt users for input

Open ramsey opened this issue 5 years ago • 14 comments

Use Case

As a project maintainer, I want Git hooks to interactively prompt contributors and accept input from them in order to proceed with the hook's action. For example, in the prepare-commit-msg hook, I would like to prompt the user to enter each part of a Conventional Commits commit message, which would then be constructed into a full commit message automatically.

Discussion

Git hooks do not appear to run in an interactive environment, so attempts to prompt a user and wait for input are ignored (the text of the prompt appears, but it does not attempt to read input from the TTY). To solve this, we must tell the hook script's current shell to read from the TTY. We can do this with:

exec </dev/tty

According to man bash, if the command for exec "is not specified, any redirections take effect in the current shell."^1

We can take this further by explicitly redirecting stdout and stderr to the TTY, as well (though this may be redundant):

exec >/dev/tty 2>/dev/tty </dev/tty

The primary drawback to this approach is when a user uses a Git command that invokes a hook from within an environment that doesn't have a terminal (such as an IDE). In these cases (at least in the case of JetBrains IDEs), the user may see an error similar to the following in the logs:

/dev/tty: Device not configured

This will cause the hook to fail, blocking success of the command.

To prevent this, we also need to detect whether the hook has a terminal. If it does not, CaptainHook should run in non-interactive mode, which will allow hooks to check IO::isInteractive() and turn off prompts accordingly.

Further Reading

ramsey avatar Jul 20 '20 16:07 ramsey

I think this is depending on the installation mode you are using You can make the Cap'n generate different types of hook scripts in ./git/hooks/ Currently there are docker, shell and php if you choose php this should not be a problem because you directly call php and have direct access to the stdIn. I have no clue if it is fixable for the docker installation but for the shell variant using the tty redirects should work.

If there is a way to check if tty is "configured" and only then execute the exec it would be safe to add it to the standard shell installation. Otherwise we could add a shell-tty installation option.

sebastianfeldmann avatar Jul 20 '20 16:07 sebastianfeldmann

If there is a way to check if tty is "configured"

The only way I've found that seems to work is to check for -t 1. According to the man page, -t fd returns "true if file descriptor fd is open and refers to a terminal." Since 1 refers to stdout, this checks to see if stdout is a terminal, and if so, it runs exec (see #88).

ramsey avatar Jul 20 '20 16:07 ramsey

I think this is depending on the installation mode you are using

I'll test a prompt with docker and php to see how they behave. 😄

ramsey avatar Jul 20 '20 16:07 ramsey

I released your shell fix with version 5.3.3

sebastianfeldmann avatar Jul 21 '20 08:07 sebastianfeldmann

There is another thing that we should consider. There is a new hook post-rebase that receives data via stdIn. Normally data is passed via command options to hooks but as the post-rebase hook receives a list of all affected commits and some additional data it is done via stdIn. I'm not sure we can support both passing the hook stdIn and the tty to the Cap`n :/

I think we could create different .git/hooks scripts, so some hooks support the tty and others forward the stdIn

sebastianfeldmann avatar Jul 21 '20 08:07 sebastianfeldmann

Hmm. Good points. I'd have to try the hooks that support stdin to see how they behave.

ramsey avatar Jul 21 '20 18:07 ramsey

Currently there is no implementation for post-rebase :( It is on my todo list to integrate something like IO::getStandardIn to the IO object provided to every hook or condition. But I haven't looked into this "problem" in much detail. I could add a post-rebase implementation, but without a way to access the provided information from stdIn it would not be very useful :/

sebastianfeldmann avatar Jul 21 '20 18:07 sebastianfeldmann

By trying the hooks, I meant that I would create a standard post-rebase bash script in the .git/hooks directory and play around with it. 😄

ramsey avatar Jul 21 '20 18:07 ramsey

I did a bit of testing and implemented a way to receive the stdIn within a hook. IO::getStandardInput. For this to work the shell installation has to differ between hooks that need the hook stdIn and the keyboard stdIn. So I created a keyboard stdIn whitelist do define the hooks that should receive the keyboard input as stdIn. For now this whitelist only contains prepare-commit-msg @ramsey do you think this will suffice or would you allow other hooks to access the keyboard as well?

I have not tested the docker installation yet and if it possible to pass the hook stdIn to the docker execution maybe @icanhazstring can help, he did most of the docker implementation anyways ;)

sebastianfeldmann avatar Jul 24 '20 09:07 sebastianfeldmann

Right now, the only use-case I can imagine for user input is prepare-commit-msg. You could start with that, and if other use cases arise, make changes as needed.

ramsey avatar Jul 24 '20 13:07 ramsey

Hopefully, some more goodies to support prompting the user to create commit messages: https://github.com/symfony/symfony/pull/37683

😁

ramsey avatar Jul 28 '20 02:07 ramsey

https://github.com/symfony/symfony/pull/37683 was merged and will be in symfony/console 5.2!

ramsey avatar Aug 14 '20 02:08 ramsey

Just to leave some research here:

There are two things we need to consider:

  1. The hook needs user keyboard input
  2. The hook "just needs" input (STDIN)

For both cases on docker we need to provide the -i option to enable interactive mode. This also covers point 2.

For user keyboard input we need to provide the -t (tty) option for docker. Also we need to have -t 1 or exec < /dev/tty before the run docker exec. This way we provide the needed tty input interface.

For a simple test script (also valid as hook)

#!/bin/bash
exec < /dev/tty
docker exec -it <container> cat /dev/stdin
#!/bin/bash
docker exec -i <container> cat /dev/stdin

icanhazstring avatar Oct 24 '20 13:10 icanhazstring

I updated the docker setup quite a bit to work on this.

I discussed with Sebastian about how to tackle this:

  • we can always add the -i to the docker command if docker exec/run is used
  • the -t is depending on the hook which is installed
  • we need to "parse" the docker command while the template is being created (or rather the config for it)
  • inside the template we decide if the exec < /dec/tty should be used depending on the config

icanhazstring avatar Oct 26 '20 08:10 icanhazstring

Hi, I'm using captainhookphp for static code analysis and conventional commits using ramsey/conventional-commits in a docker env.

The static code commands are not interactive, so no issue happens, while the interactive prepare-commit-msg hook is more problematic.

captainhook.json

{
    "prepare-commit-msg": {
        "enabled": true,
        "actions": [
            {
                "action": "\\Ramsey\\CaptainHook\\PrepareConventionalCommit",
                "options": {
                    "config": {
                        "types": ["feat", "fix", "docs", "ci", "refactor", "perf", "test", "revert"]
                    }
                }
            }
        ]
    },
    "commit-msg": {
        "enabled": true,
        "actions": [
            {
                "action": "\\Ramsey\\CaptainHook\\ValidateConventionalCommit",
                "options": {
                    "config": {
                        "types": ["feat", "fix", "docs", "ci", "refactor", "perf", "test", "revert"]
                    }
                }
            }
        ]
    }
}

The install command I used is vendor/bin/captainhook install --only-enabled --run-mode=docker --run-exec="docker run --rm -i -v ${PWD}/:/usr/src/app/ php-ci-cd:latest" so the content of .git/hooks/prepare-commit-msg is

#!/bin/sh

# installed by CaptainHook 5.16.4

docker run --rm -i -v ./:/usr/src/app/ php-ci-cd:latest ./vendor/bin/captainhook hook:prepare-commit-msg "$@"

doesn't work

prepare-commit-msg: 
 - \Ramsey\CaptainHook\PrepareConventionalCommit                     : 
Prepare Commit Message
======================

 The following prompts will help you create a commit message that
 follows the Conventional Commits specification.

 What is the type of change you're committing? (e.g., feat, fix, etc.) [feat]:
 > failed

PHP Error:Ramsey\ConventionalCommits\Message::__construct(): Argument #2 ($description) must be of type Ramsey\ConventionalCommits\Message\Description, null given, called in /usr/src/app/vendor/ramsey/conventional-commits/src/ConventionalCommits/Console/Command/PrepareCommand.php on line 147 in /usr/src/app/vendor/ramsey/conventional-commits/src/ConventionalCommits/Message.php line 61

It works if I manually add exec < /dec/tty to .git/hooks/prepare-commit-msg, but I would like captainhookphp to do it, depending on the config.

Is it possible, or do you know an alternative way?

ruddenchaux avatar Sep 14 '23 08:09 ruddenchaux

The captain now decides by Hooks::allowsUserInput if the tty handling has to be added to the hook script.

If you run in docker mode, this is currently NOT done because the --tty option does not work if you commit via PHPStorm. It seems to be a bug within the tty handling within PHPStorm. There you still have to add the --tty option manually to your .git/hooks/prepare-commit-msg script to activate it.

Released with 5.17.0

sebastianfeldmann avatar Oct 02 '23 08:10 sebastianfeldmann