Re: [PATCH v3] documentation: add lab for first contribution

[Emily Shaffer <emilyshaffer@google.com>]

On Sun, Apr 21, 2019 at 3:52 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Emily Shaffer <emilyshaffer@google.com> writes:
>
> > This tutorial covers how to add a new command to Git and, in the
> > process, everything from cloning git/git to getting reviewed on the
> > mailing list. It's meant for new contributors to go through
> > interactively, learning the techniques generally used by the git/git
> > development community.
> >
> > Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> > ---
>
> I think a stray "lab" remains in the title of the patch.  They seem
> to have disappeared from all the other places, and "tutorial" is
> consistently used, which is good.

Thanks, finished.

>
> My eyes have lost freshness on this topic, so my review tonight is
> bound to be (much) less thorough than the previous round.

I appreciate the thorough reviews you gave til now, thanks very much
for all your time.

>
> > +- `Documentation/SubmittingPatches`
> > +- `Documentation/howto/new-command.txt`
>
> Good (relative to the earlier one, these are set in monospace).
>
> > +
> > +== Getting Started
> > +
> > +=== Pull the Git codebase
> > +
> > +Git is mirrored in a number of locations. https://git-scm.com/downloads
>
> Perhaps URLs should also be set in monospace?

This breaks the hyperlink. So I'd rather not?

>
>
> > +NOTE: When you are developing the Git project, it's preferred that you use the
> > +`DEVELOPER flag`; if there's some reason it doesn't work for you, you can turn
>
> I do not think you want to set 'flag' in monospace, too.
> i.e. s| flag`|` flag|;

Thanks, good catch.

> > +     git_config(git_default_config, NULL)
> > +     if (git_config_get_string_const("user.name", &cfg_name) > 0)
> > +     {
> > +             printf(_("No name is found in config\n"));
> > +     }
> > +     else
> > +     {
> > +             printf(_("Your name: %s\n"), cfg_name);
> > +     }
>
> Style.  Opening braces "{" for control structures are never be on
> its own line, and else comes on the same line as closing "}" of if,
> i.e.
>
>         if (...) {
>                 print ...
>         } else {
>                 print ...
>         }

Took this suggestion.

>
> Or just get rid of braces if you are not going to extend one (or
> both) of if/else blocks into multi-statement blocks.
>
> > +----
> > +
> > +`git_config()` will grab the configuration from config files known to Git and
> > +apply standard precedence rules. `git_config_get_string_const()` will look up
> > +a specific key ("user.name") and give you the value. There are a number of
> > +single-key lookup functions like this one; you can see them all (and more info
> > +about how to use `git_config()`) in `Documentation/technical/api-config.txt`.
> > +
> > +You should see that the name printed matches the one you see when you run:
> > +
> > +----
> > +$ git config --get user.name
> > +----
> > +
> > +Great! Now we know how to check for values in the Git config. Let's commit this
> > +too, so we don't lose our progress.
> > +
> > +----
> > +$ git add builtin/psuh.c
> > +$ git commit -sm "psuh: show parameters & config opts"
> > +----
> > +
> > +NOTE: Again, the above is for sake of brevity in this tutorial. In a real change
> > +you should not use `-m` but instead use the editor to write a verbose message.
>
> We never encourge people to write irrelevant things or obvious
> things that do not have to be said.  But a single-liner message
> rarely is sufficient to convey "what motivated the change, and why
> the change is done in the way seen in the patch" in a meaningful
> way.
>
> i.e. s|verbose|meaningful|;

Thanks, done. I'll leave out repeating the lecture here as it was
given in the full sample commit.

>
> > +Create your test script and mark it executable:
> > +
> > +----
> > +$ touch t/t9999-psuh-tutorial.sh
> > +$ chmod +x t/t9999-psuh-tutorial.sh
> > +----
>
> I never "create an empty file" before editing in real life.  Is this
> a common workflow in some circles?
>
> I'd be tempted to suggest s/touch/edit/ here, but I dunno.

Looking back, I'm wondering why I wrote it this way - I think I wanted
to avoid prescribing an editor and also mention the permissions.

I'll try to reword this to mention the executable bit after the
content of the test script.

>
> > +https://public-inbox.org/git/foo.12345.author@example.com/other/junk
> > +----
> > +
> > +Your Message-Id is `foo.12345.author@example.com`. This example will be used
>
> Technically, <foo.12345.author@example.com> with angle bracket is
> the message Id, but the tool is lenient to allow this common mistake
> ;-) so this one, and the "send-email --in-reply-to=" example below
> can stay as-is.

I've since reworded this section to mention reading the Message-Id
from the permalinked email in public-inbox. I think it might be easier
than spying on the URL as the URL is different in some views and
doesn't reflect the Message-Id.

>
> > +below as well; make sure to replace it with the correct Message-Id for your
> > +**previous cover letter** - that is, if you're sending v2, use the Message-Id
> > +from v1; if you're sending v3, use the Message-Id from v2.
> > +
> > +Now send the emails again, paying close attention to which messages you pass in
> > +to the command:
> > +
> > +----
> > +$ git send-email --to=target@example.com
> > +              --in-reply-to=foo.12345.author@example.com
> > +----

Since I made a meaningful change in this section anyways, I've fixed
this to include the angle brackets.

> > +
> > +=== Bonus Chapter: One-Patch Changes
> > +
> > +In some cases, your very small change may consist of only one patch. When that
> > +happens, you only need to send one email. Your commit message should already be
> > +verbose, but if you need to supply even more context, you can do so below the
>
> s|be verbose|explain what and why of the change well| or something
> like that?
>
> > +`---` in your patch. Take the example below, generated with `git format-patch`
> > +on a single commit:

Thanks again. I'll send v4 later today or tomorrow to give more time
for comments if anybody else is planning to look.


-- 
Emily Shaffer