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

From: Eric Sunshine <sunshine@sunshineco.com>
To: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
Cc: Jeff King <peff@peff.net>, Git List <git@vger.kernel.org>,
	Dirk <dirk@ed4u.de>
Subject: Re: [PATCH v2] git-credential-store: skip empty lines and comments from store
Date: Mon, 27 Apr 2020 10:43:01 -0400	[thread overview]
Message-ID: <CAPig+cRy9T43yXsuj-_vPJoROe8xYRMExxnCxRmG6kij+ykxKg@mail.gmail.com> (raw)
In-Reply-To: <20200427122520.GA61348@Carlos-MBP>

On Mon, Apr 27, 2020 at 8:25 AM Carlo Marcelo Arenas Belón
<carenas@gmail.com> wrote:
> 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:
> > > +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 ;)

Hmm? In the proposal[1], q_to_tab() was used, not q_to_cr(), and
q_to_tab() seemed to fit quite well as a replacement for the original
'echo' chain[2] which emitted one whitespace-only line. Substituting
that whitespace-only line with "Q" (which gets translated into a TAB)
seems to fit much better than q_to_cr().

> 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

test_write_lines() doesn't do a very good job of representing -- at a
glance -- what the "expected" output should be. The q_to_tab() block
made it much  more clear.

(Having said that, it's probably not worth another re-roll just to change that.)

[1]: https://lore.kernel.org/git/CAPig+cR8HKcbNxxw65ERz0iHvnO5aC6RXvF9NjvFTySXpcHCSQ@mail.gmail.com/
[2]: https://lore.kernel.org/git/20200426234750.40418-1-carenas@gmail.com/