Re: [RFC PATCH v2 06/13] walken: perform our basic revision walk

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jun 27, 2019, 05:16 UTC
Message-ID: <CAPig+cQAf6ds6Fobcf+mux-93D09WQJACUXSvwgdyoOznNjsUg@mail.gmail.com>
In-Reply-To: <20190626235032.177551-7-emilyshaffer@google.com>

On Wed, Jun 26, 2019 at 7:51 PM Emily Shaffer <emilyshaffer@google.com> wrote:

Show 11 quoted lines

> Add the final steps needed and implement the walk loop itself. We add a
> method walken_commit_walk() which performs the final setup to revision.c
> and then iterates over commits from get_revision().
> [...]
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---
> diff --git a/builtin/walken.c b/builtin/walken.c
> +/*
> + * walken_commit_walk() is invoked by cmd_walken() after initialization. It
> + * does the commit walk only.
> + */

"only" as opposed to what? Maybe just say:

    ... after initialization. It performs the actual commit walk.

Show 9 quoted lines

> +static void walken_commit_walk(struct rev_info *rev)
> +{
> +       struct commit *commit;
> +       struct strbuf prettybuf = STRBUF_INIT;
> +
> +       /*
> +         * prepare_revision_walk() gets the final steps ready for a revision
> +        * walk. We check the return value for errors.
> +         */

You have some funky mix of spaces and tabs indenting the comment lines. Same for the next comment block.

Show 8 quoted lines

> +       if (prepare_revision_walk(rev)) {
> +               die(_("revision walk setup failed"));
> +       }
> +
> +       /*
> +         * Now we can start the real commit walk. get_revision grabs the next
> +        * revision based on the contents of rev.
> +        */

s/get_revision/&()/

> +       rev->diffopt.close_file = 0;

Why this? And, why isn't it set up where other 'rev' options are initialized?

> +       while ((commit = get_revision(rev))) {
> +               if (!commit)
> +                       continue;

If get_revision() returns NULL, then the while-loop exits, which means that the "if (!commit)" condition will never be satisfied, thus is unnecessary code.

Show 7 quoted lines

> +               strbuf_reset(&prettybuf);
> +               pp_commit_easy(CMIT_FMT_ONELINE, commit, &prettybuf);
> +               /*
> +                * We expect this part of the output to be machine-parseable -
> +                * one commit message per line - so we must not localize it.
> +                */
> +               puts(prettybuf.buf);

Meh, but there isn't any literal text here to localize anyway, so the comment talking about not localizing it is just confusing.

> +       }

Leaking 'prettybuf'. Add here:

    strbuf_release(&prettybuf);

> +}