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

From: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Date: Apr 30, 2020, 01:00 UTC
Message-ID: <20200430010024.GC33264@Carlos-MBP>
In-Reply-To: <xmqqlfmdua5o.fsf@gitster.c.googlers.com>

On Wed, Apr 29, 2020 at 04:47:31PM -0700, Junio C Hamano wrote:

> > instead of doing a hard check for credentials, do a soft one and
> > warn the user so any invalid entries could be corrected.
> 
> instead -> instead

;)

Show 10 quoted lines

> I do not think dropping "view or" is justifiable.  There is no need
> to invite the risk of opening with the intention to only view and
> then end up saving a modification.  In other words, do not encourage
> use of an *editor* in any way.
> 
> > +An unparseable or otherwise invalid line is ignored, and a warning
> > +message points out the problematic line number and file it appears in.
> 
> OK.  You didn't want to tell them they can remove the problematic
> line as a whole with their editor?

someone said "do not encourage use of an *editor* in any way" just a few lines before ;)

Show 13 quoted lines

> > +	test_config credential.helper store &&
> > +	git credential fill <<-\EOF >stdout 2>stderr &&
> > +	protocol=https
> > +	host=example.com
> > +	EOF
> > +	test_cmp expect-stdout stdout &&
> > +	test_i18ngrep "ignoring invalid credential" stderr &&
> > +	test_line_count = 3 stderr
> 
> The "ignoring invalid credential" message could be translated into
> two or more lines, but I think that is worrying too much about
> theoretical possibility, so checking line count would probably be
> sufficient.

yes, and specially considering that if I even ended up adding a -c option to test_i18ngrep as I intended originally we will still the same issue.

what is the preferred way to do something like this, or is it better to add a check for each line formatting issue this is handling?

Carlo