Bugfix: No prompt if no controlling terminal could be detected :tada:

[gabyx]

By https://github.com/rycus86/githooks/pull/81#issuecomment-614782632 this fix should fix the logic when we read from /dev/tty for getting an answer to a prompt. We dont modify the variable read into if we could not read something from a controlling terminal:

@matthijskooijman Shouldn't we only (if and only if) read from /dev/tty when stdin is also a tty meaning the change should be maybe even more strict:

if [-t 0] && [ -t 1 ]; then
        printf "%s %s [%s]:" "$TEXT" "$HINT_TEXT" "$SHORT_OPTIONS"
        # shellcheck disable=SC2229
        read -r "$VARIABLE" </dev/tty
    fi

[gabyx]

No controlling terminal is available for example when we commit by a GUI tool, I think vscode should work...: also it seems when we run out=$(git commit -a -m "asd" 2>&1) the tests [-t 0] && [-t 1] doesn't pass...

[gabyx]

Lets see if all tests work, by me everythin passes for test-alpine-curl.sh

[coveralls]

Pull Request Test Coverage Report for Build 537

• 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.03%) to 87.193%

---

Totals
Change from base Build 532:	0.03%
Covered Lines:	1954
Relevant Lines:	2241

---

💛 - Coveralls

[matthijskooijman]

Shouldn't we only (if and only if) read from /dev/tty when stdin is also a tty meaning the change should be maybe even more strict:

No, as I mentioned in #81, stdin is never a tty when run from git, and if it is, the /dev/tty redirect is not even needed.

[matthijskooijman]

Testing without controlling tty will probably happen already in the testsuite (since I suspect all of that runs without tty). It would be interesting to test this with a tty as well, which would I think require creating a PTY pair so you can run git on one half and send some "interactive" input to the other half.

I googled around a bit on how to do this from bash, which seems non-trivial (a C program needs to do all kinds of setup for this). However, it seems that you can let socat do the heavy lifting, according to https://stackoverflow.com/a/19848887/740048

[gabyx]

Ok to summarize to make sure I understand correctly: My input output knowhow is somewhat newbie: when running any hook we have

• stdin (dont read from that for user input, because of git)
• stdout
• and maybe a controlling terminal which we can get by /dev/tty.

As you experienced it seems that git attaches /dev/null or a pipe mostly to stdin.

• Reading from stdin is never good in that way (because of info which is sent by a pipe for example, I hope this stdin gets inherited to the hooks githooks runs, this should probably be the case...) The test [ -t 0 ] in https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html tests if a file descriptor is associated with a terminal (thats the same as a "controlling terminal" hopefuly). This does not help also.

So, when I want to show a user prompt and answer from the user I can only do this by /dev/tty. There is a another POSIX command tty (https://pubs.opengroup.org/onlinepubs/9699919799/utilities/tty.html#tag_20_135) which returns 0 if stdin is a terminal, but what does that help.

SO the only way is (as you mentioned) to open the file /dev/tty check if is valid.

[gabyx]

I reverted to the initial command by @rycus86 which is more what we want. read -r "$VARIABLE" </dev/tty 2>/dev/null and fixes the error message saying no file descriptor etc...

[matthijskooijman]

Your summary seems correct. Your proposal of just always reading from /dev/tty and masking the error would also seem useful to me (though there is a chance of masking other errors that might have been useful). You did not update the comment yet, so if this is merged, that should be done first.

I'm curious: When /dev/tty is not available, what does the read command return? i.e. what do you get when you put echo $? directly after the read? Knowing that might be helpful in relation to #84.

[gabyx]

@rycus86 yes this PR can go in (no deps) :-). as you mentioned: suppressing the error is #81 eseentially. lets leave it in, should be fine I think.