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

quoted

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.

quoted

+=== 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."

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+----
+$ git send-email --to=target@example.com
+----

Hmm...don't you need to specify a directory?

Fixed...

quoted

+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: [off-list ref]
+----
+
+Your Message-Id is `[off-list ref]`. 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.

quoted

+----
+$ git send-email --to=target@example.com
+		 --in-reply-to=[off-list ref]
+----

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.

quoted

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

quoted

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

quoted

+----
+From 1345bbb3f7ac74abde040c12e737204689a72723 Mon Sep 17 00:00:00 2001
+From: A U Thor [off-list ref]
+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 [off-list ref]
+---
+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