Re: [RFC PATCH v6 2/2] credential-store: warn for any incomplete credentials instead of usingy

["Carlo Marcelo Arenas Belón" <carenas@gmail.com>]

On Wed, Apr 29, 2020 at 02:12:21PM -0700, Junio C Hamano wrote:
> Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:
> 
> > originally any credential found was tried for matching as far as it had
> > a username and password, but that resulted in fatal errors as the rules
> > were harden.
> 
> harden -> hardened
> 
> > now that we have a way to report malformed credentials, use it to notify
> > the user when username/password was missing, instead of just silently
> > skipping.
> 
> Sorry, but isn't that what happend already in the previous step?
> What are you ordering the codebase (after applying the previous
> stpe) do further?  It already is "using it to notify the user when
> username and/or password is missing".

not sure I follow, but the current code (as well as the one after patch 1)
just silently ignores any credential that was missing username and password
since the _gently version of the call will only really fail for a missing
protocol.

so patch1 will notify ONLY(*) when the credential is so malformed that we can't
figure out which protocol to use (like empty lines and comments)

(*) it will also fail if we have a '\n' in one of the components but that
is hopefully not relevant here.

> > diff --git a/credential-store.c b/credential-store.c
> > index 1cc5ca081a..53f77ff6f5 100644
> > --- a/credential-store.c
> > +++ b/credential-store.c
> > @@ -26,9 +26,10 @@ static int parse_credential_file(const char *fn,
> >  
> >  	while (strbuf_getline_lf(&line, fh) != EOF) {
> >  		lineno++;
> > -		if (!credential_from_url_gently(&entry, line.buf, 1)) {
> > -			if (entry.username && entry.password &&
> > -				credential_match(c, &entry)) {
> > +		if (!credential_from_url_gently(&entry, line.buf, 1) &&
> > +			((entry.host && *entry.host) || entry.path) &&
> > +			entry.username && entry.password) {
> > +			if (credential_match(c, &entry)) {
> 
> ... this makes the code even harder to follow than it already was
> after the previous step.  In the previous step, at least it was
> clear that we'd catch *all* the well-formed/parseable input will
> come into the first if () {...} block, but with the extra logic,
> that is no longer true.  Even a line that is well formed may be
> bypassed from matching and will be fed to "else" side.

I think it will be clearer with the reformatting you suggested and that I
agree is needed, but wanted to make sure first that we agreed on the direction.

FWIW the test cases show the format of the lines affected and while they are
parseable enough that were processed they were obviously not valid, specially
considering the updated rules.

maybe we should make the _gently version less gently instead?, so that callers
will be able to know they have an incomplete credential for matching after
calling it without having to do their own check?

if the later, I am affraid this will be a far bigger change, which is why I
would rather duplicate the rule checking for at least the first iteration.

Carlo