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

From: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, dirk@ed4u.de, sunshine@sunshineco.com
Subject: Re: [PATCH v2] git-credential-store: skip empty lines and comments from store
Date: Mon, 27 Apr 2020 05:25:20 -0700	[thread overview]
Message-ID: <20200427122520.GA61348@Carlos-MBP> (raw)
In-Reply-To: <20200427115223.GA1718141@coredump.intra.peff.net>

On Mon, Apr 27, 2020 at 07:52:23AM -0400, Jeff King wrote:
> 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.

agree on that, which is why I have to admit I couldn't find the right
wording for the previous paragraph, and also why I didn't pinpoint a
specific commit as the one that introduced a bug.

it was instead just meant to describe a "current state of affairs" and
I was hoping the documentation entry will help guide future users to be
more careful with this file.

> 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.

I think that is a matter of semantics, because for some users it seems
like a regression on the last bugfix and therefore might be used (albeit
IMHO incorrectly) as an excuse not to upgrade, which will be even worst.

so yes, I am implementint this "feature" to prevent them to have an
excuse but it also means we would most likely need to fastrack this
into maint.

> > 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?

I know, but commiting a line with 1 tab and 4 empty spaces instead of
using Q seemed even worst and q_to_cr almost fits the bill and might
even make this test "windows compatible" for once ;)

will rewrite then using test_write_lines and I hope it is still as
readable without having to resort to the originl ugly chain of echo

Carlo