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

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

Phillip Wood <phillip.wood123@gmail.com> writes:

> All we need to do to accept a single minus sign is s/while/if/
> ...
> If we limit ourselves to accepting a single minus sign then this can become
> 	if (s == *ptr + (sign == -1))
>
> so we need very little in the way of extra code.
> ...
> A generic helper to replace strtol() that takes a length rather than 
> assuming the input is NUL terminated could be useful elsewhere but I'm 
> not sure we need something that complicated here. I do like the fact 
> that overflow does not cause undefined behavior though. Changing ret for 
> "int" to "unsigned" in peff's patch should fix that.
> 
> Thanks

Perhaps.

By the way, an interesting tangent is this.

The only reason why these fields under discussion are stored in
textual decimal is pretty much the same as the reason why the object
header expresses the byte-length of the payload in textual decimal,
i.e., to be independent from the platform natural implementation of
"int" type (e.g., endiannness and width), but unlike object files,
the index is a local matter (we are prepared for the same directory
accessed over NFS from two platforms with different endianness, but
we do not recommend network access to a repository in the first
place).  And a lot more importantly, the total number of the index
entries contained within an index file is capped to 2^32-1 (the
header has 32-bit count in the network byte order).  The total
number of subdirectories within a directory or the total number of
entries for a level of directory hierarchy that would form a tree
object from a slice of the index cannot exceed that number anyway.

And thanks to the design that made cache-tree an optional index
extension, we can make cache-tree version 2 where the in-core
representation is exactly the same as the current one, but only uses
different serialization when writing to and reading from the index
file.  The new serialization can use 32-bit network byte order
integers, or use our own varint.{c,h,rs}, to record these numbers.

A version of Git that knows about that extension could be taught to
read from the current cache-tree and convert to a new version, but
better yet, it can simply ignore the current cache-tree data in the
file, and write the new version when we do need to write the index
out with a cache-tree.  When such a transparent auto conversion
happens, one single invocation of write_index_as_tree() would become
more expensive than usual (because the last invocation of the
current Git left cache-tree data in the index and usually the next
invocation of Git would take advantage of it when it writes a tree,
but a new version of Git that uses the v2 format would behave as if
there is no cache-tree data in the index and build the tree from
scratch.  After that happens, the cache-tree data in the new format
will be reused and things will continue to work.  You could use an
older version of Git on such an index file and the same transparent
auto conversion will take care of the transition.