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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]