Re: [PATCH v4] documentation: add tutorial for first contribution

From: Emily Shaffer <emilyshaffer@google.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org, gitster@pobox.com, sunshine@sunshineco.com
Subject: Re: [PATCH v4] documentation: add tutorial for first contribution
Date: Tue, 7 May 2019 12:59:38 -0700	[thread overview]
Message-ID: <20190507195938.GD220818@google.com> (raw)
In-Reply-To: <20190506222844.261788-1-jonathantanmy@google.com>

On Mon, May 06, 2019 at 03:28:44PM -0700, Jonathan Tan wrote:
> Sorry for not looking at this sooner. 
> 
> Firstly, I'm not sure if this file should be named without the ".txt",
> like SubmittingPatches.

I should mention that during this change's life as a GitGitGadget PR,
dscho performed a review in GitHub, which included a recommendation to
name this SubmittingPatches.txt. This obviates quite a bit of
boilerplate within the Makefile as there are rules for handling *.txt
documentation generation already. You can check out Johannes's review:
https://github.com/gitgitgadget/git/pull/177

I keep forgetting to add a Reviewed-by line to my commit. I'll do that
before pushing based on your comments, as well as Josh and Phil's.

> 
> As for my other comments below, the Makefile comment below is the only
> one I feel strongly about; feel free to disagree with the rest (which I
> think are subjective).
> 
> > diff --git a/Documentation/Makefile b/Documentation/Makefile
> > index 26a2342bea..fddc3c3c95 100644
> > --- a/Documentation/Makefile
> > +++ b/Documentation/Makefile
> > @@ -74,6 +74,7 @@ API_DOCS = $(patsubst %.txt,%,$(filter-out technical/api-index-skel.txt technica
> >  SP_ARTICLES += $(API_DOCS)
> >  
> >  TECH_DOCS += SubmittingPatches
> > +TECH_DOCS += MyFirstContribution
> 
> Any reason not to keep this alphabetized?

No reason, done.

> > +=== Pull the Git codebase
> > +
> > +Git is mirrored in a number of locations. https://git-scm.com/downloads
> > +suggests one of the best places to clone from is GitHub.
> > +
> > +----
> > +$ git clone https://github.com/git/git git
> > +----
> 
> I would rename the header to "Clone the Git repository" instead, since
> "pull" has a specific meaning. Also, I think that "one of the best
> places" is unnecessary (I would just say "Clone the Git repository from
> one of its many mirrors, e.g.:"), but perhaps you want to leave it in
> there to maintain the informal tone.

I've merged the language from both and added that the GH mirror at
git/git is official.

"Git is mirrored in a number of locations. Clone the repository from one
of them; https://git-scm.com/downloads suggests one of the best places
to clone from is the official mirror on GitHub."

> > +We'll also need to add the extern declaration of psuh; open up `builtin.h`,
> > +find the declaration for `cmd_push`, and add a new line for `psuh` immediately
> > +before it, in order to keep the declarations sorted:
> > +
> > +----
> > +extern int cmd_psuh(int argc, const char **argv, const char *prefix);
> > +----
> 
> I was going to say to not include the "extern", but I see that builtin.h
> has them already, so it's probably better to leave it there for
> consistency.
> 

This was brought up in an earlier review and there's also a review
pending to remove them, which seems to have turned into a discussion
about how to maintain a script to remove them :) I'm going to avoid
politics and also remove the extern here, because it looks like that's
the direction the codebase is heading anyway.

> > +The list of commands lives in `git.c`. We can register a new command by adding
> > +a `cmd_struct` to the `commands[]` array. `struct cmd_struct` takes a string
> > +with the command name, a function pointer to the command implementation, and a
> > +setup option flag. For now, let's keep cheating off of `push`. Find the line
> > +where `cmd_push` is registered, copy it, and modify it for `cmd_psuh`, placing
> > +the new line in alphabetical order.
> 
> For an international audience, it might be better to replace "cheating
> off" with its literal meaning. It took me a while to understand that
> "cheating off" was meant to evoke a so-called cheat sheet.

You're right; I leaned too far towards casual voice here and included a
colloquialism. I've modified this to "let's keep mimicking `push`" as I
feel it means the same thing, without the slang but with a similar tone.

I also considered "copying from `push`" but didn't want to indicate we
would be copy-pasting lines of code. If anybody's got a better
suggestion for a verb here I'm happy to hear it; "cheating from X" is a
phrase I'm trying to stop using anyways :)

> > +Go ahead and inspect your new commit with `git show`. "psuh:" indicates you
> > +have modified mainly the `psuh` command. The subject line gives readers an idea
> > +of what you've changed. The sign-off line (`-s`) indicates that you agree to
> > +the Developer's Certificate of Origin 1.1 (see the
> > +`Documentation/SubmittingPatches` +++[[dco]]+++ header). If you wish to add some
> > +context to your change, go ahead with `git commit --amend`.
> 
> I think the last sentence is confusing - didn't we already add the
> context? (And if it's meant more along the lines of "if you want to
> change your commit message for whatever reason, use --amend", I don't
> think that's necessary here, since we are assuming that the user knows
> how to use Git.)

