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?
> `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.
> +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.
> + /*
> + * 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.
