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

From: Carlo Arenas <carenas@gmail.com>
Date: May 1, 2020, 02:24 UTC
Message-ID: <CAPUEspiD10bYsCDoHM6PJdidyofNadJL0bexjiNNZCXNU+Ay_Q@mail.gmail.com>
In-Reply-To: <xmqqimhgph3z.fsf@gitster.c.googlers.com>

On Thu, Apr 30, 2020 at 6:40 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

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

indeed, and thanks for the clarification.

you are correct this was a bad idea and is better to warn them instead (even if only during get operations and for the lines that were processed) than my "solution".

FWIW though, my change wasn't going to change the file on disk but only allow the line to be processed.

gave up already on sneakily "fixing" corruption issues after Peff called me on it and yes, another version you will never see had a PARSER_TRIM flag added ;)

Carlo

PS. should we really do the warn even in store/erase operations as a followup or is the warning not useful as is, and probably then all operations should be quiet (as Jonathan suggested originally?) and we could do warn (and maybe fix) in a different change (maybe adding a fsck command of sorts)?

Show 5 quoted lines

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