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

From: Christian Couder <christian.couder@gmail.com>
Date: May 26, 2019, 07:48 UTC
Message-ID: <CAP8UFD2YH50Br4BNmTqEVeUknxi1X39JCRB4XMwK8rx3DWx=KA@mail.gmail.com>
In-Reply-To: <20190517190701.49722-2-emilyshaffer@google.com>

On Fri, May 17, 2019 at 11:48 PM Emily Shaffer <emilyshaffer@google.com> wrote:

Show 6 quoted lines

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

Show 6 quoted lines

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

Show 6 quoted lines

> +=== 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.

Show 6 quoted lines

> +----
> +       const char *cfg_name;
> +
> +...
> +
> +       git_config(git_default_config, NULL)

Nit: a ";" is missing at the end of the above line.

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

Show 27 quoted lines

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

Thanks, Christian.