On Wed, Jun 26, 2019 at 7:51 PM Emily Shaffer <[email protected]> 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 <[email protected]>
> ---
> 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/&()/
> + 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.
> + 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);
> +}
