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

[Emily Shaffer <emilyshaffer@google.com>]

On Wed, Jun 19, 2019 at 04:13:35AM -0400, Eric Sunshine wrote:
> 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.

Hmm. I can say that I personally would find that much more difficult to
follow interactively, and I'd be tempted to copy-and-paste and skim
through the wall of text if I was presented with such a snippet.
However, I could also imagine the reverse - someone becoming tired of
having their hand held through a fairly straightforward implementation,
when they're perfectly capable of reading a long description and would
just like to get on with it.

I'm really curious about what others think in this scenario, since I
imagine it boils down to individual learning styles.

(Maybe we can split the difference and present a complete patch or new
function, followed by a breakdown? That would end up even more verbose
than the current approach, though.)

... Now that I'm thinking more about this, and reading some of your
later comments on this mail, I think it might be valuable to lean on the
sample patchset for complete code samples, especially if we figure a
good way to distribute the patchset near the tutorial (as Junio and I
are discussing in another branch of this thread). Then we can keep the
tutorial concise, but have the complete code available for those who
prefer to look there.

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

Yeah, I like this a lot. Thanks! I took it word for word; will be adding
you to the Helped-by line of the commit.

> 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'm of two minds about this. On the one hand, I'm somewhat in favor of
leaving contextual, informational comments in the sample code, so the
sample code can teach on its own without the tutorial (specifically, I
mean the patchset that was sent alongside this one as RFC). On the other
hand, you're right that adding these informational comments doesn't
model best practices for real commits.

I don't have a strong opposition to removing those comments from the
in-place samples in the tutorial itself. But I do think it's useful to
include them in the sample patchset, which is intended as an additional
learning tool, rather than as a pristine code example - especially if we
make it clear in the commit messages there.

> 
> > > > +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?

Yeah, that's a good point. I'll try to mention it in a more
universally-applicable way, like you suggested.

> 
> > > > +       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).

Oh, that's a very good point. I'll frame it that way - that's a handy
place to slip in some bonus context about Git. Thanks.

  NOTE: We aren't localizing the printf here because we have purposefully
  formatted it in a machine-parseable way. Commands in Git are divided into
  "plumbing" and "porcelain"; the "plumbing" commands are machine-parseable and
  intended for use in scripts, while the "porcelain" commands are intended for
  human interaction. Output intended for script usage doesn't need to be
  localized; output intended for humans does.


Thanks again for the review effort.

 - Emily