Re: [RFC PATCH v10 2/1] credential-store: warn also for store and erase

From: Junio C Hamano <gitster@pobox.com>
Date: May 1, 2020, 05:35 UTC
Message-ID: <xmqqa72sp696.fsf@gitster.c.googlers.com>
In-Reply-To: <20200501051847.31803-1-carenas@gmail.com>

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

> done in a hacky way and only as a POC, but at least works, if we
> are still thinking that warning should be at least comprehensive
> for the first iteration.

...

Show 19 quoted lines

> @@ -25,7 +23,7 @@ static int parse_credential_file(const char *fn,
>  	struct strbuf line = STRBUF_INIT;
>  	struct credential entry = CREDENTIAL_INIT;
>  	int found_credential = 0;
> -	int lineno = 0;
> +	int lineno = initial_line;
>  
>  	fh = fopen(fn, "r");
>  	if (!fh) {
> @@ -40,8 +38,7 @@ static int parse_credential_file(const char *fn,
>  		if (strchr(line.buf, '\r') ||
>  			credential_from_url_gently(&entry, line.buf, 1) ||
>  			!valid_credential(&entry)) {
> -			if (flags & PARSE_VERBOSE)
> -				warning(_("%s:%d: ignoring invalid credential"),
> +			warning(_("%s:%d: ignoring invalid credential"),
>  					fn, lineno);
>  		} else if (credential_match(c, &entry)) {
>  			found_credential = 1;

I am not sure how this would correctly count when "store" and "erase" is used. During "store", the initial_line may let us correctly count the new line, and as long as we keep copying, our output line number would stay to be correct, as we increment line by one every time we read in, but as soon as we see the older version of the credential record this "store" is updating, i.e. when the credential_match() reports a match, we will discard the line we read by hitting the "continue" and not calling other_cb(). We increment line, but we don't output a line when that happens, so any warning after that point will be out of sync, no? And "erase" is the same deal. It behaves the same way as "store" except that "erase" does not emit a replacement record at the beginning. As soon as the matching record from the input is dropped, our line number will be out of sync.

In any case, I think it probably is a good idea to start with a version that ignores without warning, and I had an impression that you were on the same page from reading your response to v9 review, so...