Re: [PATCH] doc: Hold down best practices for pull requests

[Randy Dunlap <rdunlap@infradead.org>]

On 03/03/13 04:43, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 
> Hold down some important points to pay attention to when preparing a
> pull request to upper-level maintainers and/or Linus. Based on a couple
> of agitated mails from Linus to maintainers and random crowd sitting
> around.
> 
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
> 
> Ok,
> 
> so here's what I have. I took your message from yesterday and
> morphed/fleshed it out into a set of points to pay attention to when
> prepping a pull request. I also sampled some more rants from last year
> to get a couple more recurring issues.
> 
> I hope I've covered the most important points.
> 
> Thanks.
> 
>  Documentation/SubmittingPullRequests | 148 +++++++++++++++++++++++++++++++++++
>  1 file changed, 148 insertions(+)
>  create mode 100644 Documentation/SubmittingPullRequests
> 
> diff --git a/Documentation/SubmittingPullRequests b/Documentation/SubmittingPullRequests
> new file mode 100644
> index 000000000000..d123745e0cf5
> --- /dev/null
> +++ b/Documentation/SubmittingPullRequests
> @@ -0,0 +1,148 @@
> +	Trees, merges and other hints for maintainers of all levels
> +	===========================================================
> +
> +... or how do I send my tree to Linus without him biting my head off.
> +
> +
> +This is a collection of hints, notes, suggestions and accepted practices
> +for sending pull requests to (other maintainers which then forward those
> +trees to) Linus. It is loosely based on Linus' friendly and polite hints
> +to maintainers on the Linux Kernel Mailing List and the idea behind it
> +is to document the most apparent do's and dont's, thus saving time and
> +money of everyone involved.
> +
> +Basically, pull requests and merge commits adhering to the guidelines
> +described below should establish certain practices which make
> +development history as presentable, useful and sensible as possible.
> +
> +People staring at it should be able to understand what went where, when
> +and why, without encountering senseless merge commits and internal
> +subsystem development state which don't have any bearing on the final,
> +cast-in-stone history. A second, not less important reason is tree
> +bisectability.
> +
> +Here we go:
> +
> +0.) Before asking any maintainer to pull a pile of patches, make damn
> +well sure that said pile is stable, works, and has been extensively,
> +thoroughly and to the best of your abilities, tested. There's absolutely
> +no reason in rushing half-cooked patches just because your manager says
> +so. Rule of thumb is: if the patches are so done that they're boringly
> +missing any issues or regressions and you've almost forgotten them in
> +linux-next, *then* you can send.
> +
> +1.) The patchset going to an upper level maintainer should NOT be based
> +on some random, potentially completely broken commit in the middle of a
> +merge window, or some other random point in the tree history.
> +
> +Tangential to that, it shouldn't contain back-merges - not to "next"
> +trees, and not to a "random commit of the day" in Linus' tree.
> +
> +Why, you ask?
> +
> +Linus: "Basically, I do not want people's development trees to worry
> +about some random crazy merge-window tree of the day. You should always
> +pick a good starting point that makes sense (where "makes sense" is very
> +much about "it's stable and well known" and just do your own thing. What
> +other people do should simply not concern you over-much. You are the
> +[ ... ] maintainer, and your job is not integration, it's to make sure
> +that *your* work is as stable and unsurprising as possible."
> +
> +2.) Write a sensible, human-readable mail message explaining in terms
> +understandable to outsiders what your patchset entails, maybe describe
> +the high-level big picture of where your patchset fits and why. And
> +do not use abbreviations - they might be clear to you and the people
> +working on your subsystem but not necessarily to the rest of the world.
> +Also, include the diffstat in that mail (git request-pull should take
> +care of all that nicely).
> +
> +3.) GPG-sign your pull request and put the high-level description you've
> +created into the signed tag. Of course, your key should be signed by
> +fellow developers who are in the chain of trust of the receiving end of
> +your pull request.
> +
> +4.) The URL of your pull request should contain the signed tag. Make
> +sure that URL is valid and externally accessible. This is especially
> +valid for the k.org crowd who get it wrong a *lot*. Also, people tend to
> +forget to push the signed tag.
> +
> +5.) THEN AND ONLY THEN do we start worrying about possible merge issues
> +your pull request could cause with upstream. It can be very helpful if
> +you look into and describe them in the pull request mail, maybe even do
> +an *example* merge. This merge won't normally be used but it is very
> +helpful to show the person doing the merge how to resolve possible
> +conflicts.
> +
> +Here's Linus counting the ways why you shouldn't make merges yourself:
> +
> +" - I'm usually a day or two behind in my merge queue anyway, partly
> +because I get tons of pull requests in a short while and I just want
> +to get a feel for what's going on, and partly because I tend to do
> +pulls in waves of "ok, I'm going filesystems now, then I'll look at

                              doing ?

