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

From: Junio C Hamano <gitster@pobox.com>
Date: May 1, 2020, 01:40 UTC
Message-ID: <xmqqimhgph3z.fsf@gitster.c.googlers.com>
In-Reply-To: <20200501003041.GD33264@Carlos-MBP>

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

Show 8 quoted lines

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