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

[Junio C Hamano <gitster@pobox.com>]

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> with the added checks for invalid URLs in credentials, any locally
> modified store files which might have empty lines or even comments
> were reported[1] failing to parse as valid credentials.
>
> instead of doing a hard check for credentials, do a soft one and
> warn the user so any invalid entries could be corrected.

Yeah, this version looks be best so far.  No need to worry about
redacting anything; we are dealing with folks who have edited
the file, and they shouldn't have any trouble going to the line with
a problem given an exact line number.

> diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt
> index 76b0798856..30d498fe54 100644
> --- a/Documentation/git-credential-store.txt
> +++ b/Documentation/git-credential-store.txt
> @@ -95,8 +95,16 @@ https://user:pass@example.com
>  ------------------------------
>  
>  No other kinds of lines (e.g. empty lines or comment lines) are
> -allowed in the file, even though some may be silently ignored. Do
> -not view or edit the file with editors.
> +allowed in the file, even though historically the parser was very
> +lenient and some might had been silently ignored.
> +
> +Do not edit the file with editors as it could compromise the validity
> +of your credentials by sometimes subtle formatting issues (like spaces)

I do not think dropping "view or" is warranted.  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.

> +In cases where those formatting issues are detected during parsing a
> +warning indicating the line found will be printed to stderr so it can
> +be corrected at your earliest convenience, but the remaining valid
> +credentials will be used to try to find a match as described below.

I am often just as guilty, but the above four lines in a single
sentence is hard to read, especially without minimum number of
comma.  With a comma after "during parsing", and another after "to
stderr", might make it more readable, but at that point, it would
become far more readable if we split them into two sentences.

Perhaps

	An unparseable line is ignored, and a warning message points
	out the line number the problematic line appears in (you may
	want to delete them with an editor).

>  	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) {
			warning(_("ignoring..."));
		} else if (the entry matches) {
			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.