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

From: Junio C Hamano <gitster@pobox.com>
To: "Carlo Marcelo Arenas Belón" <carenas@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [RFC PATCH v10 2/1] credential-store: warn also for store and erase
Date: Thu, 30 Apr 2020 22:35:17 -0700	[thread overview]
Message-ID: <xmqqa72sp696.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200501051847.31803-1-carenas@gmail.com> ("Carlo Marcelo Arenas =?utf-8?Q?Bel=C3=B3n=22's?= message of "Thu, 30 Apr 2020 22:18:47 -0700")

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.

...

> @@ -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...