Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store

From: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Date: May 1, 2020, 00:30 UTC
Message-ID: <20200501003041.GD33264@Carlos-MBP>
In-Reply-To: <xmqq1ro4sp1p.fsf@gitster.c.googlers.com>

On Thu, Apr 30, 2020 at 01:21:06PM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
> > @@ -23,17 +34,24 @@ static int parse_credential_file(const char *fn,
> >  		return found_credential;
> >  	}
> >  
> > -	while (strbuf_getline_lf(&line, fh) != EOF) {
> > -		credential_from_url(&entry, line.buf);
> > -		if (entry.username && entry.password &&
> > -		    credential_match(c, &entry)) {
> > +	while (strbuf_getline(&line, fh) != EOF) {
> 
> Hmph, I probably have missed some discussion that happened in the
> last few days, but from the use of _lf() in the original, I sense
> that it is very much deliberate that the file format is designed to
> be LF delimited records, *not* a text file in which each line is
> terminated with some end-of-line marker that is platform dependent.
> IOW, an explicit use of _lf() shouts, at least to me, that we want a
> sequence of LF terminated records even on Windows and Macs.

apologize for the confusion, the only discussion was the one I had with myself late at night when I realized that specific corruption was not being detected, and might be related to issues that seemed to be common[1]

the problem is that practically speaking, if users in Windows and Macs edited the file they "might" had saved lines with the wrong line ending (*) (part of the reason I added a warning about "encoding" to the documentation), and those are difficult to "see" as invalid.

using the non _lf() version ensures any trailing '\r' character would get removed from the credential and allow us to use them, instead of silently failing.

originally I added a explicit check for it, which I assume you would prefer but ..

> So, I am not sure why this change is desirable.

to give users immediate relief from what seems to be a commonly seen issue.

IMHO after we get this released out and they see the updated documentation explaining the risks, and some learn how to fix their credential files ( and hopefully use an editor that lets them see the problem); we could then add also a detection for this edge case and go back to _lf()

I also had to admit I might had overreacted, as I was testing before v8 with a very corrupted file and was seeing warnings twice on each operation and somehow even got myself into the original fatal, which I had to admit I can't replicate now after some needed rest.

Carlo

[1] https://github.com/desktop/desktop/issues/9597

(*) textedit, which comes with macOS doesn't even write an EOF record when
    creating files, for example, and that behaviour seems to be common for
    most other native editors but a credential line WITHOUT line ending works