> 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 Ah, thanks. > > > +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. > > 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@xxxxxxxxxxxxxxxxxxxxxxxxx/ > 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. > > > +=== 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. > > > +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. > > 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 <jonathantanmy@xxxxxxxxxx>
