Hi contributors!

Hi All,

I don't know if this has ever been said on this list or if this is even the best place for it. I have no problems sending to the openbmc wiki or anywhere else people might find useful. If it has been said before then, well I've wasted my time.

It has become increasingly visible that many contributors aren't as intimately familiar with, for lack of a better term, opensource development.

There are three things I aim to help with here.
Firstly, sending patches that everyone can work with.
Secondly, git, by now everyone should have realised that git is an incredibly complex and powerful tool, I am probably not the best person to teach the intricacies of git but I can try to help.
Lastly the mailing list and I am well aware (too much, trust me!) that much of these problems are outside the control of people here but I'll say it anyway.

Don't forget that Jeremy has written something that everyone should have read:
https://github.com/openbmc/docs/blob/master/contributing.md. Step 1 for any project should be reading their equivalent.

-----------------------------------------------------------------------------------
Being a good citizen.
Lets us go back to what Jeremy wrote:
https://github.com/openbmc/docs/blob/master/contributing.md contains a very important link
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches
When I said you should read what Jeremy wrote, it was implied that you should follow links and read those too.

I know that SubmittingPatches is long and you'll be tempted to skim read it, so I'm going to take the time to highlight some parts that I think should be read by all contributors to this list.

Let us start at point 2 "Describe your changes." Please read the entire section, I'll quote parts here purely for emphasis. "describe what you are actually doing about it in technical detail" and "Solve only one problem per patch." and "try to make your explanation understandable without external resources".

Point 3:
Again, please read the entire thing, this one is short and very relevant. This was written for the kernel but this applies to any project. We should ensure openbmc builds and runs property *after each patch*. This is very important, the best argument is git bisect. Having bisectability is a powerful tool and we want to be able to use that as much as possible. There have been a lot of series posted to this list where one patch fixes a problem introduced by a previous patch. There are many ways to solve this using git, I'll explain in the git section.

By now you should have realised that point 4 is very important. We are following the kernel style guide so the first sentence is relevant. We do not have a checkpatch script, just make sure you do the best you can to check your style.

I know point 6 overlaps with what I'm going to talk about later but please please please try to stay plain text!

Do not forget point 11 "Sign your work". Jeremy kindly copied the Developer's Certificate of Origin into the guide and as the kernel docs say "then you just add a line saying

using your real name (sorry, no pseudonyms or anonymous contributions.)"

The real name and trackable email address are important here!

Finally, the first reference is quite good - consider reading it too. Andrew Morton, "The perfect patch" (tpp).
<http://www.ozlabs.org/~akpm/stuff/tpp.txt>

-----------------------------------------------------------------------------------
Git!
I assume everyone here is at a basic level. I have done some googling and there are more detailed explanations but this one is concise and to the point.
http://stackoverflow.com/questions/20956154/whats-the-workflow-to-contribute-to-an-open-source-project-using-git-pull-reque
There is one comment on there that you can (and should) ignore:
"Also, it's recommended to squash your commits so that the pull request only has one big commits – Heisenberg Jul 22 '15 at 19:28"

Firstly, all contributors should run:
git config --global user.email "[email protected]" and
git config --global user.name "Your full name"

So for me I've done:
git config --global user.email "[email protected]"
git config --global user.name "Cyril Bur"
I've specifically not done anything like set my github username or an email I don't check or don't contribute from:
git config --global user.email "[email protected]" or
git config --global user.name "cyrilbur-ibm"
Special mention to IBMers, your @yy.ibm.com username as your full name isn't good enough! Your full name probably has at least one space in it.

I've looked around for some online guides. There are so many on "Create branch, commit something, send upstream" but not many "So you realised your stuff is broken" "a reviewer has made some good points and you need to address them" or even "Ah help my tree got a bit crazy I need to clean it up" guides.

The frustrating thing with git can be that you want to do something but it can be quite hard to find out how.

Before I give any commands, be careful, "Perfect is the enemy of good" (sigh English), while you can make your tree much cleaner with these methods, you can undoubtedly make things much worse.

Let us introduce options to git commit, notably '--amend'. --amend: This allows you to add to the commit to the top of your tree. So if you just committed something and realise that you forgot a semicolon, --amend is your friend.

--amend only allows you to change the top commit, lets say you need to change a commit further down. git rebase --interactive is helpful here. The most basic use of git rebase --interactive is if origin changes while you're working on your feature.
Assuming you have your feature branch checked out, you can simply do git rebase --interactive origin/master and you'll be presented with this:
pick 4d01a04 Add new interface
pick bbafdd7 Add tests for cool new interface
pick f75f556 Expose new interface to module_x()
pick 60190ca Use new interface in function_a()
pick fcd6a13 Use new interface in function_b()

