[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH v9] credential-store: warn instead of fatal for bogus lines from store
From:       Junio C Hamano <gitster () pobox ! com>
Date:       2020-04-30 21:14:15
Message-ID: xmqq7dxwr80o.fsf () gitster ! c ! googlers ! com
[Download RAW message or body]

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.

> 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.
[prev in list] [next in list] [prev in thread] [next in thread]