Re: [PATCH] documentation: add tutorial for revision walking

On Mon, Jun 17, 2019 at 7:20 PM Emily Shaffer <emilyshaffer@google.com> wrote:

You're right, and that's not what I was "complaining" about. Looking back at your original email, I see that I somehow got confused and didn't realize or (quickly) forgot that you had already presented a _complete_ cmd_walken() snippet just above that spot, and that the cmd_walken() snippet upon which I was commenting was _incomplete_, thus the "..." was perfectly justified. Not realizing that the incomplete cmd_walken() example was just that (incomplete), I "complained" that the following "compile the project" instructions would barf on "...".

Maybe I got confused because the tiny cmd_walken() snippets followed one another so closely (or because I got interrupted several times during the review), but one way to avoid that would be to present a single _complete_ snippet from the start, followed by a bit of explanation. That is, something like this:

    Open up a new file `builtin/walken.c` and set up the command handler:

    ----
    /* "git walken" -- Part of the "My First Revision Walk" tutorial. */
    #include "builtin.h"

    int cmd_walken(int argc, const char **argv, const char *prefix)
    {
        const char * const usage[] = {
            N_("git walken"),
            NULL,
        }
        struct option options[] = {
            OPT_END()
        };

        argc = parse_options(argc, argv, prefix, options, usage, 0);

        printf(_("cmd_walken incoming...\n"));
        return 0;
    }
    ----

    `usage` is the usage message presented by `git -h walken`, and
    `options` will eventually specify command-line options.

Seeing as my initial review comment was in error, I'm not sure that you ought to replace "..." with anything else.

By trying to express too many things at once, it's still difficult to follow. Perhaps use shorter, more easily digestible sentences, like this:

    Before examining configuration files which may modify command
    behavior, set up default state for switches or options your
    command may have. If your command utilizes other Git components,
    ask them to set up their default states, as well. For instance,
    `git log` takes advantage of `grep` and `diff` functionality; its
    init_log_defaults() sets its own state (`decoration_style`) and
    asks `grep` and `diff` to initialize themselves by calling their
    initialization functions.

As this is just a toy example, I don't care too strongly about the unnecessary second sentence. On the other hand, the tutorial is trying to teach people how to contribute to this project, and on this project, that sort of pointless comment is likely to be called out in review. In fact, given that view, the entire comment block is unnecessary (it doesn't add any value for anyone reviewing or reading the code), so it might make more sense to drop the comment from the code entirely, and just do a better job explaining in prose above the snippet why you are calling that function. For instance:

    ... Let's start the helper with the call to `prepare_revision_walk()`,
    which does the final setup of the `rev_info` structure before it can
    be used.

The above observation may be more widely applicable than to just this one instance. Don't use in-code comments for what should be explained in prose if the in-code comment adds no value to the code itself (to wit, if a reviewer would say "don't repeat in a comment what the code already says clearly" or "don't use a comment to state the obvious").

I found that this sort of tangential reference disturbed the flow of the tutorial, leading the mind astray from the otherwise natural progression of the presentation. So, I'm not convinced that talking about the migration of a topic in the Git project itself adds value to this tutorial. The same effect could be seen when commits have been re-ordered via git-rebase, too, right? Perhaps mention that instead?

True, it's not a big deal, and I don't insist upon it. But, if you mention in prose that this output is easily machine-parseable, then perhaps that nudges the reader a bit in the direction of thinking about porcelain vs. plumbing, which is something a contributor to this project eventually has to be concerned with (the sooner, the better).