Subject: Re: [PATCH v6 04/19] fsck: Offer a function to demote fsck errors to warnings

From: Junio C Hamano <[email-masked]>
Date: Sun, 21 Jun 2015 10:36:05 -0700

Johannes Schindelin <[email-masked]> writes:

> Hmm. I really do not like that kind of thinking, i.e. having to
> duplicate, then modify data to be able to call the API, only to have
> to modify the data back afterwards, and eventually having to
> unallocate the data in all code paths. That feels just very inelegant
> to me.

You can see in our codebase that I have avoided touching end-user
strings by using a substring in place by introducing a new API
function or using an existing API function that takes <ptr, len> for
exactly that reason.  There also are cases where we are better off
if we make a copy upfront at a very high level in the callchain if
that makes the processing of that string deeper in the callchain
much simpler without customized helpers that take counted strings.

And 03/19 and this one taken together, I think it is an example of
the latter [*1*].

You not only need to invent counted string comparison in 04/19 but
also need upcasing byte-by-byte comparison in a loop in 03/19; both
of which can be made much simpler if you massaged the end-user input
"foo=error,bar=ignore" into "FOO=error,BAR=ignore" and allowed the
code to loop over it to turn ',' into NUL while parsing each
individual piece (i.e. "FOO=error").

So contrary to what you said in response to my review on 03/19, I
view this as not "adding complexity" but its total opposite.  It is
to make the code and logic much less complex by paying the price for
one copied (and massaged) string.

Having said all that, as long as the result functions correctly, I
suspect that it may not matter much either way.  As I already said,
"we can demote this error to a warning (or ignored)" is much less
useful in practice than the "we know this and that object in this
project does not pass fsck, so please do not bother checking" in my
mind, and that makes me think that this part of the series is much
less important than the skip-list thing.  Unnecessary complexity in
the code and otherwise useless helper functions may be something
that can be simplified following my advice, but that can be done as
a code clean-up, simplification and optimization by somebody else
later if they really cared.

Thanks.

[Footnote]

*1* Everything I say in my messages is what "I think", so saying "I
think" before saying what I think is redundant, but this one needs
that, because I think ;-) it is in the "taste" territory to view
each individual case and decide which one of these two opposite
approaches is more appropriate.