On Mon, May 06, 2019 at 03:28:44PM -0700, Jonathan Tan wrote:

quoted

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

Ah, thanks.

quoted

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

"Mimicking" sounds good to me.

quoted

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.

OK, that's fine.

quoted

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.

Those are good points.

quoted

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

Sounds good.

quoted

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.

I agree - the main thing is to remember that this is meant for the
newcomer who wants to start with a small-scoped project instead of the
experienced contributor who wants to know all there is about a topic.
(Which I think you've done very well, and should not be a problem for
the rest of the project to keep in mind too with the "My First
Contribution" name.)

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?

I agree that it's fine the way it is, and that the informal tone does
make this document (and by extension, the project) more accessible to
newcomers, which is a good thing.

I think that Emily is planning to send out a v5 with the Makefile
alphabetization, section header link targets, and some other textual
changes, and once she sends that out, I think that this is ready to be
merged in. So here's my reviewed-by:

Reviewed-by: Jonathan Tan <redacted>