Re: [PATCH v4 2/3] git-core: Support retrieving passwords with GIT_ASKPASS

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 3, 2010, 19:56 UTC
Message-ID: <7vd3zlqivv.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1267616208-3844-1-git-send-email-lznuaa@gmail.com>

Frank Li <lznuaa@gmail.com> writes:

Show 8 quoted lines

> imap-send and authority http connect reads passwords from an interactive
> terminal. This behavious cause GUIs to hang waiting for git complete.
>
> Fix this problem by allowing a password-retrieving command
> to be specified in GIT_ASKPASS
>
> Signed-off-by: Frank Li <lznuaa@gmail.com>
> ---

It appears that between v3 and v4 you incorporated all the changes J6t suggested. I would have preferred to see you Cc him, so that he can simply respond with "Acked-by:" (or another round of review cycle).

Show 9 quoted lines

> +char *git_getpass(const char *prompt)
> +{
> ...
> +	memset(&pass, 0, sizeof(pass));
> +	pass.argv = args;
> +	pass.out = -1;
> +
> +	if (start_command(&pass))
> +		exit(1);

I understand that you exit silently here because start-command would have given diagnostic messages already.

> +	if (strbuf_read(&buffer, pass.out, 20) < 0)
> +		exit(1);

What about this one? Running out of memory inside strbuf_grow() may have died with message but other error conditions that do give control back to the caller seem pretty quiet to my cursory look...

> +	close(pass.out);
> +
> +	if (finish_command(&pass))
> +		exit(1);

This silent exit is correct, I think; wait_or_whine() called by finish_command() would have reported errors.

> +	strbuf_setlen(&buffer, strcspn(buffer.buf, "\r\n"));
> +
> +	/*it maybe memory leak because getpass return a static buffer*/
> +	return strbuf_detach(&buffer, NULL);

Perhaps keep this value in a "static char *" in the function and free it upon entry on the second call?