git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Victoria Dye <vdye@github.com>
To: Philippe Blain <levraiphilippeblain@gmail.com>,
	Philippe Blain via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: Emily Shaffer <emilyshaffer@google.com>,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH 2/3] MyFirstContribution: also explain cover letter in GitGitGadget section
Date: Fri, 29 Apr 2022 09:27:22 -0700	[thread overview]
Message-ID: <f7f64135-7d57-5926-501b-13e83411b4fd@github.com> (raw)
In-Reply-To: <c7030c98-bd65-7d0e-89ac-ae291e22b10d@gmail.com>

Philippe Blain wrote:
> Hi Victoria,
> 
> Le 2022-04-27 à 16:43, Victoria Dye a écrit :
>> Philippe Blain via GitGitGadget wrote:
> 
>>> +----
>>> +Adding the 'psuh' command
>>
>> Typically I see patch series titles follow the same "imperative mood" as
>> commit titles/messages (see 'Documentation/SubmittingPatches.txt'). I'm not
>> sure whether that's a rule written down somewhere or just convention, but
>> for the sake of consistency you might want to do something like:
>>
>> 	"Add the 'psuh' command"
>>
> 
> I fully agree. I just copied the existing patch series title from 
> the git-send-email section further down. I think it would make sense
> to change this to also using the imperative mood just like commit messages
> in a preparatory commit. I'll do that.
> 
>>> +----
>>> +
>>> +Your PR's description will used as the body of the cover letter.
>>
>> Including the line "Your PR's description..." is somewhat confusing to me as
>> a first-time reader, since I was interpreting this section to be the
>> *verbatim* text of the pull request title & description. If this *is* meant
>> to be that description, then the note about the PR description can be
>> removed. That point is also mentioned above, so it's probably not needed
>> here anyway.
> 
> I'm not exactly sure what you mean. I meant that the description of the PR
> will be used as the body of the cover letter...
> 

Sorry for being unclear. What I meant was that, the way this part of the
document reads (to me) right now, it looks like the recommendation is to
create a pull request with the title & description:

Title: 
	Adding the 'psuh' command
Description:
	Your PR's description will [be] used as the body of the cover
	letter. <insert content from MyFirstContribution-coverletter.txt>

