git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Tan <jonathantanmy@google.com>
To: emilyshaffer@google.com
Cc: git@vger.kernel.org, gitster@pobox.com, sunshine@sunshineco.com,
	Jonathan Tan <jonathantanmy@google.com>
Subject: Re: [PATCH v4] documentation: add tutorial for first contribution
Date: Mon,  6 May 2019 15:28:44 -0700	[thread overview]
Message-ID: <20190506222844.261788-1-jonathantanmy@google.com> (raw)
In-Reply-To: <20190423193410.101803-1-emilyshaffer@google.com>

Sorry for not looking at this sooner. 

Firstly, I'm not sure if this file should be named without the ".txt",
like SubmittingPatches.

As for my other comments below, the Makefile comment below is the only
one I feel strongly about; feel free to disagree with the rest (which I
think are subjective).

> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index 26a2342bea..fddc3c3c95 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -74,6 +74,7 @@ API_DOCS = $(patsubst %.txt,%,$(filter-out technical/api-index-skel.txt technica
>  SP_ARTICLES += $(API_DOCS)
>  
>  TECH_DOCS += SubmittingPatches
> +TECH_DOCS += MyFirstContribution

Any reason not to keep this alphabetized?

> +=== Pull the Git codebase
> +
> +Git is mirrored in a number of locations. https://git-scm.com/downloads
> +suggests one of the best places to clone from is GitHub.
> +
> +----
> +$ git clone https://github.com/git/git git
> +----

I would rename the header to "Clone the Git repository" instead, since
"pull" has a specific meaning. Also, I think that "one of the best
places" is unnecessary (I would just say "Clone the Git repository from
one of its many mirrors, e.g.:"), but perhaps you want to leave it in
there to maintain the informal tone.

> +We'll also need to add the extern declaration of psuh; open up `builtin.h`,
> +find the declaration for `cmd_push`, and add a new line for `psuh` immediately
> +before it, in order to keep the declarations sorted:
> +
> +----
> +extern int cmd_psuh(int argc, const char **argv, const char *prefix);
> +----

I was going to say to not include the "extern", but I see that builtin.h
has them already, so it's probably better to leave it there for
consistency.

> +The list of commands lives in `git.c`. We can register a new command by adding
> +a `cmd_struct` to the `commands[]` array. `struct cmd_struct` takes a string
> +with the command name, a function pointer to the command implementation, and a
> +setup option flag. For now, let's keep cheating off of `push`. Find the line
> +where `cmd_push` is registered, copy it, and modify it for `cmd_psuh`, placing
> +the new line in alphabetical order.

For an international audience, it might be better to replace "cheating
off" with its literal meaning. It took me a while to understand that
"cheating off" was meant to evoke a so-called cheat sheet.

> +Go ahead and inspect your new commit with `git show`. "psuh:" indicates you
> +have modified mainly the `psuh` command. The subject line gives readers an idea
> +of what you've changed. The sign-off line (`-s`) indicates that you agree to
> +the Developer's Certificate of Origin 1.1 (see the
> +`Documentation/SubmittingPatches` +++[[dco]]+++ header). If you wish to add some
> +context to your change, go ahead with `git commit --amend`.

I think the last sentence is confusing - didn't we already add the
context? (And if it's meant more along the lines of "if you want to
change your commit message for whatever reason, use --amend", I don't
think that's necessary here, since we are assuming that the user knows
how to use Git.)

> +=== 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:
> +
> +----
> +	int i;
> +
> +	...
> +
> +	printf(Q_("Your args (there is %d):\n",
> +		  "Your args (there are %d):\n",
> +		  argc),
> +	       argc);
> +	for (i = 0; i < argc; i++) {
> +		printf("%d: %s\n", i, argv[i]);
> +	}
> +	printf(_("Your current working directory:\n<top-level>%s%s\n"),
> +	       prefix ? "/" : "", prefix ? prefix : "");

Follow the Git style by not using braces around the single-line `for`
block.

> +Still, it'd be nice to know what the user's working context is like. Let's see
> +if we can print the name of the user's current branch. We can cheat off of the
> +`git status` implementation; the printer is located in `wt-status.c` and we can
> +see that the branch is held in a `struct wt_status`.

Same comment about "cheat off" as previously.

> +----
> +$ git send-email --to=target@example.com
> +----

Hmm...don't you need to specify a directory?

> +You will also need to go and find the Message-Id of your previous cover letter.
> +You can either note it when you send the first series, from the output of `git
> +send-email`, or you can look it up on the
> +https://public-inbox.org/git[mailing list]. Find your cover letter in the
> +archives, click on it, then click "permalink" or "raw" to reveal the Message-Id
> +header. It should match:
> +
> +----
> +Message-Id: <foo.12345.author@example.com>
> +----
> +
> +Your Message-Id is `<foo.12345.author@example.com>`. This example will be used
> +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.

I think it's better to describe the message ID as without the angle
brackets. Reading the RFC (https://tools.ietf.org/html/rfc2392), the
message-id doesn't have them.

[snip]

> +----
> +$ git send-email --to=target@example.com
> +		 --in-reply-to=<foo.12345.author@example.com>
> +----

The angle brackets can be omitted. Also, directory (or glob expression
in this case)?

> +=== Bonus Chapter: One-Patch Changes

This is not truly a bonus - the mailing list prefers this if the patch
set contains only one patch.

> +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
> +meaningful and explain the at a high level the purpose (what is happening and
> +why) of your patch, but if you need to supply even more context, you can do so
> +below the `---` in your patch. Take the example below, generated with
> +`git format-patch` on a single commit:

It's not clear to me how `git format-patch` can generate the extra
paragraph below. The user would either have to include "---" in the
commit message (in which case there would be an extra "---" below the
extra paragraph, which is perfectly safe) or edit the email *after*
`git-format-patch` has generated the email.

> +----
> +From 1345bbb3f7ac74abde040c12e737204689a72723 Mon Sep 17 00:00:00 2001
> +From: A U Thor <author@example.com>
> +Date: Thu, 18 Apr 2019 15:11:02 -0700
> +Subject: [PATCH] README: change the grammar
> +
> +I think it looks better this way. This part of the commit message will
> +end up in the commit-log.
> +
> +Signed-off-by: A U Thor <author@example.com>
> +---
> +Let's have a wild discussion about grammar on the mailing list. This
> +part of my email will never end up in the commit log. Here is where I
> +can add additional context to the mailing list about my intent, outside
> +of the context of the commit log.
> +
> + README.md | 2 +-
> + 1 file changed, 1 insertion(+), 1 deletion(-)
> +
> +diff --git a/README.md b/README.md
> +index 88f126184c..38da593a60 100644

[snip]

There's also the issue of titles having Capital Initials raised in
another review [1]. I think it's better to use sentence case, like in
SubmittingPatches.

[1] https://public-inbox.org/git/CABURp0rE23SCxB4VD0-kVWp6OfS7-4O6biyD7zMqSUQvR_RZxg@mail.gmail.com/

Overall, thanks for writing this. I think it's a good overview of what a
contributor should do when they write a set of patches for inclusion in
Git.

I had a meta-concern about the length of this document, but I think most
(if not all) of the information contained herein is useful, so I think
that the length is fine.

The other meta-concern is maintaining the informal tone when we update
this document (for example, when we add features like range-diff which
can be used when sending v2 - well, somebody can add information about
that to this document once it has been merged); but I don't think that
is a concern in practice (either we keep the tone or there is a slight
tone mismatch, and I don't think that either is a big deal).

  parent reply	other threads:[~2019-05-06 22:28 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
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 [this message]
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=20190506222844.261788-1-jonathantanmy@google.com \
    --to=jonathantanmy@google.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --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).