Re: [PATCH 06/14] shortlog: replace hand-parsing of author with pretty-printer

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Dec 29, 2015 at 2:29 AM, Jeff King <peff@peff.net> wrote:
> When gathering the author and oneline subject for each
> commit, we hand-parse the commit headers to find the
> "author" line, and then continue past to the blank line at
> the end of the header.
>
> We can replace this tricky hand-parsing by simply asking the
> pretty-printer for the relevant items. This also decouples
> the author and oneline parsing, opening up some new
> optimizations in further commits.
>
> One reason to avoid the pretty-printer is that it might be
> less efficient than hand-parsing. However, I measured no
> slowdown at all running "git shortlog -ns HEAD" on
> linux.git.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> @@ -112,45 +112,32 @@ static void read_from_stdin(struct shortlog *log)
>  void shortlog_add_commit(struct shortlog *log, struct commit *commit)
>  {
> -       const char *author = NULL, *buffer;
> -       struct strbuf buf = STRBUF_INIT;
> -       struct strbuf ufbuf = STRBUF_INIT;
> -
> -       pp_commit_easy(CMIT_FMT_RAW, commit, &buf);
> -       buffer = buf.buf;
> -       while (*buffer && *buffer != '\n') {
> -               const char *eol = strchr(buffer, '\n');
> -
> -               if (eol == NULL)
> -                       eol = buffer + strlen(buffer);
> -               else
> -                       eol++;
> -
> -               if (starts_with(buffer, "author "))
> -                       author = buffer + 7;
> -               buffer = eol;
> -       }
> -       if (!author) {
> +       struct strbuf author = STRBUF_INIT;
> +       struct strbuf oneline = STRBUF_INIT;
> +       struct pretty_print_context ctx = {0};
> +
> +       ctx.fmt = CMIT_FMT_USERFORMAT;
> +       ctx.abbrev = log->abbrev;
> +       ctx.subject = "";
> +       ctx.after_subject = "";
> +       ctx.date_mode.type = DATE_NORMAL;
> +       ctx.output_encoding = get_log_output_encoding();
> +
> +       format_commit_message(commit, "%an <%ae>", &author, &ctx);
> +       if (author.len <= 3) {

I suppose magic number 3 is the space, '<', and '>'...

>                 warning(_("Missing author: %s"),
>                     oid_to_hex(&commit->object.oid));
>                 return;

Is this leaking strbuf 'author'?

>         }
> -       if (log->user_format) {
> -               struct pretty_print_context ctx = {0};
> -               ctx.fmt = CMIT_FMT_USERFORMAT;
> -               ctx.abbrev = log->abbrev;
> -               ctx.subject = "";
> -               ctx.after_subject = "";
> -               ctx.date_mode.type = DATE_NORMAL;
> -               ctx.output_encoding = get_log_output_encoding();
> -               pretty_print_commit(&ctx, commit, &ufbuf);
> -               buffer = ufbuf.buf;
> -       } else if (*buffer) {
> -               buffer++;
> -       }
> -       insert_one_record(log, author, !*buffer ? "<none>" : buffer);
> -       strbuf_release(&ufbuf);
> -       strbuf_release(&buf);
> +
> +       if (log->user_format)
> +               pretty_print_commit(&ctx, commit, &oneline);
> +       else
> +               format_commit_message(commit, "%s", &oneline, &ctx);
> +
> +       insert_one_record(log, author.buf, oneline.len ? oneline.buf : "<none>");
> +       strbuf_release(&author);
> +       strbuf_release(&oneline);
>  }