On Wed, Nov 15, 2017 at 04:09:32PM +0200, Jani Nikula wrote: > On Wed, 15 Nov 2017, "Tobin C. Harding" <me@xxxxxxxx> 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@xxxxxxxx> 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@xxxxxxxx> > >> > --- > >> > > >> > 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@xxxxxxxxxxxxxxxxxxxx > >> > + signingkey = torvalds@xxxxxxxxxxxxxxxxxxxx > >> > + > >> > +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@xxxxxxxx>. > > > > Original email thread > > > > https://lkml.org/lkml/2017/11/14/184 > > Please use http://lkml.kernel.org/r/20171114110500.GA21175@xxxxxxxxx so > people can find the messages using the message-id. How did you generate this URL? Is this the format that should be used whenever linking to LKML messages i.e when including links in mail being sent to LKML? > Please consider using reStructuredText references instead of a > semi-random looking indented block. Yep, thanks. Amateur effort this one. Expect better for next spin. thanks, Tobin. -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