> +drivers".
> +
> + - I do a *lot* of merges. I try to make them look good, and have
> +*explanations* in them. And if a merge conflict happens, I want to
> +know about them.
> +
> + - I want to have a gut feel about what goes into the tree when. I
> +know, for example, that "oops, we had a bug in ext4 that got
> +discovered only after the merge, and for a while there it didn't work
> +well with larger disks". When people complain, my job is often to have
> +that high-level view of "ok, you're hitting this known bug". If people
> +do back-merges, that basically pulls in my tree at some random point
> +that I'm not even aware of, and now you mixed up *other* peoples bugs
> +into your tree.
> +
> + - and somewhat most importantly (for me personally): backmerges make
> +history messy, and it can be a huge pain for me when I do merge
> +conflict resolution, or when other people do bisects etc. It's *much*
> +nicer in many ways (visually, and for bisect reasons) to try to have
> +as much independent development as possible."
> +
> +One more from LKML history: it can happen that your merge *actually*
> +breaks upstream because it refers to symbols which are being removed by
> +another, previous merge. So don't merge.
> +
> +6.) Avoid an excessive amount of senseless cross-merges. Think of
> +it this way: a merge appearing in the final git history has to mean
> +something, it has to say why it is there. So, having too many pointless
> +merges simply pollutes the history and devalues it. Instead, think of
> +a good point to do a cross merge, do it and *document* exactly why it is
> +there.
> +
> +7.) And while we're at it, you should *almost* *never* rebase your
> +tree. More so if it has gone public and people are possibly relying on
> +it to remain stable and basing their stuff on top - especially then
> +you're absolutely not allowed to rebase it anymore. But that's not that
> +problematic if you've adopted the incremental development model and your
> +tree is at least as well-cooked as in 1) above.
> +
> +IOW, "rebasing has other deeper problems too, like the fact that your

missing ending '"' somewhere.

> +tree is no longer something that other people can depend on. That's not
> +a big issue for a new architecture, but it's a big issue going forward.
> +Which is why rebasing is generally even *worse* than back-merges (but
> +both are basically big "just don't do that").
> +
> +So the rule for both rebasing and back-merging should be:
> +
> + - you should have damn good reasons for it, and you should document
> +them.
> +
> + - you should basically *never* rebase or back-merge random commits in

hm, sometimes it is backmerge and other times it is back-merge.  Please be
consistent.

> +the merge window. That's *NEVER* a good idea. If you have a conflict,
> +ignore it. Explain it to me when you ask me to pull, but it is *my* job
> +to know "ok, I've pulled fiftynine different trees, and trees X and Y

                            fifty-nine

> +end up conflicting, and this is how it got resolved". Seriously.
> +
> + - if you have some really long-lived development tree, and you want to
> +back-merge just to not be *too* far away, back-merge to actual releases.
> +Don't pull my master branch. Say "git merge v3.8" or something like
> +that, and then you write a good merge message that talks about *why* you
> +wanted to update to a new release."
> +
> +8.) After the maintainer has pulled, it is always a good idea to take a
> +look at the merge and verify it has happened as you've expected it to,
> +maybe even run your tests on it to double-check everything went fine.
> +
> +Further reading: Documentation/development-process/*
> 


Looks good and useful overall.

thanks,
-- 
~Randy