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

From: René Scharfe <l.s.r@web.de>
Date: Jun 13, 2017, 18:09 UTC
Message-ID: <d229403a-d078-87b4-f3e8-89058fa4b548@web.de>
In-Reply-To: <xmqqd1a8n7o8.fsf@gitster.mtv.corp.google.com>

Am 13.06.2017 um 00:49 schrieb Junio C Hamano:

Show 29 quoted lines

> Michael Giuffrida <michaelpg@chromium.org> writes:
> 
>> For the abbreviated commit hash placeholder ('h'), pretty.c uses
>> add_again() to cache the result of the expansion, and then uses that
>> result in future expansions. This causes problems when the expansion
>> includes whitespace:
>>
>>      $ git log -n 1 --pretty='format:newline:%+h%nno_newline:%h'
>>      newline:
>>      a0b1c2d
>>      no_newline:
>>      a0b1c2
>>
>> The second expansion of the hash added an unwanted newline and removed
>> the final character. It seemingly used the cached expansion *starting
>> from the inserted newline*.
>>
>> The expected output is:
>>
>>      $ git log -n 1 --pretty='format:newline:%+h%nno_newline:%h'
>>      newline:
>>      a0b1c2d
>>      no_newline:a0b1c2d
> 
> Nicely explained.  The add_again() mechanism caches an earlier
> result by remembering the offset and the length in the strbuf the
> formatted string is being accumulated to, but because %+x (and
> probably %-x) magic placeholders futz with the result of
> format_commit_one() in place, the cache goes out of sync, of course.

Indeed, a very nice bug report!

Show 8 quoted lines

> 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. Does the caching feature justify the added complexity? Alternatives:

- Don't cache anymore, now that we have placeholders that change output
   on top of the original append-only ones.  Yields a net code reduction.
   Duplicated %h, %t and %p will have to be resolved at each occurrence.

- Provide some space for the cache instead of reusing the output, like
   a strbuf for each placeholder.  Adds a memory allocation to each
   first occurrence of %h, %t and %p.  Such a cache could be reused for
   multiple commits by truncating it instead of releasing; not sure how
   to pass it in nicely for it to survive a sequence of calls, though.

René