At this point, if you simply exit your editor, your branch to add new interface will now be (re)based on upstream, starting with 4d01a04 and applying commits DOWN the list. But wait there's more, you'll notice there's help text:
# Commands:
# p, pick = use commit
# r, reword = use commit, but edit the commit message
# e, edit = use commit, but stop for amending
# s, squash = use commit, but meld into previous commit
# f, fixup = like "squash", but discard this commit's log message # x, exec = run command (the rest of the line) using shell
# d, drop = remove commit

So in the above example I could have changed the 'pick' words at the start of each line with one of those commands and done more.
Lets pretend those commits looked more like this:
pick 4d01a04 Add new interface
pick bbafdd7 Add tests for cool new interface
pick f75f556 Fix new interface to pass tests
pick 60190ca Add more tests
pick fcd6a13 Use new interface in function_b()

Now, I don't want to send this series as is. If I do it means that just applying 4d01a04 leaves the tree in a broken state until f75f556 is applied. Possibly even worse (but we're just talking levels of bad here), if only 4d01a04 and bbafdd7 are applied then tests will fail, that's never good! Lets use the power of rebase, I can manually edit what is being presented to me to do:
pick 4d01a04 Add new interface
fixup f75f556 Fix new interface to pass tests
pick bbafdd7 Add tests for cool new interface
fixup 60190ca Add more tests
pick fcd6a13 Use new interface in function_b()

Also notice that I did a fixup of 60190ca into bbafdd7 because it really makes no sense to have two patches adding tests for the same interface.

These features a incredibly useful for sending clean and logical series. Don't forget about reword, it is critical to ensure commit messages are as clear and as descriptive as possible.

It might be wise to say that a rebase might have conflicts, in which case git will stop the rebase when it detects a conflict and ask you what to to do. It will put you at the point where it detects on the conflict with both possibilities in the file where the problem is. You can run git diff to quickly see what's going on and start fixing the problem. Git gives you some tips on what to do, just know that at this point git rebase --abort is perfectly safe and will put you back where you were at before you started the rebase.

One final note with rebase, git won't let you do it if you have uncommitted changes in your tree, but that's ok, fixing up the tree is probably the last thing you want to do so that shouldn't be a problem.

This isn't the optimal way of dealing with these problems and most likely not the only way, but it is a way which works. If you want to dive more into git, I'll let you understand what the options '--fixup' and '--squash' to git commit do.

Like all the git tutorials out there, this example is fake and not real world, mostly because no one ever just works on one aspect of a project without simultaneously working on some other part. That means that having a clean tree can be a challenge. Historically there is a rather adhoc way of dealing with this problem. I won't mention that here because I'm pretty sure no one has ever used stashes without making a mess.

Somewhat recently git introduced 'worktree's, this is a decent explanation of what it does:
https://developer.atlassian.com/blog/2015/10/cool-features-git-2.x/ have a read, it will make your life easier.

-----------------------------------------------------------------------------------
The mailing list...
I'm going to outline problems as I see them, I know that it may prove impossible to fix some or all of them and that's ok but I would encourage anyone to apply pressure to those who can fix the problems for you because not being able to send:
a) Plaintext
b) Whitespace correct
c) Correct headers
d) Standards compliant
email in 2016 is a little embarrassing. In most cases we're embarrassed for you but not at you. If it is at all possible, setup a gmail account, use git format-patch and/or git send-email for patches and participate on the list through gmail. I'm happy to provide howto links, just ask (this is getting a bit long that's all).

I will admit that github pull requests have been a good attempt at trying to solve these problems but has quite a few side problems. It creates a lot of spam. I know I've been guilty of this (although I don't know how) but it posts new versions of series really easily, all I can say here is be careful.

Perhaps we should start a system of responding to superseded versions. Maintainers could respond to indicate that they have merged or closed requests. I have often found myself reviewing code which has been merged. Doing so isn't useless but had I known, probably would have spent my time reviewing unmerged code.

There can often be 5+ versions with no replies which has left me wondering if I'm looking at the most recent version, also, these versions sometimes aren't different (github I know), could we start a habit of better tracking (almost ironically with more emails) to help people reviewing know where their time is best spent. If your series gets multi posted, please indicate in the older versions that they can be ignored.

On reviewing: we should all be participating and once again, in plain text and inline responses. Please don't ignore comments, address everything, respond to the viewer and let them know what you think of their comments. Usually posting the next version involves sending more emails and normally a cover letter would be used to indicate the changes. With the github pull requests, I'm *told*, you can edit the first comment of the pull request and it comes up in the cover letter of the subsequent version. PLEASE DO THIS! It makes life so much easier for everyone when a v2 says how it is different from v1.

