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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 4, 2010, 18:26 UTC
Message-ID: <7vd3zk6j01.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1267713364-2976-1-git-send-email-lznuaa@gmail.com>

Frank Li <lznuaa@gmail.com> writes:

> imap-send and authority http connect reads passwords from an interactive
> terminal. This behavious cause GUIs to hang waiting for git complete.

"authority http"?  Perhaps...

    git tries to read a password from the terminal in imap-send and
    talking to http server that requires authentication.

    When a GUI is driving git, however, the end user is not paying
    attention to the terminal (there may not even be a terminal).  GUI
    would appear to hang forever.

Show 5 quoted lines

> Fix this problem by allowing a password-retrieving command
> to be specified in GIT_ASKPASS
>
> Signed-off-by: Frank Li <lznuaa@gmail.com>
> ---

Here after three-dash lines, could you describe the changes (what changed and why) since the last round, when sending a revised version? Saying "This round addressed such and such issues raised and I think it is ready for inclusion" (when you think it is ready) would also help.

Show 5 quoted lines

> +	if (start_command(&pass))
> +		exit(1);
> +
> +	if (strbuf_read(&buffer, pass.out, 20) < 0)
> +		die("fail get password from %s\n", askpass);

"failed to get..."

Show 12 quoted lines

> +	close(pass.out);
> +
> +	if (finish_command(&pass))
> +		exit(1);
> +
> +	strbuf_setlen(&buffer, strcspn(buffer.buf, "\r\n"));
> +
> +	if (pd) {
> +		free(pd);
> +		pd = NULL;
> +	}
> +	/*it maybe memory leak because getpass return a static buffer*/

Hmph, why not do this at the beginning? Is the "maybe leak" comment still valid?