Re: [BUG] add_again() off-by-one error in custom format

From: René Scharfe <l.s.r@web.de>
Date: Jun 13, 2017, 20:29 UTC
Message-ID: <99d19e5a-9f79-9c1e-3a23-7b2437b04ce9@web.de>
In-Reply-To: <xmqqtw3j68rc.fsf@gitster.mtv.corp.google.com>

Am 13.06.2017 um 20:29 schrieb Junio C Hamano:

Show 17 quoted lines

> René Scharfe <l.s.r@web.de> writes:
> 
>> Indeed, a very nice bug report!
>>
>>> I think the call to format_commit_one() needs to be taught to pass
>>> 'magic' through, and then add_again() mechanism needs to be told not
>>> to cache when magic is in effect, or something like that.
>>>
>>> Perhaps something along this line...?
>>>
>>>    pretty.c | 64 ++++++++++++++++++++++++++++++++++++++--------------------------
>>>    1 file changed, 38 insertions(+), 26 deletions(-)
>>
>> That looks quite big.  You even invent a way to classify magic.
> 
> Hmph, by "a way to classify", do you mean the enum?  That thing was
> there from before, just moved.

Oh, yes, sorry. I didn't even get that far into the patch. (I'll better go to bed after hitting send..)

Show 16 quoted lines

> Also I think we do not have to change add_again() at all, because
> chunk->off tolerates being given a garbage value as long as
> chunk->len stays 0, and add_again() does not change chunk->len at
> all.
> 
> Which cuts the diffstat down to
> 
>   pretty.c | 40 +++++++++++++++++++++++++---------------
>   1 file changed, 25 insertions(+), 15 deletions(-)
> 
>> Does the caching feature justify the added complexity?
> 
> That's a good question.  I thought about your second alternative
> while adding the "don't cache" thing, but if we can measure and find
> out that we are not gaining much from caching, certainly that sounds
> much simpler.

The difference is about the same as the one between:

	$ time git log --format="" >/dev/null

	real    0m0.463s
	user    0m0.448s
	sys     0m0.012s

and:

	$ time git log --format="%h" >/dev/null

	real    0m1.062s
	user    0m0.636s
	sys     0m0.416s

With caching duplicates are basically free and without it short hashes have to be looked up again. Other placeholders may reduce the relative slowdown, depending on how expensive they are.

Forgot a third option, probably because it's not a particularly good idea: Replacing the caching in pretty.c with a short static cache in find_unique_abbrev_r().

René