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

From: Jeff King <peff@peff.net>
Date: Apr 28, 2020, 05:41 UTC
Message-ID: <20200428054155.GB2376380@coredump.intra.peff.net>
In-Reply-To: <20200428052510.GA201501@google.com>

On Mon, Apr 27, 2020 at 10:25:10PM -0700, Jonathan Nieder wrote:

Show 23 quoted lines

> 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) {
> 
> And then we can tighten the handling of unrecognized lines to first
> warn and then error out, as a controlled change that doesn't lead
> people to regret updating git.

I like that solution, as it mostly brings us back to the original behavior, as weird or unexpected as it was.

The only I think would be different is:

  ://user:pass@example.com

which I think the old code would have treated as matching any protocol (and the new code will reject as a bogus URL).

I wondered if you'd need to free the partially-parsed struct fields on failure, but the answer is no; we'd clear it the next time through the loop, or at the very end of the function.

-Peff