From: Jonathan Tan <jonathantanmy@google.com>
Date: Mon, 01 May 2017 22:36:05 GMT
Subject: Re: [PATCH v2 11/53] fast-import: convert to struct object_id
Message-ID: <cc8ea2df-0679-cd52-a1be-cc49de4362bd@google.com>
In-Reply-To: <20170501222758.fzhrwg4w3p3ayxhg@sigill.intra.peff.net>

On 05/01/2017 03:27 PM, Jeff King wrote:
> On Mon, May 01, 2017 at 03:07:22PM -0700, Jonathan Tan wrote:
>
>>> @@ -2298,8 +2296,12 @@ static uintmax_t do_change_note_fanout(
>>>  static uintmax_t change_note_fanout(struct tree_entry *root,
>>>  		unsigned char fanout)
>>>  {
>>> -	char hex_sha1[40], path[60];
>>> -	return do_change_note_fanout(root, root, hex_sha1, 0, path, 0, fanout);
>>> +	/*
>>> +	 * The size of path is due to one slash between every two hex digits,
>>> +	 * plus the terminating NUL.
>>> +	 */
>>> +	char hex_oid[GIT_MAX_HEXSZ], path[GIT_MAX_HEXSZ * 3 / 2];
>>
>> If your comment is correct, shouldn't the size of path be 61 (that is, add
>> "+ 1")? I took a look at do_change_note_fanout() and your comment seems
>> correct.
>
> If you have 40 hex digits, then you have 20 hex pairs. But delimiting
> them all takes only 19 slashes, since they only go in between pairs[1].
>
> So the fully expanded formula is:
>
>   GIT_MAX_HEXSZ +               (1) actual hex bytes
>   (GIT_MAX_HEXSZ / 2) - 1 +     (2) internal slashes between pairs
>   1                             (3) NUL terminator

Ah...yes, you're right. (And I checked that do_change_note_fanout() does 
place the slashes only between characters, not before or after them.)

>
> which simplifies to 3/2 GIT_MAX_HEXSZ. It may be better to write it out
> (the compiler can simplify) or explain that in the comment, though. It
> took me a minute to figure out that it was correct, too.
>
> -Peff
>
> [1] This is sort of a reverse-fencepost error:
>     https://en.wikipedia.org/wiki/Off-by-one_error#Fencepost_error
>