Re: [PATCH v6 1/2] documentation: add tutorial for first contribution

[Emily Shaffer <emilyshaffer@google.com>]

On Sun, May 26, 2019 at 09:48:26AM +0200, Christian Couder wrote:
> On Fri, May 17, 2019 at 11:48 PM Emily Shaffer <emilyshaffer@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