lkml.org 
[lkml]   [2017]   [Nov]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] docs: add submitting-pull-requests.rst
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@tobin.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@tobin.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@linux-foundation.org
> > + signingkey = torvalds@linux-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@domain>
> > +
> > +
> > +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@tobin.cc>.

Original email thread

https://lkml.org/lkml/2017/11/14/184

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

\
 
 \ /
  Last update: 2017-11-15 03:30    [W:0.086 / U:0.604 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site