Junio C Hamano <gitster@xxxxxxxxx> 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.