I think you're right. Removed. This seems like a holdover from the first
iteration, where I provided oneline commit messages for each commit,
instead of good practice examples.

> > +=== Implementation
> > +
> > +It's probably useful to do at least something besides printing out a string.
> > +Let's start by having a look at everything we get.
> > +
> > +Modify your `cmd_psuh` implementation to dump the args you're passed:
> > +
> > +----
> > +	int i;
> > +
> > +	...
> > +
> > +	printf(Q_("Your args (there is %d):\n",
> > +		  "Your args (there are %d):\n",
> > +		  argc),
> > +	       argc);
> > +	for (i = 0; i < argc; i++) {
> > +		printf("%d: %s\n", i, argv[i]);
> > +	}
> > +	printf(_("Your current working directory:\n<top-level>%s%s\n"),
> > +	       prefix ? "/" : "", prefix ? prefix : "");
> 
> Follow the Git style by not using braces around the single-line `for`
> block.

Done.

> > +Still, it'd be nice to know what the user's working context is like. Let's see
> > +if we can print the name of the user's current branch. We can cheat off of the
> > +`git status` implementation; the printer is located in `wt-status.c` and we can
> > +see that the branch is held in a `struct wt_status`.
> 
> Same comment about "cheat off" as previously.

Done. Again used s/cheat off of/mimic.

> > +----
> > +$ git send-email --to=target@example.com
> > +----
> 
> Hmm...don't you need to specify a directory?

Fixed...

> > +You will also need to go and find the Message-Id of your previous cover letter.
> > +You can either note it when you send the first series, from the output of `git
> > +send-email`, or you can look it up on the
> > +https://public-inbox.org/git[mailing list]. Find your cover letter in the
> > +archives, click on it, then click "permalink" or "raw" to reveal the Message-Id
> > +header. It should match:
> > +
> > +----
> > +Message-Id: <foo.12345.author@example.com>
> > +----
> > +
> > +Your Message-Id is `<foo.12345.author@example.com>`. This example will be used
> > +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.
> 
> I think it's better to describe the message ID as without the angle
> brackets. Reading the RFC (https://tools.ietf.org/html/rfc2392), the
> message-id doesn't have them.
> 
> [snip]

Junio argued the opposite here:
https://public-inbox.org/git/xmqqr29vbpge.fsf@gitster-ct.c.googlers.com/
and it looks like the RFC (possibly poorly-worded) also indicates the
angle brackets are part of the Message-ID spec (the ID without the
brackets is a '"mid" URL'):

   A "cid" URL is converted to the corresponding Content-ID message
   header [MIME] by removing the "cid:" prefix, converting the % encoded
   character to their equivalent US-ASCII characters, and enclosing the
   remaining parts with an angle bracket pair, "<" and ">".

   ...

   A "mid" URL is converted to a Message-ID or Message-ID/Content-ID
   pair in a similar fashion.

So I'll leave this the way it is.

> 
> > +----
> > +$ git send-email --to=target@example.com
> > +		 --in-reply-to=<foo.12345.author@example.com>
> > +----
> 
> The angle brackets can be omitted. Also, directory (or glob expression
> in this case)?

See above re: angle brackets. I've added the argument "psuh/v2*" to
include the v2 patches.

> 
> > +=== Bonus Chapter: One-Patch Changes
> 
> This is not truly a bonus - the mailing list prefers this if the patch
> set contains only one patch.

In the context specifically of this tutorial, I sort of think it is -
since the tutorial doesn't send out a one-patch changeset, this seems
like an aside to me. That is, I feel like the flow of the tutorial says,
"First you do A, then B, then C (and by the way, if you're doing C', you
would do it like this)."

I also liked the phrasing as a bonus because it covers something that
GitGitGadget does not support, so it's "extra content" compared to the
analogous chapter on using GGG.

