On Mon, Apr 27, 2020 at 10:25:10PM -0700, Jonathan Nieder wrote:
> I wonder if in addition to the above documentation change we may want
> something guaranteed to catch all cases where people would have
> experienced a regression, like
>
> diff --git i/credential-store.c w/credential-store.c
> index c010497cb21..294e7716815 100644
> --- i/credential-store.c
> +++ w/credential-store.c
> @@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn,
> }
>
> while (strbuf_getline_lf(&line, fh) != EOF) {
> - credential_from_url(&entry, line.buf);
> - if (entry.username && entry.password &&
> + if (!credential_from_url_gently(&entry, line.buf, 1) &&
> + entry.username && entry.password &&
> credential_match(c, &entry)) {
> found_credential = 1;
> if (match_cb) {
>
> And then we can tighten the handling of unrecognized lines to first
> warn and then error out, as a controlled change that doesn't lead
> people to regret updating git.
I like that solution, as it mostly brings us back to the original
behavior, as weird or unexpected as it was.
The only I think would be different is:
://user:pass@xxxxxxxxxxx
which I think the old code would have treated as matching any protocol
(and the new code will reject as a bogus URL).
I wondered if you'd need to free the partially-parsed struct fields on
failure, but the answer is no; we'd clear it the next time through the
loop, or at the very end of the function.
-Peff
