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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 24, 2025, 23:09 UTC
Message-ID: <xmqqms4buix0.fsf@gitster.g>
In-Reply-To: <20251124223023.GA2051672@coredump.intra.peff.net>

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.

Show 11 quoted lines

> 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).