Re: [PATCH] shortlog: skip format/parse roundtrip for internal traversal
- Date: Sun, 10 Sep 2017 10:44:46 +0200
- From: René Scharfe <l.s.r@xxxxxx>
- Subject: Re: [PATCH] shortlog: skip format/parse roundtrip for internal traversal
Am 08.09.2017 um 11:21 schrieb Jeff King:
> Note that the non-stdin path no longer looks at the "mailmap" entry of
> "struct shortlog" (instead we use the one cached inside pretty.c). But
> we still waste time loading it. I'm not sure if it's worth addressing
> that. It's only once per program invocation, and it's a little tricky to
> fix (we do shortlog_init() before we know whether or not we're using
> stdin). We could just load it lazily, though, which would cover the
> stdin case.
The difference in performance and memory usage will only be measurable
with really big mailmap files. However, it may be an opportunity for
simplifying the mailmap API in general. Conceptually the map data
should fit into struct repository instead of being read and stored by
each user, right?
> -- >8 --
> Subject: shortlog: skip format/parse roundtrip for internal traversal
> The original git-shortlog command parsed the output of
> git-log, and the logic went something like this:
> 1. Read stdin looking for "author" lines.
> 2. Parse the identity into its name/email bits.
> 3. Apply mailmap to the name/email.
> 4. Reformat the identity into a single buffer that is our
> "key" for grouping entries (either a name by default,
> or "name <email>" if --email was given).
> The first part happens in read_from_stdin(), and the other
> three steps are part of insert_one_record().
> When we do an internal traversal, we just swap out the stdin
> read in step 1 for reading the commit objects ourselves.
> Prior to 2db6b83d18 (shortlog: replace hand-parsing of
> author with pretty-printer, 2016-01-18), that made sense; we
> still had to parse the ident in the commit message.
> But after that commit, we use pretty.c's "%an <%ae>" to get
> the author ident (for simplicity). Which means that the
> pretty printer is doing a parse/format under the hood, and
> then we parse the result, apply the mailmap, and format the
> result again.
> Instead, we can just ask pretty.c to do all of those steps
> for us (including the mailmap via "%aN <%aE>", and not
> formatting the address when --email is missing).
> And then we can push steps 2-4 into read_from_stdin(). This
> speeds up "git shortlog -ns" on linux.git by about 3%, and
> eliminates a leak in insert_one_record() of the namemailbuf
Great! Thanks for stepping back, looking at the bigger
picture and making it prettier.
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> builtin/shortlog.c | 56 ++++++++++++++++++++++++++++++++++--------------------
> 1 file changed, 35 insertions(+), 21 deletions(-)
While longer, the resulting code is split up into more