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

From: Jeff King <peff@peff.net>
To: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
Cc: Jonathan Nieder <jrnieder@gmail.com>,
	Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org, dirk@ed4u.de, sunshine@sunshineco.com,
	Stefan Tauner <stefan.tauner@gmx.at>
Subject: Re: [PATCH v3] git-credential-store: skip empty lines and comments from store
Date: Tue, 28 Apr 2020 04:16:27 -0400	[thread overview]
Message-ID: <20200428081627.GB2381876@coredump.intra.peff.net> (raw)
In-Reply-To: <20200428071802.GF61348@Carlos-MBP>

On Tue, Apr 28, 2020 at 12:18:02AM -0700, Carlo Marcelo Arenas Belón wrote:

> > >  	while (strbuf_getline_lf(&line, fh) != EOF) {
> > > -		credential_from_url(&entry, line.buf);
> > > -		if (entry.username && entry.password &&
> > > +		if (!credential_from_url_gently(&entry, line.buf, 1) &&
> > > +		    entry.username && entry.password &&
> > >  		    credential_match(c, &entry)) {
> > >  			found_credential = 1;
> > >  			if (match_cb) {
> > > 
> > > And then we can tighten the handling of unrecognized lines to first
> > > warn and then error out, as a controlled change that doesn't lead
> > > people to regret updating git.
> > 
> > I like that solution, as it mostly brings us back to the original
> > behavior, as weird or unexpected as it was.
> 
> I like this version better as well, and we could even reuse my test
> case.
> 
> it wouldn't cover cases where there were leading spaces/tabs around
> the credential which I have to admit I liked just because it is
> more robust to bad input, and there is no sane way now to tell the
> user that there is invalid data anyway, but I am ok eitherway.

I think if we're discouraging people from hand-editing the file, then
that feature would be going in the wrong direction anyway.

Did you or Jonathan want to wrap it up with the test and commit message?

-Peff