git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Firmin Martin <firminmartin24@gmail.com>,
	git@vger.kernel.org,
	Johannes Schindelin <johannes.schindelin@gmail.com>,
	Erik Faye-Lund <kusmabite@gmail.com>,
	Denton Liu <liu.denton@gmail.com>
Subject: Re: [PATCH v1 1/8] compat/terminal: let prompt accept input from pipe
Date: Fri, 7 May 2021 00:54:18 -0400	[thread overview]
Message-ID: <YJTH+sTP/O5Nxtp9@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqr1ijwi0i.fsf@gitster.g>

On Fri, May 07, 2021 at 08:37:49AM +0900, Junio C Hamano wrote:

> Firmin Martin <firminmartin24@gmail.com> writes:
> 
> > Currently, git_prompt ignores input coming from anywhere other than
> > terminal (pipe, redirection etc.) meaning that standard prompt
> > auto-answering methods would have no effect:
> >
> >         echo 'Y' | git ...
> >         yes 'Y' | git ...
> >         git ... <input.txt
> >
> > It also prevents git subcommands using git_prompt to be tested using
> > such methods.
> 
> For testing, wouldn't lib-terminal.sh be usable for your purpose?
> If not, what is the reason why it is insufficient?  Can we fix that
> instead?

That doesn't work, because it insists on reading from /dev/tty and not
the pty that lib-terminal will set up as stdin. But...

> Allowing prompter to read from pipe has a big downside in the
> production code: you cannot pipe data into our command, and let it
> ask interactive questions from the end user by opening /dev/tty.

Right. The main purpose of the function was to let git-remote-https,
whose stdin is connected to git-fetch, get a password from the user.
Reading from stdin would break things badly there[1].

Looking at the second patch, the motivation here seems to be to use
git_prompt() for another run-of-the-mill prompt. But the right answer
is: don't do that. In fact, we recently-ish removed a similar case in
97387c8bdd (am: read interactive input from stdin, 2019-05-20) that was
likewise causing problems with the test suite.

I think we might consider renaming git_prompt(), or adding an
explanatory comment above it.

-Peff

[1] Sadly I don't think our test suite could notice the breakage
    introduced by this function. It uses the askpass feature to avoid
    triggering this code at all, because of course we can not reliably
    read from /dev/tty in the script. But with just this patch applied,
    and no credential helpers defined, trying "git ls-remote
    https://github.com/you/some-private-repo" shows the problem: you get
    prompted, but it never reads your input.

  reply	other threads:[~2021-05-07  4:54 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-06 16:50 [PATCH v1 0/8] format-patch: introduce --confirm-overwrite Firmin Martin
2021-05-06 16:50 ` [PATCH v1 1/8] compat/terminal: let prompt accept input from pipe Firmin Martin
2021-05-06 23:37   ` Junio C Hamano
2021-05-07  4:54     ` Jeff King [this message]
2021-05-07  5:25       ` Junio C Hamano
2021-05-10  4:18       ` Firmin Martin
2021-05-10 21:32         ` Jeff King
2021-05-11  3:38           ` Junio C Hamano
2021-05-11  6:10             ` Jeff King
2021-05-11  6:17               ` Junio C Hamano
2021-05-11  6:37                 ` Jeff King
2021-05-06 16:50 ` [PATCH v1 2/8] format-patch: confirmation whenever patches exist Firmin Martin
2021-05-06 23:48   ` Junio C Hamano
2021-05-10  3:30     ` Firmin Martin
2021-05-10  7:32       ` Junio C Hamano
2021-05-11  3:17         ` Firmin Martin
2021-05-06 16:50 ` [PATCH v1 3/8] format-patch: add config option confirmOverwrite Firmin Martin
2021-05-06 16:50 ` [PATCH v1 4/8] format-patch: add the option --confirm-overwrite Firmin Martin
2021-05-06 16:50 ` [PATCH v1 5/8] t4014: test patches overwrite confirmation Firmin Martin
2021-05-06 16:51 ` [PATCH v1 6/8] t4014: fix tests overwriting cover letter in silent Firmin Martin
2021-05-06 16:51 ` [PATCH v1 7/8] doc/format-patch: describe --confirm-overwrite Firmin Martin
2021-05-07  3:32   ` Bagas Sanjaya
2021-05-10  4:22     ` Firmin Martin
2021-05-06 16:51 ` [PATCH v1 8/8] config/format: describe format.confirmOverwrite Firmin Martin
2021-05-06 22:33 ` [PATCH v1 0/8] format-patch: introduce --confirm-overwrite Junio C Hamano
2021-05-11  0:18   ` Firmin Martin
2021-05-07  1:46 ` Felipe Contreras
2021-05-07  8:55   ` Denton Liu
2021-05-11  1:09     ` Firmin Martin
2021-05-11  5:12       ` Felipe Contreras
2021-05-11  5:03     ` Felipe Contreras
2021-05-07 14:02   ` Sergey Organov
2021-05-11  0:46   ` Firmin Martin
2021-05-10 12:02 ` Ævar Arnfjörð Bjarmason

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=YJTH+sTP/O5Nxtp9@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=firminmartin24@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmail.com \
    --cc=kusmabite@gmail.com \
    --cc=liu.denton@gmail.com \
    --subject='Re: [PATCH v1 1/8] compat/terminal: let prompt accept input from pipe' \
    /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).