If you feel very strongly, I could change it, but for now I disagree.

> > +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
> > +meaningful and explain the at a high level the purpose (what is happening and
> > +why) of your patch, but if you need to supply even more context, you can do so
> > +below the `---` in your patch. Take the example below, generated with
> > +`git format-patch` on a single commit:
> 
> It's not clear to me how `git format-patch` can generate the extra
> paragraph below. The user would either have to include "---" in the
> commit message (in which case there would be an extra "---" below the
> extra paragraph, which is perfectly safe) or edit the email *after*
> `git-format-patch` has generated the email.

I will clarify the wording to indicate that I mean the user should edit
the patch after generating. Brevity got in the way of completeness here.
Thanks.

I've modified the sentence to include that there was a second step here:
"generated with `git format-patch` on a single commit, and then edited
to add the content between the `---` and the diffstat."

I've also added a sentence to the note in the commit at the end, "This
section was added after `git format-patch` was run, by editing the patch
file in a text editor."

> > +----
> > +From 1345bbb3f7ac74abde040c12e737204689a72723 Mon Sep 17 00:00:00 2001
> > +From: A U Thor <author@example.com>
> > +Date: Thu, 18 Apr 2019 15:11:02 -0700
> > +Subject: [PATCH] README: change the grammar
> > +
> > +I think it looks better this way. This part of the commit message will
> > +end up in the commit-log.
> > +
> > +Signed-off-by: A U Thor <author@example.com>
> > +---
> > +Let's have a wild discussion about grammar on the mailing list. This
> > +part of my email will never end up in the commit log. Here is where I
> > +can add additional context to the mailing list about my intent, outside
> > +of the context of the commit log.
> > +
> > + README.md | 2 +-
> > + 1 file changed, 1 insertion(+), 1 deletion(-)
> > +
> > +diff --git a/README.md b/README.md
> > +index 88f126184c..38da593a60 100644
> 
> [snip]
> 
> There's also the issue of titles having Capital Initials raised in
> another review [1]. I think it's better to use sentence case, like in
> SubmittingPatches.

Phil Hord pointed this out too. I've combed through the titles, and
caught one which missed a monospace formatting.

> 
> [1] https://public-inbox.org/git/CABURp0rE23SCxB4VD0-kVWp6OfS7-4O6biyD7zMqSUQvR_RZxg@mail.gmail.com/
> 
> Overall, thanks for writing this. I think it's a good overview of what a
> contributor should do when they write a set of patches for inclusion in
> Git.
> 
> I had a meta-concern about the length of this document, but I think most
> (if not all) of the information contained herein is useful, so I think
> that the length is fine.

Thanks. I wondered about that too, but mostly regarding review velocity.
I'm not sure that it makes sense to split this into parts, but I wonder
if it's worth it to add anchors to each chapter so that it'd be easier
to send a specific section to someone who had a question. For example,
in the standup last week, dscho suggested to vishal that they have a
look at this patch, and named a specific section. It'd be easier if
dscho could link something like
git-scm.org/docs/MyFirstContribution.html#adding-tests.

I've convinced myself that this is a good idea, so I'll add them before
I push v5.

> The other meta-concern is maintaining the informal tone when we update
> this document (for example, when we add features like range-diff which
> can be used when sending v2 - well, somebody can add information about
> that to this document once it has been merged); but I don't think that
> is a concern in practice (either we keep the tone or there is a slight
> tone mismatch, and I don't think that either is a big deal).

I see your concern. I'm not sure whether it would really be a big deal
as long as folks who are editing the document remember that this is a
tutorial, not a reference document. That is, with your range-diff
example, an editor should mention something like "An alternative is to
use `range-diff`; you can first run `foo` against your new commit and
old diff, and then you can run `bar` to send it." rather than "Or, use
`range-diff`. Usage: `git range-diff [foo] [bar] <baz>`." And hopefully
that kind of tone difference should be pretty clear in the context of
the rest of the document.

The one concern I do have with the informal tone is that it may be
exclusionary to international or ESL contributors in ways that I can't
understand as a native US speaker. It looks like you caught one such
instance in your review this time. I'm not sure whether it makes sense
to reword the entire document, because I was hoping to keep it from
being intimidating by being overly formal/technical. It seems like so
far folks on the list have been comfortable reading it, so maybe it's
fine the way it is?

Thanks a bunch for your review, Jonathan.

 - Emily