Re: [RFC PATCH v2 05/13] walken: configure rev_info and prepare for walk

[Emily Shaffer <emilyshaffer@google.com>]

On Thu, Jun 27, 2019 at 01:06:32AM -0400, Eric Sunshine wrote:
> On Wed, Jun 26, 2019 at 7:51 PM Emily Shaffer <emilyshaffer@google.com> wrote:
> > `struct rev_info` is what's used by the struct itself.
> 
> What "struct itself"? Do you mean 'struct rev_info' is used by the
> _walk_ itself? Or something?

Yep, that's the one. Thanks for the fresh eyes.

> 
> > `repo_init_revisions()` initializes the struct; then we need to set it
> > up for the walk we want to perform, which is done in
> > `final_rev_info_setup()`.
> > [...]
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
> > diff --git a/builtin/walken.c b/builtin/walken.c
> > @@ -30,6 +31,40 @@ static void init_walken_defaults(void)
> > +/*
> > + * cmd_log calls a second set of init after the repo_init_revisions call. We'll
> > + * mirror those settings in post_repo_init_init.
> > + */
> 
> What is 'post_repo_init_init'?
> 
> I found the reference to cmd_log() confusing because I was looking for
> it in this patch (as if it was being introduced here). Newcomers might
> be even more confused. Perhaps if you state explicitly that you're
> referring to existing code in an existing file, it might be clearer.
> Maybe:
> 
>     builtin/log.c:cmd_log() calls a second ...
> 
> Overall, I find this entire function comment mystifying.

Yeah, this is very stale and never got updated when I realized cmd_log()
was calling two init functions for apparently legacy reasons, and I
didn't need to mirror it here. Again a case where fresh eyes caught
something that became invisible after I stared at it for weeks. I really
appreciate you doing the deep review, Eric.

I've replaced it:

 /*
  * Perform configuration for commit walk here. Within this function we set a
  * starting point, and can customize our walk in various ways.
  */

> 
> > +static void final_rev_info_setup(int argc, const char **argv, const char *prefix,
> > +               struct rev_info *rev)
> > +{
> > +       /*
> > +        * Optional:
> > +        * setup_revision_opt is used to pass options to the setup_revisions()
> > +        * call. It's got some special items for submodules and other types of
> > +        * optimizations, but for now, we'll just point it to HEAD and call it
> > +        * good. First we should make sure to reset it. This is useful for more
> > +        * complicated stuff but a decent shortcut for the first pass is
> > +        * add_head_to_pending().
> > +        */
> 
> I had to pause over "call it good" for several seconds (since I
> couldn't understand why someone would want to write "bad" code) until
> I figured out you meant "do nothing else". It would be clearer simply
> to drop that, ending the sentence at "HEAD":
> 
>     ..., but for now, we'll just point it at HEAD.

Definitely. Done.

> 
> > +       /*
> > +        * struct setup_revision_opt opt;
> > +
> > +        * memset(&opt, 0, sizeof(opt));
> > +        * opt.def = "HEAD";
> > +        * opt.revarg_opt = REVARG_COMMITTISH;
> > +        * setup_revisions(argc, argv, rev, &opt);
> > +        */
> > +
> > +       /* Let's force oneline format. */
> > +       get_commit_format("oneline", rev);
> > +       rev->verbose_header = 1;
> > +
> > +       /* add the HEAD to pending so we can start */
> > +       add_head_to_pending(rev);
> > +}
> 
> It would be easier for the reader to associate the
> add_head_to_pending() invocation with the commented-out setting of
> "HEAD" via 'setup_revision_opt' if the two bits abutted one another
> without being separated by the "oneline" gunk.

Good point, done.