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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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.
