Date:   Wed, 15 Nov 2017 16:09:32 +0200
From:   Jani Nikula <jani.nikula@...ux.intel.com>
To:     "Tobin C. Harding" <me@...in.cc>, Jonathan Corbet <corbet@....net>
Cc:     linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Ulf Hansson <ulf.hansson@...aro.org>,
        Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH] docs: add submitting-pull-requests.rst

On Wed, 15 Nov 2017, "Tobin C. Harding" <me@...in.cc> wrote:
> On Tue, Nov 14, 2017 at 04:48:16PM -0700, Jonathan Corbet wrote:
>
> Awesome comments Jon, I knew there would be more to writing docs than
> first met the eye.
>
>> On Wed, 15 Nov 2017 09:54:21 +1100
>> "Tobin C. Harding" <me@...in.cc> wrote:
>> 
>> > There is currently no documentation on how to create a pull request for
>> > Linus.
>> > 
>> >     Anyway, this actually came up at the kernel summit / maintainer
>> >     meeting a few weeks ago, in that "how do I make a good pull request
>> >     to Linus" is something we need to document.
>> > 
>> >     Here's what I do, and it seems to work well, so maybe we should turn
>> >     it into the start of the documentation for how to do it.
>> > 
>> > Create document from email thread on LKML (referenced in document).
>> > 
>> > Signed-off-by: Tobin C. Harding <me@...in.cc>
>> > ---
>> > 
>> > Is it rude to send this during the merge window? Can resend after it closes if
>> > it makes life easier.
>> 
>> I can handle patches during the merge window.  That said, while I welcome
>> this effort and think it's a good start, there's a few things I'll
>> quibble about:
>> 
>>  - Much of this was actually written by Greg, I believe, and some by Linus.
>>    That deserves credit in the changelog, if nowhere else.
>
> Yeah, I struggled for ages with the tense, Greg's stuff is obviously
> written as him. But I didn't want to paraphrase and present it as if I'd
> written it. After your comments I'm still unsure of the _best_ way to
> present this material with a good flow but still giving credit where
> credit is due? I didn't seem right to add their names to the document
> (thereby presenting myself as them). I did not think of the changelog -
> I'll go that path for v2.
>
>>  - Putting it in Documentation/process as RST is good.  But it should be
>>    added to index.rst and made part of the docs build.  I suspect you
>>    haven't run it through sphinx at all yet, right?  Some things are
>>    unlikely to format the way you think they might.
>
> My bad, I knew I would botch some of the RST stuff, didn't think to run
> it through sphinx (I tend to view kernel docs as the raw files ;)
>
>> Finally, I see this as being the first installment in what, I hope, will
>> someday be a nice "how to be a kernel maintainer" manual.  I wouldn't
>> insist on it before taking a patch like this, but if you could see
>> through to organizing it as a chapter in a bigger sub-book, that would be
>> great.
>
> Happy to do so. I'm no way qualified to produce much of the text but
> perhaps can assist in getting things moving.
>
>> Finally finally... Dan Williams [CC'd] has plans for doing some
>> maintainer-level documentation.  He may have thoughts on how this fits
>> into what he's scheming, and I'd suggest copying him on the next
>> iteration.
>
> Let's liaise on this Dan (if you want to).
>
>> Finally finally finally...some specific comments on the text.  Some of
>> them might be read to suggest a major expansion of the work you've done;
>> please see that as me saying "that would be nice".  Doing all of this is
>> not a precondition to getting this document added!
>
> There is no rush to get merged, let's get it into shape first :)
>
>> > +Submitting Pull Requests to Linus: a guide for maintainers
>> > +==========================================================
>> > +
>> > +This document is aimed at kernel maintainers.  It describes a method for creating
>> > +a pull request to be sent to Linus.
>> 
>> Limiting text widths to, say, 75 columns when possible is preferable.  Word
>> has it some maintainers are still reading the docs on their adm3a
>> terminals.
>
> Got it. (idea for next doc 'column widths howto' - your canonical guide
> to column widths (includes git brief, commit log, email, source code,
> and docs).
>
> I'm kidding. 75 it is.
>
>> Most maintainers push directly to Linus, so that's an obvious best focus,
>> but pull requests happen at other levels too.  One would hope that this
>> information would be applicable at all levels, so it might be nice to
>> describe it as such.
>
> Oh, Greg had this, I stripped it out. Back in on next spin.
>
>> > +Configure Git
>> > +-------------
>> 
>> "Configure Git to use your private key"
>> 
>> We are, of course, missing the whole discussion on why one would want a
>> keypair, how to create it, how to get it into the web of trust, etc.  All
>> fodder for a separate chapter in our shiny new maintainer book :)  But it
>> is worth saying at least that this is about making Git use your key so you
>> can sign tags for pull requests.
>
> Funny you should say that, I'm trying to get into the web of trust so
> perhaps I can help with that document (as I work out how to do it).
>
>> > +Since you _usually_ would use the same key for the same project, just set it
>> > +once with
>> 
>> If you end a line like that with "::", the following indented section will
>> be formatted as code by sphinx.  That's almost always what you want.
>> 
>> > +	git config user.signingkey "keyname"
>
> cool.
>
>> > +
>> > +and if you use the same key for everything, just add "--global".
>> > +
>> > +Or just edit your .git/config or ~/.gitconfig file by hand, it's designed to be
>> > +human-readable and writable, and not some garbage like XML:
>> > +
>> > +	[torvalds@i7 ~]$ head -4 .gitconfig
>> > +	[user]
>> > +		name = Linus Torvalds
>> > +		email = torvalds@...ux-foundation.org
>> > +		signingkey = torvalds@...ux-foundation.org
>> > +
>> > +You may need to tell git to use gpg2
>> > +
>> > +	[gpg]
>> > +		program = /path/to/gpg2
>> > +
>> > +You may also like to tell gpg which tty to use (add to shell rc file)
>> > +
>> > +	export GPG_TTY=$(tty)
>> > +
>> > +
>> > +Branch, Tag, Push
>> > +-----------------
>> > +
>> > +Next, put your changes on a branch, hopefully one named in a semi-useful way (I
>> > +use 'char-misc-next' for my char/misc driver patches to be merged into
>> > +linux-next).  That is the branch you wish to tag and have Linus pull from.
>> 
>> Management of patches and branches would, of course, make for another nice
>> chapter.
>
> Not maintainer specific though, right? 
>
>> > +Name the tag with something useful that you can understand if you run across it
>> > +in a few weeks, and something that will be "unique".  Continuing the example of
>> 
>> Greg likes to put quotes in weird places, but we don't need to preserve
>> that :)  Git will force the tag to be "unique", so we can just say unique. 
>
> He also adds two spaces in between sentences, that threw me. He is
> correct though, I intend on imitating.
>
>> > +the char-misc tree, for the patches to be sent to Linus for the 4.15-rc1 merge
>> > +window, I would name the tag 'char-misc-4.15-rc1':
>> > +
>> > +	git tag -s char-misc-4.15-rc1 char-misc-next
>> > +
>> > +that will create a signed tag called 'char-misc-4.15-rc1' based on the last
>> > +commit in the char-misc-next branch, and sign it with your gpg key (configured
>> > +above).
>> > +
>> > +When you run the above command, git will drop you into an editor and ask you to
>> > +describe the tag.  In this case, you are describing a pull request, so outline
>> > +what is contained here, why it should be merged, and what, if any, testing has
>> > +happened to it.  All of this information will end up in the tag itself, and then
>> > +in the merge commit that Linus makes, so write it up well, as it will be in the
>> > +kernel tree for forever.
>> 
>> s/for//
>> 
>> Sphinx will format the following indented text differently, which may not
>> be what you want.  I think you should really introduce it with "Linus said
>> this:" perhaps with a link to the list archive.
>
> Ok, perhaps there are examples currently in tree of how best to
> quote. I'll dig around.
>
>> > +	Anyway, at least to me, the important part is the *message*. I want to
>> > +	understand what I'm pulling, and why I should pull it. I also want to
>> > +	use that message as the message for the merge, so it should not just
>> > +	make sense to me, but make sense as a historical record too.
>> > +
>> > +	Note that if there is something odd about the pull request, that
>> > +	should very much be in the explanation. If you're touching files that
>> > +	you don't maintain, explain _why_. I will see it in the diffstat
>> > +	anyway, and if you didn't mention it, I'll just be extra suspicious.
>> > +	And when you send me new stuff after the merge window (or even
>> > +	bug-fixes, but ones that look scary), explain not just what they do
>> > +	and why they do it, but explain the _timing_. What happened that this
>> > +	didn't go through the merge window..
>> > +
>> > +	I will take both what you write in the email pull request _and_ in the
>> > +	signed tag, so depending on your workflow, you can either describe
>> > +	your work in the signed tag (which will also automatically make it
>> > +	into the pull request email), or you can make the signed tag just a
>> > +	placeholder with nothing interesting in it, and describe the work
>> > +	later when you actually send me the pull request.
>> > +
>> > +	And yes, I will edit the message. Partly because I tend to do just
>> > +	trivial formatting (the whole indentation and quoting etc), but partly
>> > +	because part of the message may make sense for me at pull time
>> > +	(describing the conflicts and your personal issues for sending it
>> > +	right now), but may not make sense in the context of a merge commit
>> > +	message, so I will try to make it all make sense. I will also fix any
>> > +	speeling mistaeks and bad grammar I notice, particularly for
>> > +	non-native speakers (but also for native ones ;^). But I may miss
>> > +	some, or even add some.
>> > +
>> > +			Linus
>> > +
>> > +An example pull request of mine might look like:
>> 
>> Here's a change of voice back to Greg.  Be careful about appearing to put
>> one person's words into another's mouth.
>
> Agreed. (commented on above).
>
>> Here you definitely want the :: treatment, or sphinx will whine about the
>> strange (to it) indents.
>> 
>> > +	Char/Misc patches for 4.15-rc1
>> > +
>> > +	Here is the big char/misc patch set for the 4.15-rc1 merge
>> > +	window.  Contained in here is the normal set of new functions
>> > +	added to all of these crazy drivers, as well as the following
>> > +	brand new subsystems:
>> > +		- time_travel_controller: Finally a set of drivers for
>> > +		  the latest time travel bus architecture that provides
>> > +		  i/o to the CPU before it asked for it, allowing
>> > +		  uninterrupted processing
>> > +		- relativity_shifters: due to the affect that the
>> > +		  time_travel_controllers have on the overall system,
>> > +		  there was a need for a new set of relativity shifter
>> > +		  drivers to accommodate the newly formed black holes
>> > +		  that would threaten to suck CPUs into them.  This
>> > +		  subsystem handles this in a way to successfully
>> > +		  neutralize the problems.  There is a Kconfig option to
>> > +		  force these to be enabled when needed, so problems
>> > +		  should not occur.
>> > +
>> > +	All of these patches have been successfully tested in the latest
>> > +	linux-next releases, and the original problems that it found
>> > +	have all been resolved (apologies to anyone living near Canberra
>> > +	for the lack of the Kconfig options in the earlier versions of
>> > +	the linux-next tree creations.)
>> > +
>> > +	Signed-off-by: Your-name-here <your_email@...ain>
>> > +
>> > +
>> > +The tag message format is just like a git commit id.  One line at the top for a
>> > +"summary subject" and be sure to sign-off at the bottom.
>> 
>> FWIW, I've never formatted tag messages that way, and I'm not sure how many
>> others do.  But perhaps we should all be doing it?
>
> Hopefully the patches going in will be reviewed by other maintainers and
> suggestions will flow :) 
>
>> > +Now that you have a local signed tag, you need to push it up to where it can be
>> > +retrieved by Linus:
>> > +
>> > +	git push origin char-misc-4.15-rc1
>> > +
>> > +pushes the char-misc-4.15-rc1 tag to where the 'origin' repo is located.
>> > +
>> > +
>> > +Create Pull Request
>> > +-------------------
>> > +
>> > +The last thing to do is create the pull request message.  Git handily will do
>> > +this for you with the 'git request-pull' command, but it needs a bit of help
>> > +determining what you want to pull, and what to base the pull against (to show
>> > +the correct changes to be pulled and the diffstat.)
>> > +
>> > +The following command(s) will generate a pull request:
>> > +
>> > +	$TREE=git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git/
>> 
>> I don't believe that $ is correct
>
> Bad Tobin, no biscuit. (read: cookie)
>
>> > +	git request-pull master $TREE char-misc-4.15-rc1
>> > +
>> > +This is asking git to compare the difference from the 'char-misc-4.15-rc1' tag
>> > +location, to the head of the 'master' branch (which in my case points to the
>> > +last location in Linus's tree that I diverged from, usually a -rc release).
>> > +
>> > +Note: please use the git protocol (for justification from Linus see referenced
>> > +email thread).
>> 
>> We need a reference to that thread.
>> 
>> > +If the char-misc-4.15-rc1 tag is not present in the repo that I am asking to be
>> > +pulled from, git will complain saying it is not there, a handy way to remember
>> > +to actually push it to a public location.
>> > +
>> > +The output of 'git request-pull' will contain the location of the git tree and
>> > +specific tag to pull from, and the full text description of that tag (which is
>> > +why you need to provide good information in that tag.)  It will also create a
>> > +diffstat of the pull request, and a shortlog of the individual commits that the
>> > +pull request will provide.
>> > +
>> > +
>> > +References
>> > +----------
>> > +
>> > +The thread that this document is based on:
>> > +
>> > +	https://lkml.org/lkml/2017/11/14/184
>> 
>> Ah, there's that reference.  I think it should be at the top before you
>> first start quoting from it.
>
> Perhaps (at start of document)
>
>     This document was written by Tobin C. Harding (not an experienced
>     maintainer) primarily from emails by Greg Kroah-Hartman and Linus
>     Torvalds. Suggestions and fixes by Jonathan Corbet. Misrepresentation
>     was unintentional but inevitable, please direct abuse to "Tobin
>     C. Harding" <me@...in.cc>. 
>
>     Original email thread 
>
>         https://lkml.org/lkml/2017/11/14/184

Please use http://lkml.kernel.org/r/20171114110500.GA21175@kroah.com so
people can find the messages using the message-id.

Please consider using reStructuredText references instead of a
semi-random looking indented block.

BR,
Jani.


>
>> I think there's something missing here: what do you do with that output
>> from 'git request-pull'?  There should be a little section on emailing it
>> to the relevant upstream maintainer and how to decide where to copy the
>> request to.  Pull requests should always be copied to a public list so that
>> others know that the request has been made.  Some guidance on subject-line
>> formatting would be good; as I recall, Linus filters mail that says "git"
>> or "pull" specially.  I might also add something about how to know when the
>> pull has happened (sign up to the commits list if nothing else).
>> 
>> Thanks for doing this,
>
> Cheers Jon, nice to work with you.
>
> Tobin.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-doc" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Jani Nikula, Intel Open Source Technology Center

Powered by blists - more mailing lists