On Thu, Jun 27, 2019 at 01:16:58AM -0400, Eric Sunshine wrote:
> On Wed, Jun 26, 2019 at 7:51 PM Emily Shaffer <emilyshaffer@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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.
Done.
>
> > +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.
Done.
>
> > + 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/&()/
Done.
>
> > + rev->diffopt.close_file = 0;
>
> Why this? And, why isn't it set up where other 'rev' options are initialized?
Removed. Artifact of closely mirroring log.
>
> > + 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.
Yep, removed.
>
> > + 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.
Yeah, you're right. I'll change to "so we send it to stdout", which is
less obvious from puts().
>
> > + }
>
> Leaking 'prettybuf'. Add here:
>
> strbuf_release(&prettybuf);
Thanks, done.
