Re: [PATCH 3/3] Rework pretty_print_commit to use strbufs instead of custom buffers.

On Sat, Sep 08, 2007 at 11:59:31AM +0000, David Kastrup wrote:

Pierre Habouzit [off-list ref] writes:

quoted

  Also remove the "len" parameter, as:
  (1) it was used as a max boundary, and every caller used ~0u
  (2) we check for final NUL no matter what, so it doesn't help for speed.

That sounds like a change that makes improvement of callers impossible
when it is found out that it leads to a performance issue.  Is it only
the pretty-print that is affected?

  I removed the "len" argument of pretty_print_commit and all the sub
pp_* functions it uses. This argument was supposed to tell which size
the commit message you want to read to format the pretty printing.

  It leads to a lot of code that works like this:

  if (position < len || *msg) /* test if len is overflowed or if we are
                                 at the end of the string */
    break;

  This impose us to maintain the len of the message while we make the
"msg" pointer progress, and so on. And it's a limit of what to read in the
commit message.

Seeing where pretty_print_commit is used:

builtin-branch.c:			pretty_print_commit(CMIT_FMT_ONELINE, commit,
builtin-log.c:			pretty_print_commit(CMIT_FMT_ONELINE, commit,
builtin-rev-list.c:		pretty_print_commit(revs.commit_format, commit,
builtin-show-branch.c:		pretty_print_commit(CMIT_FMT_ONELINE, commit,
commit.c:void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
log-tree.c:	pretty_print_commit(opt->commit_format, commit, &msgbuf,


  I assume that the user would not be very pleased if we decide to crop
his commits logs when he uses git-log. And given that git log in the
linux repository on my laptop takes:

  2,13s user 0,06s system 99% cpu 2,213 total

when the repo is hot ... I hardly think it can ever be a performance
issue :)

  But if people disagree I can refactor a patch with the "len" argument
kept.
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org