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

From: Jeff King <peff@peff.net>
Date: Apr 27, 2020, 11:52 UTC
Message-ID: <20200427115223.GA1718141@coredump.intra.peff.net>
In-Reply-To: <20200427084235.60798-1-carenas@gmail.com>

On Mon, Apr 27, 2020 at 01:42:35AM -0700, Carlo Marcelo Arenas Belón wrote:

> with the added checks for invalid URLs in credentials, any locally
> modified store files which might have empty lines or even comments
> were reported failing[1] to parse as valid credentials.

Those were never supposed to work. I'm mildly surprised that they did. It looks like the username/password check here is what prevented us from matching an empty line against a very broad pattern:

          while (strbuf_getline_lf(&line, fh) != EOF) {
                  credential_from_url(&entry, line.buf);
                  if (entry.username && entry.password &&
                      credential_match(c, &entry)) {
                          found_credential = 1;

And there was no such thing as a comment character. E.g., a "commented-out" url like this:

  $ echo "#https://user:pass@example.com/" >creds

would still be matched:

  $ echo host=example.com | git credential-store --file creds get
  host=example.com
  username=user
  password=pass

I guess I'm not really opposed to adding this as a feature, but I think the justification should be "because it is somehow useful" and not because it's a bugfix.

Show 15 quoted lines

> diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh
> index d6b54e8c65..0d13318255 100755
> --- a/t/t0302-credential-store.sh
> +++ b/t/t0302-credential-store.sh
> @@ -120,4 +120,23 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi
>  	test_must_be_empty "$HOME/.config/git/credentials"
>  '
>  
> +test_expect_success 'get: allow for empty lines or comments in store file' '
> +	q_to_cr >"$HOME/.git-credentials" <<-\EOF &&
> +	#this is a comment and the next line contains leading spaces
> +	    Q
> +	https://user:pass@example.com
> +	Q
> +	EOF

q_to_cr is a little weird here, as we wouldn't expect there to be CRs in the file. They do get removed by strbuf_trim(), even in non-comment lines. But perhaps "sed s/Q//" would accomplish the same thing (making the whitespace more visible) without making anyone wonder whether the CR is an important part of the test?

-Peff