git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Emily Shaffer <emilyshaffer@google.com>
Cc: git@vger.kernel.org, Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCH v3] documentation: add lab for first contribution
Date: Sun, 21 Apr 2019 19:52:17 +0900	[thread overview]
Message-ID: <xmqqr29vbpge.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20190419165709.123502-1-emilyshaffer@google.com> (Emily Shaffer's message of "Fri, 19 Apr 2019 09:57:09 -0700")

Emily Shaffer <emilyshaffer@google.com> 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@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).

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

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

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

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

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

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

  reply	other threads:[~2019-04-21 10:52 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-11 18:32 [PATCH 0/1] documentation: add lab for first contribution Emily Shaffer via GitGitGadget
2019-04-11 18:32 ` [PATCH 1/1] " Emily Shaffer via GitGitGadget
2019-04-12  3:20   ` Junio C Hamano
2019-04-12 22:03     ` Emily Shaffer
2019-04-13  5:39       ` Junio C Hamano
2019-04-15 17:26         ` Emily Shaffer
2019-04-11 21:03 ` [PATCH 0/1] " Josh Steadmon
2019-04-12  2:35 ` Junio C Hamano
2019-04-12 22:58   ` Emily Shaffer
2019-04-16 20:26 ` [PATCH v2 " Emily Shaffer via GitGitGadget
2019-04-16 20:26   ` [PATCH v2 1/1] " Emily Shaffer via GitGitGadget
2019-04-17  5:32     ` Junio C Hamano
2019-04-17  8:07       ` Eric Sunshine
2019-04-18  0:05         ` Junio C Hamano
2019-04-17 23:16       ` Emily Shaffer
2019-04-16 21:13   ` [PATCH v2 0/1] " Emily Shaffer
2019-04-19 16:57   ` [PATCH v3] " Emily Shaffer
2019-04-21 10:52     ` Junio C Hamano [this message]
2019-04-22 22:27       ` Emily Shaffer
2019-04-23 19:34     ` [PATCH v4] documentation: add tutorial " Emily Shaffer
2019-04-30 18:59       ` Josh Steadmon
2019-05-02  0:57         ` Emily Shaffer
2019-05-03  2:11       ` Phil Hord
2019-05-07 19:05         ` Emily Shaffer
2019-05-06 22:28       ` Jonathan Tan
2019-05-07 19:59         ` Emily Shaffer
2019-05-07 20:32           ` Jonathan Tan
2019-05-08  2:45         ` Junio C Hamano
2019-05-07 21:30       ` [PATCH v5 0/2] documentation: add lab " Emily Shaffer
2019-05-07 21:30         ` [PATCH v5 1/2] documentation: add tutorial " Emily Shaffer
2019-05-07 23:25           ` Emily Shaffer
2019-05-08  3:46           ` Junio C Hamano
2019-05-08 18:58             ` Emily Shaffer
2019-05-08 19:53               ` Jonathan Tan
2019-05-07 21:30         ` [PATCH v5 2/2] documentation: add anchors to MyFirstContribution Emily Shaffer
2019-05-08  3:30         ` [PATCH v5 0/2] documentation: add lab for first contribution Junio C Hamano
2019-05-17 19:03         ` [PATCH v6 0/2] documentation: add tutorial " Emily Shaffer
2019-05-17 19:07           ` [PATCH v6 1/2] " Emily Shaffer
2019-05-26  7:48             ` Christian Couder
2019-05-29 20:09               ` Emily Shaffer
2019-10-18 16:40             ` SZEDER Gábor
2019-10-18 22:54               ` Emily Shaffer
2019-05-17 19:07           ` [PATCH v6 2/2] documentation: add anchors to MyFirstContribution Emily Shaffer
2019-05-29 20:18           ` [PATCH] doc: add some nit fixes " Emily Shaffer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqr29vbpge.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).