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

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

> 
> > +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.

> 
> > +       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/&()/

> 
> > +       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.

> 
> > +               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);