Frank Li schrieb:
> connect.c | 40 ++++++++++++++++++++++++++++++++++++++++
> http.c | 4 ++--
> imap-send.c | 2 +-
I don't see any header file changes. Don't you get warnings about an
undeclared function git_getpass() at the call sites?
> +char *git_getpass(char *prompt)
char *git_getpass(const char *prompt)
> + askpass = getenv("GIT_ASKPASS");
> + if (askpass && strlen(askpass) != 0) {
> + args[0] = getenv("GIT_ASKPASS");
if (askpass && *askpass) {
args[0] = askpass;
BTW, to save a level of indentation, you could handle the "trivial" case
early like this:
if (!askpass || !*askpass)
return get_pass(prompt);
and continue without an 'else' branch.
> + args[1] = prompt;
> + args[2] = NULL;
> +
> + memset(&pass, 0, sizeof(pass));
> + pass.argv = args;
> + pass.out = -1;
> + pass.no_stdin = 1;
> + pass.no_stderr = 1;
Is it such a good idea to redirect stdin and stderr to /dev/null? What if
my password prompt program depends on them? I think it should not matter
for your use-case, where a GUI is invoked, to just inherit all channels.
OTOH, it may be worthwhile to set
pass.use_shell = 1;
to allow commands that are not just a single plain word. But perhaps this
has security implications - I don't know.
> +
> + if (start_command(&pass)) {
> + error("could not run %s\n", askpass);
> + return getpass(prompt);
I don't think this is a good idea. The user instructed to use GIT_ASKPASS,
and you fall back to asking a password from the terminal. I think the most
sensible thing to do here is to 'exit(1)' (start_command has already
printed an error message that included the command), because there are
callers that do not expect NULL.
> + }
> +
> + strbuf_read(&buffer, pass.out, 20);
> + close(pass.out);
> + for (i = 0; i < buffer.len; i++)
> + if (buffer.buf[i] == '\n' || buffer.buf[i] == '\r') {
> + buffer.buf[i] = '\0';
> + buffer.len = i;
> + }
> + return strbuf_detach(&buffer, NULL);
You don't call finish_command() anywhere. Call it after the close() call.
> +
> + } else {
> + return getpass(prompt);
You handle the return value in different ways. getpass() returns a pointer
to a static buffer, but in the 'then' branch you return an allocated
buffer. Not that it matters a lot, though. You could add a comment that
you are aware that the memory is leaked.
> + }
> + return NULL;
What is this good for?
-- Hannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html