On Sun, May 26, 2019 at 09:48:26AM +0200, Christian Couder wrote: > On Fri, May 17, 2019 at 11:48 PM Emily Shaffer <emilyshaf...@google.com> > wrote: > > > > 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. > > Very nice, thanks! I tried it and I liked it very much. > > I noted a few nits that might help improve it a bit. > > > +---- > > +$ git clone https://github.com/git/git git > > Nit: maybe add "$ cd git" after that.
Sure, done. > > > +Check it out! You've got a command! Nice work! Let's commit this. > > + > > +---- > > +$ git add Makefile builtin.h builtin/psuh.c git.c > > +$ git commit -s > > +---- > > Nit: when building a "git-psuh" binary is created at the root of the > repo which will pollute the `git status` output. The usual way we deal > with that is by adding "/git-psuh" to the ".gitignore" at the root of > the repo. Right you are - good catch. I'll add a paragraph about adding to the gitignore. > > > +=== 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: > > Nit: maybe it could be a bit more clear that the previous printf() > call should be kept as is, otherwise the test added later will fail. Done. > > > +---- > > + const char *cfg_name; > > + > > +... > > + > > + git_config(git_default_config, NULL) > > Nit: a ";" is missing at the end of the above line. Yikes, done. > > > +Let's commit this as well. > > + > > +---- > > +$ git commit -sm "psuh: print the current branch" > > Nit: maybe add "builtin/psuh.c" at the end of the above line, so that > a `git add builtin/psuh.c` is not needed. This is purely personal preference, but I prefer manually adding files first. I didn't add any indication about staging the changes to psuh.c though, so I'm adding a line to `git add builtin/psuh.c`. I found one other place where the commit line was shown without the add line, so I included the add there too. > > > +.... > > +git-psuh(1) > > +=========== > > + > > +NAME > > +---- > > +git-psuh - Delight users' typo with a shy horse > > + > > + > > +SYNOPSIS > > +-------- > > +[verse] > > +'git-psuh' > > + > > +DESCRIPTION > > +----------- > > +... > > + > > +OPTIONS[[OPTIONS]] > > +------------------ > > +... > > + > > +OUTPUT > > +------ > > +... > > + > > + > > Nit: it seems that the above newline could be removed. Sure, why not. > > Thanks, > Christian. Thanks for trying it out and for your thorough review, Christian. I appreciate it! Since this is checked into next already, I'll be sending a follow-on patch in reply to my last version in this thread which is based on the tip of next. - Emily
