Re: [PATCH v3] git-credential-store: skip empty lines and comments from store

From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: "Jeff King" <peff@peff.net>,
	"Carlo Marcelo Arenas Belón" <carenas@gmail.com>,
	git@vger.kernel.org, dirk@ed4u.de, sunshine@sunshineco.com,
	"Stefan Tauner" <stefan.tauner@gmx.at>
Subject: Re: [PATCH v3] git-credential-store: skip empty lines and comments from store
Date: Tue, 28 Apr 2020 09:03:36 -0700	[thread overview]
Message-ID: <xmqqsggn4mxz.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200428052510.GA201501@google.com> (Jonathan Nieder's message of "Mon, 27 Apr 2020 22:25:10 -0700")

Jonathan Nieder <jrnieder@gmail.com> writes:

> I wonder if in addition to the above documentation change we may want
> something guaranteed to catch all cases where people would have
> experienced a regression, like
>
> diff --git i/credential-store.c w/credential-store.c
> index c010497cb21..294e7716815 100644
> --- i/credential-store.c
> +++ w/credential-store.c
> @@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn,
>  	}
>  
>  	while (strbuf_getline_lf(&line, fh) != EOF) {
> -		credential_from_url(&entry, line.buf);
> -		if (entry.username && entry.password &&
> +		if (!credential_from_url_gently(&entry, line.buf, 1) &&
> +		    entry.username && entry.password &&
>  		    credential_match(c, &entry)) {
>  			found_credential = 1;
>  			if (match_cb) {

Hmph, so the idea is, instead of ignoring the potential error
detected by credential_from_url() and using credential when it is
available, we loudly attempt to parse and give warning on malformed
entries when we discard a line?

I think that is an excellent idea.

It would be nicer if we can somehow add where we found the offending
line, e.g.

    /home/me/.gitcredential:396: url has no scheme: ://u:p@host/path

Do we feel it a bit disturbing that u:p is shown in the error
message, without redacting, by the way?