Re: [BUG] add_again() off-by-one error in custom format
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 13, 2017, 18:29 UTC
- Message-ID
- <xmqqtw3j68rc.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <d229403a-d078-87b4-f3e8-89058fa4b548@web.de>
René Scharfe <l.s.r@web.de> writes:
Show 12 quoted lines
> 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.
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.
Show 12 quoted lines
> > - 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é