git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: emilyshaffer@google.com, git@vger.kernel.org, sunshine@sunshineco.com
Subject: Re: [PATCH v4] documentation: add tutorial for first contribution
Date: Wed, 08 May 2019 11:45:30 +0900	[thread overview]
Message-ID: <xmqq5zqlk6k5.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <20190506222844.261788-1-jonathantanmy@google.com> (Jonathan Tan's message of "Mon, 6 May 2019 15:28:44 -0700")

Jonathan Tan <jonathantanmy@google.com> writes:

> Sorry for not looking at this sooner. 
>
> Firstly, I'm not sure if this file should be named without the ".txt",
> like SubmittingPatches.

SubmittingPatches has historical baggage but this does not, so its
source can be left as .txt (alternatively we could have added .txt
to SubmittingPatches and left a symlink to keep the historical name,
without introducing "copy X to produce X.txt" rule).

cf. http://public-inbox.org/git/xmqqbm15kxi0.fsf@gitster-ct.c.googlers.com/

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

I do not think the order matters to $(MAKE), and I do not know if
the order matters to humans---sane ordering is done to futureproof
when we know we will have many more, but I do not know if we will.

So I find it a borderline Meh, but let's not waste your finding, as
sorting them in alpha order would be the sensible default.

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

I am guilty of the verbosity there---just did not want to give an
impression that that one is the single authoritative copy (the k.org
one is probably the one, if only that it is the one pushed to first
when a new development happens).  I personally feel that your
rephrasing is fine, too, and do not have strong preference between
the two.

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

Yup, this was discussed before.  If we can have the "NEEDSWORK" in
the asciidoc source that is not rendered in the end result, it may
be worth leaving a note to say when we remove them from builtin.h we
need to update this example, or something like that.

>> +----
>> +$ git send-email --to=target@example.com
>> +----
>
> Hmm...don't you need to specify a directory?

Even better would be the directory/*.patch glob pattern, as we'll
show how to emit format-patch output for v2 into the same directory
in a later step.  Just giving the directory and letting readdir()
collect would work for v1 but not for later iterations.

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

See earlier review(s).
Your reading is probably wrong, as that directly contradicts my
earlier suggestion based on the same RFC ;-)

>> +----
>> +$ 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)?

Yeah, a glob would be appropriate.

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

Yes, I think the editions after v2 of this series has consistently
encouraged users to edit format-patch output in a separate editor
session (be it 0/n cover letter, or 1/1 single patch) before sending
it out, discouraging use of --compose in send-email or driving
format-patch from within a send-email session.

And the following example just shows how the finished result of such
an editing session would look like.

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

I am OK as long as those who care about maintaining coherent tone
pay attention to changes proposed to this document in the future ;-)

Thanks for comments.  Let's finish this topic and merge it down
soonish.

  parent reply	other threads:[~2019-05-08  2:45 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
2019-05-07 19:59         ` Emily Shaffer
2019-05-07 20:32           ` Jonathan Tan
2019-05-08  2:45         ` Junio C Hamano [this message]
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=xmqq5zqlk6k5.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.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).