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

From: Pierre Habouzit <madcoder@debian.org>
Date: Sep 8, 2007, 12:17 UTC
Message-ID: <20070908121748.GA21864@artemis.corp>
In-Reply-To: <851wd9xt98.fsf@lola.goethe.zz>

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

Show 9 quoted lines

> Pierre Habouzit <madcoder@debian.org> writes:
> 
> >   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