That is, the line "Your PR description will..." is the first line of the
description (which I don't think was your intention).

That said, upon a more holistic re-read, I've found a few more things that
are confusing/oddly phrased. I'll try re-reviewing (part of) the patch
below, with (hopefully) more direct/concrete feedback this time:

> -Review the PR's title and description, as it's used by GitGitGadget as the cover
> -letter for your change. When you're happy, submit your pull request.
> +Review the PR's title and description, as they're used by GitGitGadget as the
> +cover letter for your change. The cover letter describes your proposed
> +contribution as a whole and is ideal to mention any context that might be

s/is ideal to/should(?)

> +useful for reviewers. The title of your PR should be something which
> +succinctly covers the purpose of your entire topic branch, for example:
> +
> +----
> +Adding the 'psuh' command
> +----

Other than the "Adding" -> "Add" change, this is good - the example title
will be formatted as a monospaced block, clearly separating it from the
advice/context preceding it.

> +
> +Your PR's description will used as the body of the cover letter.
> +include::MyFirstContribution-coverletter.txt[]
> +

Conversely, this section has no visible separation between the context
("Your PR's description...") and the example
("include::MyFirstContribution-coverletter.txt[]"), hence my confusion
earlier. 

To parallel the title example, maybe you could use similar monospace
formatting for the example? E.g.:

+
+Your PR's description will used as the body of the cover letter.
+ 
+----
+include::MyFirstContribution-coverletter.txt[]
+----
+

One thing I noticed on my re-read is that the context you provide for the
title ("The title of your PR should be something...") isn't quite paralleled
in the context for the description ("Your PR's description..."). The former
talks about *how* you should write your cover letter title, whereas the
latter simply states that the PR description will become the cover letter's
content. For added context, then, you might want to describe how the cover
letter content should be written, e.g.:

+
+Your PR's description will be used as the body of the cover letter and
+should explain the purpose of the series, for example:
+ 
+----
+include::MyFirstContribution-coverletter.txt[]
+----
+

(note the s/will used/will be used)

Apologies for all the picky grammar comments - this series includes a lot of
really helpful information for new contributors, and I really appreciate
your work here! Hopefully my feedback is a bit more clear this time (and is
relatively straightforward to implement).

Thanks!


> Thanks for having a look!
> 
> Philippe.


  reply	other threads:[~2022-04-29 16:28 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-27 19:04 [PATCH 0/3] Improve MyFirstContribution's " Philippe Blain via GitGitGadget
2022-04-27 19:04 ` [PATCH 1/3] MyFirstContribution: move cover letter description to a separate file Philippe Blain via GitGitGadget
2022-04-27 21:15   ` Eric Sunshine
2022-04-27 22:02     ` Philippe Blain
2022-04-27 21:48   ` Junio C Hamano
2022-04-28 18:16     ` Philippe Blain
2022-04-27 19:04 ` [PATCH 2/3] MyFirstContribution: also explain cover letter in GitGitGadget section Philippe Blain via GitGitGadget
2022-04-27 20:43   ` Victoria Dye
2022-04-28 18:21     ` Philippe Blain
2022-04-29 16:27       ` Victoria Dye [this message]
2022-05-10 23:45         ` Philippe Blain
2022-04-27 19:04 ` [PATCH 3/3] MyFirstContribution: drop PR description for GGG single-patch contributions Philippe Blain via GitGitGadget
2022-04-27 21:56   ` Junio C Hamano
2022-04-28 18:25     ` Philippe Blain
2022-04-28 19:43       ` Junio C Hamano
2022-05-11  1:47 ` [PATCH v2 0/5] Improve MyFirstContribution's GitGitGadget section Philippe Blain via GitGitGadget
2022-05-11  1:47   ` [PATCH v2 1/5] MyFirstContribution: add "Anatomy of a Patch Series" section Philippe Blain via GitGitGadget
2022-05-11  6:20     ` Bagas Sanjaya
2022-05-11 21:30       ` Junio C Hamano
2022-05-11 21:48         ` Philippe Blain
2022-05-11 22:09           ` Junio C Hamano
2022-05-12 23:00             ` Philippe Blain
2022-05-12 12:11     ` Philip Oakley
2022-05-12 22:53       ` Philippe Blain
2022-05-11  1:47   ` [PATCH v2 2/5] MyFirstContribution: add standalone section on cover letter Philippe Blain via GitGitGadget
2022-05-11 10:01     ` Junio C Hamano
2022-05-11 21:49       ` Philippe Blain
2022-05-11  1:47   ` [PATCH v2 3/5] MyFirstContribution: reference "The cover letter" in "Preparing Email" Philippe Blain via GitGitGadget
2022-05-11  1:47   ` [PATCH v2 4/5] MyFirstContribution: reference "The cover letter" in GitGitGadget section Philippe Blain via GitGitGadget
2022-05-11  1:47   ` [PATCH v2 5/5] MyFirstContribution: drop PR description for GGG single-patch contributions Philippe Blain via GitGitGadget
2022-05-11 10:11   ` [PATCH v2 0/5] Improve MyFirstContribution's GitGitGadget section Junio C Hamano
2022-05-11 17:45   ` Victoria Dye
2022-05-12 23:43   ` [PATCH v3 " Philippe Blain via GitGitGadget
2022-05-12 23:43     ` [PATCH v3 1/5] MyFirstContribution: add "Anatomy of a Patch Series" section Philippe Blain via GitGitGadget
2022-05-12 23:43     ` [PATCH v3 2/5] MyFirstContribution: add standalone section on cover letter Philippe Blain via GitGitGadget
2022-05-12 23:43     ` [PATCH v3 3/5] MyFirstContribution: reference "The cover letter" in "Preparing Email" Philippe Blain via GitGitGadget
2022-05-12 23:43     ` [PATCH v3 4/5] MyFirstContribution: reference "The cover letter" in GitGitGadget section Philippe Blain via GitGitGadget
2022-05-12 23:43     ` [PATCH v3 5/5] MyFirstContribution: drop PR description for GGG single-patch contributions Philippe Blain via GitGitGadget
2022-05-19 21:49     ` [PATCH v3 0/5] Improve MyFirstContribution's GitGitGadget section Junio C Hamano
2022-05-21 16:17       ` Philippe Blain
2022-05-22  1:35         ` Junio C Hamano
2022-05-24 12:29           ` Philippe Blain
2022-05-24 19:06             ` Junio C Hamano

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=f7f64135-7d57-5926-501b-13e83411b4fd@github.com \
    --to=vdye@github.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=levraiphilippeblain@gmail.com \
    --subject='Re: [PATCH 2/3] MyFirstContribution: also explain cover letter in GitGitGadget section' \
    /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

Code repositories for project(s) associated with this 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).