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

> On Mon, Jun 17, 2019 at 7:20 PM Emily Shaffer <emilyshaffer@google.com> wrote:
> > On Fri, Jun 07, 2019 at 02:21:07AM -0400, Eric Sunshine wrote:
> > > On Thu, Jun 6, 2019 at 9:08 PM Emily Shaffer <emilyshaffer@google.com> wrote:
> > > > +int cmd_walken(int argc, const char **argv, const char *prefix)
> > > > +{
> > > > +       struct option options[] = {
> > > > +               OPT_END()
> > > > +       };
> > > > +
> > > > +       argc = parse_options(argc, argv, prefix, options, walken_usage, 0);
> > > > +
> > > > +       ...
> > >
> > > Perhaps comment out the "..." or remove it altogether to avoid having
> > > the compiler barf when the below instructions tell the reader to build
> > > the command.
> >
> > Hmm. That part I'm not so sure about. I like to use the "..." to
> > indicate where the code in the snippet should be added around the other
> > code already in the file - which I suppose it does just as clearly if
> > it's commented - but I also hope folks are not simply copy-pasting
> > blindly from the tutorial.
> >
> > It seems like including uncommented "..." in code tutorials is pretty
> > common.
> 
> 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.

> 
> > I don't think I have a good reason to push back on this except that I
> > think "/* ... */" is ugly :)
> >
> > I'll go through and replace "..." with some actual hints about what's
> > supposed to go there; for example, here I'll replace with "/* print and
> > return */".
> 
> Seeing as my initial review comment was in error, I'm not sure that
> you ought to replace "..." with anything else.
> 
> > > "invoke anything" is pretty nebulous, as is the earlier "components
> > > you may invoke". A newcomer is unlikely to know what this means, so
> > > perhaps it needs an example (even if just a short parenthetical
> > > comment).
> >
> > I have tried to reword this; I hope this is a little clearer.
> >
> >   Before you begin to examine user configuration for your revision walk, it's
> >   common practice for you to initialize to default any switches that your command
> >   may have, as well as ask any other components you may invoke to initialize as
> >   well (for example, how `git log` also uses the `grep` and `diff` components).
> >   `git log` does this in `init_log_defaults()`; in that case, one global
> >   `decoration_style` is initialized, as well as the grep and diff-UI components.
> 
> 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").

> 
> > > > +This display is an indicator for the latency between publishing a commit for
> > > > +review the first time, and getting it actually merged into master.
> > >
> > > Perhaps: s/master/`&`/
> > >
> > > Even as a long-time contributor to the project, I had to pause over
> > > this statement for several seconds before figuring out what it was
> > > talking about. Without a long-winded explanation of how topics
> > > progress from submission through 'pu' through 'next' through 'master'
> > > and finally into a release, the above statement is likely to be
> > > mystifying to a newcomer. Perhaps it should be dropped.
> >
> > Such an explanation exists in MyFirstContribution.txt. I will include a
> > shameless plug to that document here. :)
> 
> 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?

> 
> > > > +       printf(_("Object walk completed. Found %d commits, %d blobs, %d tags, "
> > > > +                "and %d trees.\n"), commit_count, blob_count, tag_count,
> > > > +              tree_count);
> > >
> > > Or make the output more useful by having it be machine-parseable (and
> > > not localized):
> > >
> > >     printf("commits %d\nblobs %d\ntags %d\ntrees %d\n",
> > >         commit_count, blob_count, tag_cont, tree_count);
> >
> > I'm not sure whether I agree, since it's a useless toy command only for human
> > parsing.
> 
> 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).