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

[Junio C Hamano <gitster@pobox.com>]

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> 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.

You are forgetting why we are fixing credential-store, aren't you?

It is primarily to help those who damaged their files by editing,
and introducing cruft that cannot be parsed, from a stricter parsing
introduced recently in 2.17.4 and above.  Without the fix, they
cannot operate with the store they already have.

Now, think about those users who saved their file, after adding CR
at the end of each line, but didn't do any other edit (like adding a
blank line or "# comment").  It may have happened 3 years ago.  What
did they see from the system back then?  It may have happened 3
minutes ago.  What would they see with the stricter parser now?

With or without the recent parser change, they would have seen that
these lines no longer match the URLs they wanted to match, but the
credential store does not die on them for malformed lines, no?

In other words, the stricter parsing did nothing to these users.

In fact, those users who added CR at the end of each line 3 years
ago may have even depended on the disappearance of these entries for
all these years.  Maybe lines that record their ancient passwords
for sites are still buried in the later parts of the file, with CR,
but doing no harm because these lines do not match anything.  These
users may have changed their password since then and wrote new
records with "credential store", and these new records are stored
without CR at the end of the line, so they match the URLs.

By using the non _lf() variant, you are suddenly resurrecting these
old records that the users thought are already gone and have been
causing no harm to them.  Do we know that resurrecting these old
records is a good thing to do?  I don't.  For example, once the user
decides to "sort" the file (after all, we are talking about users
who edit the file, so we cannot assume they won't do so), they would
end up with duplicate records that record two passwords to the same
site and they cannot tell which one is current, as you even lost the
CR at the end of line that would have told you which ones are
broken.

In short, you wouldn't know what ramification it has by suddenly
using non _lf() variant.  And it has nothing to do with the fix we
are trying to make.

When fixing something, it is tempting to see if you can cover a bit
more area with the same change.  But you should make it a habit to
stick to the absolute minimum first for safety.  When contemplating
to step out of the absolute minimum, you need to think twice to see
if that extra "fix" really fixes, or if it potentially harms.

And I am not convinced that turning CRLF into LF in this case is a
good change.  In any case, it certainly does not belong to the same
commit as the one that fixes the fallout from stricter parser
introduced in 2.17.4 and above.

Thanks.