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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 29, 2020, 21:17 UTC
Message-ID: <xmqqftcmuh3a.fsf@gitster.c.googlers.com>
In-Reply-To: <xmqqo8rauhoh.fsf@gitster.c.googlers.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 24 quoted lines

>>  	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)) {
>> +				found_credential = 1;
>> +				if (match_cb) {
>> +					match_cb(&entry);
>> +					break;
>> +				}
>>  			}
>>  		}
>> +		else
>> +			warning(_("ignoring invalid credential in %s:%d"),
>> +				fn, lineno);
>> +		if (!found_credential && other_cb)
>>  			other_cb(&line);
>>  	}
>
> The above is harder to follow than necessary.
>
> 	while (... != EOF) {
> 		lineno++;
> 		if (credential is not well formed) {

After reading your [2/2], I think this part deserves a bit of explanation. By "is not well formed", what I mean is not just credential_from_url_gently() returns non-zero, but also user/pass are specified. And your step [2/2] may make the condition for a line to be "well formed" even stricter by requiring at least one of host or path, and that would also belong to this test.

> 			warning(_("ignoring..."));
> 		} else if (the entry matches) {

And the "entry matches" test here would only be "what does credential_match() say?"

Show 16 quoted lines

> 			found_credential = 1;
> 			if (match_cb) {
> 				match_cb(&entry);
> 				break; /* stop at the first match */
> 			}
> 			continue;
> 		}
>
>                 if (other_cb)
> 			other_cb(&line);
> 	}
>
> may make the intention a bit clearer, with the loud "continue" inside.
>
>  (1) we give warning for malformed one; and
>  (2) we do not let other_cb touch a matching entry.