From: Eric Sunshine <email@example.com> To: Orgad Shaneh <firstname.lastname@example.org> Cc: Orgad Shaneh via GitGitGadget <email@example.com>, Git List <firstname.lastname@example.org> Subject: Re: [PATCH v4 1/2] hooks: allow input from stdin for commit-related hooks Date: Fri, 20 Nov 2020 01:38:58 -0500 [thread overview] Message-ID: <CAPig+cS5BUCaFN=MN+7gSTbvskffRdTJOgck6TrRRacxCc_CwA@mail.gmail.com> (raw) In-Reply-To: <CAGHpTBKHmdjqrz1ABdGUUz7AwcixU_VBy1DQzybpFizqVo8C7A@mail.gmail.com> On Fri, Nov 20, 2020 at 12:23 AM Orgad Shaneh <email@example.com> wrote: > On Thu, Nov 19, 2020 at 11:23 PM Eric Sunshine <firstname.lastname@example.org> 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.)
next prev parent reply other threads:[~2020-11-20 6:41 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-11-17 15:02 [PATCH] hooks: allow input from stdin Orgad Shaneh via GitGitGadget 2020-11-17 19:59 ` Junio C Hamano 2020-11-19 15:50 ` [PATCH v2] " Orgad Shaneh via GitGitGadget 2020-11-19 15:56 ` [PATCH v3] hooks: allow input from stdin for commit-related hooks Orgad Shaneh via GitGitGadget 2020-11-19 19:16 ` Junio C Hamano 2020-11-19 20:41 ` Orgad Shaneh 2020-11-19 20:56 ` [PATCH v4 0/2] " Orgad Shaneh via GitGitGadget 2020-11-19 20:56 ` [PATCH v4 1/2] " Orgad Shaneh via GitGitGadget 2020-11-19 21:23 ` Eric Sunshine 2020-11-19 21:32 ` Junio C Hamano 2020-11-20 5:23 ` Orgad Shaneh 2020-11-20 6:38 ` Eric Sunshine [this message] 2020-11-20 6:48 ` Eric Sunshine 2020-11-20 7:16 ` Orgad Shaneh 2020-11-20 18:13 ` Junio C Hamano 2020-11-20 10:59 ` Ævar Arnfjörð Bjarmason 2020-11-20 12:34 ` Orgad Shaneh 2020-11-19 20:56 ` [PATCH v4 2/2] commit: fix stdin conflict between message and hook Orgad Shaneh via GitGitGadget 2020-12-09 20:06 ` [PATCH v5 0/2] hooks: allow input from stdin for commit-related hooks Orgad Shaneh via GitGitGadget 2020-12-09 20:06 ` [PATCH v5 1/2] hooks: lay foundations for passing stdin to hooks Orgad Shaneh via GitGitGadget 2020-12-09 20:06 ` [PATCH v5 2/2] hooks: allow input from stdin for commit-related hooks Orgad Shaneh via GitGitGadget 2020-12-09 22:37 ` Junio C Hamano
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAPig+cS5BUCaFN=MN+7gSTbvskffRdTJOgck6TrRRacxCc_CwA@mail.gmail.com' \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH v4 1/2] hooks: allow input from stdin for commit-related hooks' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Code repositories for project(s) associated with this 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).