On Sun, Apr 21, 2019 at 3:52 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Emily Shaffer <emilyshaffer@xxxxxxxxxx> 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@xxxxxxxxxx> > > --- > > 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@xxxxxxxxxxx/other/junk > > +---- > > + > > +Your Message-Id is `foo.12345.author@xxxxxxxxxxx`. This example will be used > > Technically, <foo.12345.author@xxxxxxxxxxx> 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@xxxxxxxxxxx > > + --in-reply-to=foo.12345.author@xxxxxxxxxxx > > +---- 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
