Re: [PATCH v2 4/9] cache-tree: avoid strtol() on non-string buffer

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> Looking over what you wrote below, it seems pretty reasonable to me.
> What do you consider unfinished in it?

Two things I am unhappy about are that (1) parsing the digit
sequence that represents abs(x) into unsigned int while catching
wraparound and (2) checking if 'val' that has abs(x) would fit in a
signed int when 'negate' is applied.  For both of them, there ought
to be a better way to write, and perhaps there may be a clean way to
do both at the same time that is easier reason about.

> Another option is to scrap this approach entirely, and copy up until the
> trailing newline into a separate buffer, NUL-terminate it, and parse
> from that buffer. That feels a little dirty to me, but I suspect it is
> pretty performant in practice, and it pushes all of the complexity back
> onto strtol().
>
> Another variant of that is: parse up to the trailing newline, making
> sure it's there, and then leave the rest of the code as-is. We know that
> strtol() will do the right thing in that case, but it does mean we
> cannot use ASan's strict_string_checks (it would still yield a false
> positive, because it does not know we've checked for the newline).

Or perhaps introduce cache-tree-version-2 index extension.  If there
are other things we may want to fix while we are at it, that would
be a better way to spend our engineering resource, but I offhand do
not know of anything gravely lacking there that we may want to fix
(there are little things like how the pathnames are sorted that I
regret the way it was implemented, but that does not motivate me
enough).