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@xxxxxxxxxxx/"; >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.
> 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@xxxxxxxxxxx
> + 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
