Re: [PATCH] drop unnecessary copying in credential_ask_one

From: Jeff King <peff@peff.net>
Date: Jan 7, 2014, 17:50 UTC
Message-ID: <20140107175009.GA19691@sigill.intra.peff.net>
In-Reply-To: <xmqq7gaiqjzw.fsf@gitster.dls.corp.google.com>

On Thu, Jan 02, 2014 at 11:08:51AM -0800, Junio C Hamano wrote:

Show 11 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > ... But the test suite, of course, always uses askpass because it
> > cannot rely on accessing a terminal (we'd have to do some magic with
> > lib-terminal, I think).
> >
> > So it doesn't detect the problem in your patch, but I wonder if it is
> > worth applying the patch below anyway, as it makes the test suite
> > slightly more robust.
> 
> Sounds like a good first step in the right direction.  Thanks.

I took a brief look at adding "real" terminal tests for the credential code using our test-terminal/lib-terminal.sh setup. Unfortunately, it falls short of what we need.

test-terminal only handles stdout and stderr streams as fake terminals. We could pretty easily add stdin for input, as it uses fork() to work asynchronously. But the credential code does not actually read from stdin. It opens and reads from /dev/tty explicitly. So I think we'd have to actually fake setting up a controlling terminal. And that means magic with setsid() and ioctl(TIOCSCTTY), which in turn sounds like a portability headache.

So it's definitely possible under Linux, and probably under most Unixes. But I'm not sure it's worth the effort, given that review already caught the potential bug here.

Another option would be to instrument git_terminal_prompt with a mock-terminal interface (say, reading from a file specified in an environment variable). But I really hate polluting the code with test cruft, and it would not actually be testing an interesting segment of the code, anyway.

-Peff