On cover letters: they aren't mandatory or anything but there are a lot of pull requests that appear on the list as bunch of patches that are already lacking in the commit message quality so the lack of cover letter is not helping. A great tip I can give here for cover letters and commit messages is assume you're talking a new member of your team who hasn't seen the code before but is expected to understand it soon. You need to describe the problem and how you've gone about fixing it.

Finally, this is more of a request to the admins. Is it possible to fix the list so that the 'Reply to all' button works like it does on other lists? If I go to linuxppc-dev and click 'Reply to all' to review a patch I get "To:
Patch Author" and "Cc: Everyone orignally CCed and the list".
If I do that on a patch on openbmc it is 'To: "OpenBMC Patches <[email protected]>"' (because it was originally from that address) and sometimes the author isn't on Cc and other times the list isn't on Cc.

I won't drag out the ending, thanks so much for reading this! Send me an email if you've got any questions! I'll endeavour to put a cut down version of some of this in the docs repo and maybe put an (expanded) git help up online in a more permanent place.

Sincerely,

Cyril

Cyril,

You wrote a lot.

On commit messages:
I totally agree. Poor commit messages are a pet peeve of mine as well. I also recommend http://chris.beams.io/posts/git-commit/ . Going forward, I will attempt to do a better job of encouraging better formed messages in reviews.

On git documentation:
Thanks for the write up. Rather than put yet another git tutorial in a wiki, I would prefer we point to a good existing tutorial. For some reason new people seem to always want a "cookbook" of simple git commands. [Un]Fortunately, nothing in git is that simple.

On Reviews:
We do have a problem with some people reviewing in Github (Reviewable.io) and some people reviewing via mailing list. We also have an issue with maintainers not ensuring all comments have been addressed.

We discussed at the Wed (5/18 PM for US) call last week how to do reviews going forward. We agreed to the following:
1) For OpenBMC specific packages we are going to set up a Gerrit server for all code reviews. There is a Github integration module for Gerrit that allows mirroring pull requests into Gerrit and replicating merged code back to Github. Look for this in the near future.
2) For packages we are modifying from upstream, we should use upstream method (ie. mailing lists for kernel, yocto, etc.). Patches accepted into the openbmc meta-layers should have already be submitted upstream and have a reasonable confidence about their long-term acceptance.
3) For packages we are _temporarily_ forking from upstream, to do significant and focused development on, we can use either Gerrit or this mailing list. Eventually, changes should be submitted, via path #2, upstream.

Once we get the Gerrit server in place, I plan to deprecate the automatic pull-request mailer.

On 'Reply-All' issue with patches:
At some level, this is an intrinsic problem with Github. People can optionally set their Github profile to "Hide my email address whenever possible." The content for the cover letter comes from the pull request information they have entered into the Github web-field. When my patch mailer gets the content from the pull request to make the cover letter, it has to ask Github for their email address. For people who have hidden their email address in their profile this yields something like "Joe Developer <[email protected]>" (I forget the exact format off the top of my head). Unfortunately, this means my auto-mailer and anyone responding to the cover gets lots of bounces, so I disabled the auto-cc on the cover. (There is an obvious solution to filter out the @...github.com email addresses but I haven't had the spare cycles).

For the code patches themselves, I use git-format-patch and git-send-email and I don't think I suppress-cc at all. That means the author, per the commit message, should be cc'd.

Like I said early, this script is planned to be deprecated at some point in the not-too-distant future anyhow.

Patrick Williams

Someone in ozlabs (who heard me ranting over the past few days as I wrote all that) pointed out that part of the issue there is that good commit or bad commit message, code gets merged. There is no incentive for contributors to put any effort there.

Yeah I won't put it in the wiki. I didn't expect to have to write quite as much myself, when I started I thought I'd put a series of links to existing stuff but all I could find was the 'perfect workflow' style thing that didn't address what I (hope) I did.

There's a blog I sometimes contribute to, I think I'll expand a bit and write a full post there if even more googling doesn't yield a good existing tutorial

Word of this has peculated to us in Canberra. Admittedly a lot of us are stuck in our ways for better and (probably worse). After the debacle involving github/reviewable I will admit I wasn't thrilled to hear that some other tool was being considered and we aren't just going to use mailing lists.

Joel did step in here and back Gerrit and say that it can be used to improve productivity and not be a hindrance. Of course it remains to be seen if our employer who fails at giving people correct emails solutions can give us a 'real and useable' Gerrit instance.

Can't say I have strong opinions, 3 feels like it is just a subset of 2 though...

I was afraid of this

Sounds like the sooner we move away from the github solution the better, this problem goes away once we inforce a 'this is through Gerrit' or 'this is through the list' and github can stop polluting the list.

Looking forward to a new and improved openbmc soon!!