From: René Scharfe Date: Tue, 13 Jun 2017 18:09:17 GMT Subject: Re: [BUG] add_again() off-by-one error in custom format Message-ID: In-Reply-To: Am 13.06.2017 um 00:49 schrieb Junio C Hamano: > Michael Giuffrida 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! > 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é