Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 30, 2020, 21:14 UTC
Message-ID: <xmqq7dxwr80o.fsf@gitster.c.googlers.com>
In-Reply-To: <xmqq1ro4sp1p.fsf@gitster.c.googlers.com>

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

[part of the commit log message]

>> Warn the user indicating the filename and line number so any invalid
>> entries could be corrected but continue parsing until a match is
>> found or all valid credentials are processed.

We should say "Do so only during the 'get' operation; give up giving any warning for 'erase' or 'store' operation, as keeping track of the line number of the input file, while having to issue a warning that has the line number of the output file, is too hard for us" or something like that here.

I suspect that it might not be "too hard", but we'd need to rename other_cb to a more specific name first and limit the possibility of repurposing parse_credential_file().

For example, other_cb can become "copy_cb", we declare that the function works in two (and no other) modes, one is to look-up (which is read-only so we report the input line number in our warning messges) and the other is to copy-out-with-filtering (which will leave a file that is different from the input, so we report the output line number).

To support the copy-out-wiht-filtering mode, we pass the starting line number into the function (i.e. 'store' may store a new line, so the first line copied out to the file may be the second line in the output), and count the output lines there:

	if (other_cb) {
		lines++;
		other_cb(&line);
	}

and of course we won't increment lines++ when we saw a match in the copy-out-with-filtering mode. In look-up mode (i.e. copy_cb == NULL), we count the input lines just like your code.

But I suspect that it may not be worth doing. But if we decide not to do so, we should document why we chose (not) to in the commit log message.

Show 7 quoted lines

> Validating and warning about bad entries is *not* the main purpose
> of the "credential-store" program, so I fully agree with the design
> of the "get" part.  I am not so sure about the other two operations
> (i.e. "store" and "erase") that do scan all the entries and has
> chance to warn about bad ones, though (note: I am not saying that we
> should parse verbosely---it is just that I do not know why you chose
> not to and I am not convinced that it is a good idea not to warn).

I re-read the incremental log for v8 and found your explanation.

Thanks.