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

["Carlo Marcelo Arenas Belón" <carenas@gmail.com>]

On Thu, Apr 30, 2020 at 01:21:06PM -0700, Junio C Hamano wrote:
> 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