On Thu, Apr 30, 2020 at 01:21:06PM -0700, Junio C Hamano wrote:
> Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx> 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
