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