Am 13.06.2017 um 00:49 schrieb Junio C Hamano:
Michael Giuffrida <michae...@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:a0b1c2dNicely 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é
