Re: [PATCH v3] documentation: add lab for first contribution

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 21, 2019, 10:52 UTC
Message-ID: <xmqqr29vbpge.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190419165709.123502-1-emilyshaffer@google.com>

Emily Shaffer <emilyshaffer@google.com> writes:

Show 8 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.
>
> Signed-off-by: Emily Shaffer <emilyshaffer@google.com>
> ---

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.

My eyes have lost freshness on this topic, so my review tonight is bound to be (much) less thorough than the previous round.

> +- `Documentation/SubmittingPatches`
> +- `Documentation/howto/new-command.txt`

Good (relative to the earlier one, these are set in monospace).

Show 6 quoted lines

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

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

Show 6 quoted lines

> +This commit message is intentionally formatted to 72 columns per line,
> +starts with a single line as "commit message subject" that is written as
> +if to command the codebase to do something (add this, teach a command
> +that). The body of the message is designed to add information about the
> +commit that is not readily deduced from reading the associated diff,
> +such as answering the question "why?".

Nicely written.

Show 9 quoted lines

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

Or just get rid of braces if you are not going to extend one (or both) of if/else blocks into multi-statement blocks.

Show 24 quoted lines

> +----
> +
> +`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|;

Show 6 quoted lines

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

> +https://public-inbox.org/git/foo.12345.author@example.com/other/junk
> +----
> +
> +Your Message-Id is `foo.12345.author@example.com`. This example will be used

Technically, <foo.12345.author@example.com> 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.

Show 17 quoted lines

> +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@example.com
> +		 --in-reply-to=foo.12345.author@example.com
> +----
> +
> +=== 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: