Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
On Fri, Jun 29, 2012 at 03:27:25PM -0500, Andrew Bogott wrote: On 6/27/12 8:40 AM, Daniel P. Berrange wrote: On Wed, Jun 27, 2012 at 03:24:21PM +0200, Vincent Untz wrote: Hi, It'd be really great if we could first improve Gerrit to handle the patch series workflow in a better way. Without such a change, pushing patch series to Gerrit is really no fun for anyone :/ Yep, no argument that Gerrit could do with some improvements, but having submitted a number of non-trivial patch series to Nova, I don't think current Gerrit UI is a complete blocker to adoption. It is not ideal, but it isn't too painful if you're aware of what to look for. I think the main problem is that since the patch dependancies are not obvious in the UI, reviewers tend to miss the fact that they're reviewing a patch that's part of a series. I agree that patchsets are better than monolithic patches. Today, though, I am working on a 3-patch set and the process is driving me crazy. a) Any time Jenkins has a hiccup, I have to resubmit the entire patchset. This obscures any reviews or votes that might be attached to other patches in the set. Yeah, this is a major PITA. We need an easy way for patch authors to retrigger Jenkins without re-submitting each time. b) Similarly, any time I change a single patch in the set, I have to resubmit the whole set, which causes review history to be obscured, even for those patches which have not changed at all. Gerrit will look at the GIT changeset hashes and notice if only 2 out of the 5 patches have actually changed. THe trouble is that 'git review' with no args will always rebase your patch series against master before pushing. So even if you only modified the last patch in your series, this will make Gerrit see all the patches as new :-( I'm getting into the habit of always running 'git review --no-rebase' to get around this behaviour. Case b) would be entirely solved via a fix to this: http://code.google.com/p/gerrit/issues/detail?id=71. That would also help with a) but not resolve it entirely... the best solution to a) would be a 'retrigger' button in Jenkins or a 'prompt Jenkins to re-review' button in Gerrit. The fact that people (including me) are submitting trivial edits to patches only in order to nudge Jenkins is pretty stupid. I must say that this has been driving me mad this last week. IIUC, only members of the core review team have permission to retrigger Jenkins, but I feel it is putting too much burden on them to have to track every patch with a bogus Jenkins failure. If we can't get Jenkins to be more reliable, then can we see about letting patch submitters retrigger Jenkins builds for their own patches. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
On 07/02/2012 05:14 AM, Daniel P. Berrange wrote: I must say that this has been driving me mad this last week. IIUC, only members of the core review team have permission to retrigger Jenkins, but I feel it is putting too much burden on them to have to track every patch with a bogus Jenkins failure. If we can't get Jenkins to be more reliable, then can we see about letting patch submitters retrigger Jenkins builds for their own patches. There's a whole other thread about this at the moment, which outlines the problems and what we're doing about it. One of the main issues should be already solved. The primary goal is to get jenkins to be more reliable so that hiccups don't happen in the first place. In any case, if you want all the details, please see James Blair's recent email to the Jenkins and Transient Failures thread. Monty ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
Hey! I agree with most of this in general. A few comments about a section I just happened to read: On 06/27/2012 06:52 AM, Daniel P. Berrange wrote: snip Including external references - The commit message is primarily targetted towards human interpretation, but there is always some metadata provided for machine use. In the case of OpenStack this includes at least the 'Change-id', but also optional bug ID references and blueprint name references. Although GIT records the author committer of a patch, it is common practice across many open source projects to include a Signed-off-by tag. Though OpenStack does not mandate its use, the latter is still useful to include if a patch is a combination of work by many different developers, since GIT only records a single author. All machine targetted metadata, however, is of secondary consequence to humans and thus it should all be grouped together at the end of the commit message. For example: Switch libvirt get_cpu_info method over to use config APIs The get_cpu_info method in the libvirt driver currently uses XPath queries to extract information from the capabilities XML document. Switch this over to use the new config class LibvirtConfigCaps. Also provide a test case to validate the data being returned Fixes: bug #1003373 Implements: blueprint libvirt-xml-cpu-model These two are fine in general - although we actually parse and do bug and blueprint linking a bit more permissively. I believe our current regexes should still apply to those though. Change-Id: I4946a16d27f712ae2adf8441ce78e6c0bb0bb657 Signed-off-by: Daniel P. Berrange berra...@redhat.com As well as the 'Signed-off-by' tag, there are various other ad-hoc tags that can be used to credit other people involved in a patch who aren't the author. - 'Reviewed-by: ...some name.. ...email...' Although Gerrit tracks formal review by project members, some patches have been reviewed by people outside the community prior to submission I'm not in support of this one. Our review system has no restrictions on who can review things - and I value the actual content of their reviews, which itself is stored in git notes along with the change. So, in general, you can tell me that someone else reviewed it somewhere else, but I don't really care. They should review it in the same place everyone else does. - 'Suggested-by: ...some name.. ...email...' If a person other than the patch author suggested the code enhancement / influnced the design Great idea. - 'Reported-by: ...some name.. ...email...' If a person reported the bug / problem being fixed but did not otherwise file a launchpad bug report. I think this should be strongly discouraged. If there is a bug or problem, it should be filed as a bug report. I don't really know of a valid use case where someone can't do this, but where referencing them here is valid. We have systems, and just as you're suggesting overall that we be a bit stricter in how we do things, I think that being more lax in how we record some of this metadata is counter productive. However - strongly in favor of better commit message practice! Monty ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
Vaze, Mandar wrote: I particularly hate the single-line Fixes bug 1234566-type commit messages. I assume your concern was regarding commits where Fixes bug 1234566 is the first and ONLY line. Yes, that is the particularly offensive form :) Plus there is restriction on how long the first line of the commit message can be. Not everyone is able to describe their change in one short sentence. So typically *I* put Fixes bug 1234567 on the *first* line followed by additional lines describing the change. Brian suggested the following addition to HACKING.rst in https://review.openstack.org/#/c/9118 : The first line of the commit message should provide an accurate description of the change, not just a reference to a bug or blueprint. It must be followed by a single blank line. I agree with him: fixes bug XX is a useless shortlog line that gives you no information whatsoever about the nature of the change. You have to look somewhere else (in the rest of the commit message, in the diff, or on the bug tracker) to have the beginning of an idea. So anything else (even Fixes bug about scheduler) is more useful. Regards, -- Thierry Carrez (ttx) Release Manager, OpenStack ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
On Fri, Jun 29, 2012 at 04:57:06AM +, Vaze, Mandar wrote: I particularly hate the single-line Fixes bug 1234566-type commit messages. I assume your concern was regarding commits where Fixes bug 1234566 is the first and ONLY line. Fixes bug 1234566 comes from Wiki. Plus there is restriction on how long the first line of the commit message can be. Not everyone is able to describe their change in one short sentence. At the very least it is always possible to describe what area of the code is being changed, so that you alert the reviewers you are familiar with that area. So typically *I* put Fixes bug 1234567 on the *first* line followed by additional lines describing the change. IMHO that is one of the most unhelpful things you can do. If you are a reviewer scanning through your email for patches to review and you see a subject line Fixes bug 123456 you are given no useful information. Few people will bother to go find out what 'bug 123456' is, when there are plenty of other patches pending with useful subject lines. It is also pretty useless for people skimming through the online patch summaries in GIT history. As in my first mail this thread, the bug number should be just a line item at the end of the commit message, and the commit message first line should be a complete self-contained description. http://wiki.openstack.org/GerritWorkflow#Committing_Changes should be updated when this discussion is concluded. Yep, totally agreed. Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
At the very least it is always possible to describe what area of the code is being changed, so that you alert the reviewers you are familiar with that area. Yes, Makes sense. -Mandar __ Disclaimer:This email and any attachments are sent in strictest confidence for the sole use of the addressee and may contain legally privileged, confidential, and proprietary data. If you are not the intended recipient, please advise the sender by replying promptly to this email and then delete and destroy this email and any attachments without any further use, copying or forwarding ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
On 6/27/12 8:40 AM, Daniel P. Berrange wrote: On Wed, Jun 27, 2012 at 03:24:21PM +0200, Vincent Untz wrote: Hi, It'd be really great if we could first improve Gerrit to handle the patch series workflow in a better way. Without such a change, pushing patch series to Gerrit is really no fun for anyone :/ Yep, no argument that Gerrit could do with some improvements, but having submitted a number of non-trivial patch series to Nova, I don't think current Gerrit UI is a complete blocker to adoption. It is not ideal, but it isn't too painful if you're aware of what to look for. I think the main problem is that since the patch dependancies are not obvious in the UI, reviewers tend to miss the fact that they're reviewing a patch that's part of a series. I agree that patchsets are better than monolithic patches. Today, though, I am working on a 3-patch set and the process is driving me crazy. a) Any time Jenkins has a hiccup, I have to resubmit the entire patchset. This obscures any reviews or votes that might be attached to other patches in the set. b) Similarly, any time I change a single patch in the set, I have to resubmit the whole set, which causes review history to be obscured, even for those patches which have not changed at all. Case b) would be entirely solved via a fix to this: http://code.google.com/p/gerrit/issues/detail?id=71. That would also help with a) but not resolve it entirely... the best solution to a) would be a 'retrigger' button in Jenkins or a 'prompt Jenkins to re-review' button in Gerrit. The fact that people (including me) are submitting trivial edits to patches only in order to nudge Jenkins is pretty stupid. -A ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
Daniel P. Berrange wrote: [...] In other words, when reviewing a change in Gerrit, do not simply look at the correctness of the code. Review the commit message itself and request improvements to its content. Look out for commits which are mixing multiple logical changes and require the submitter to split them into separate commits. Ensure whitespace changes are not mixed in with functional changes. Ensure no-op code refactoring is done separately from functional changes. And so on. [...] Nice work, and agreed on all points ! I particularly hate the single-line Fixes bug 1234566-type commit messages. Is there a way a concise version of this advice could find its way into HACKING.rst ? And/Or into http://wiki.openstack.org/ReviewChecklist ? -- Thierry Carrez (ttx) Release Manager, OpenStack ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
On Thu, Jun 28, 2012 at 12:01:10PM +0200, Thierry Carrez wrote: Daniel P. Berrange wrote: [...] In other words, when reviewing a change in Gerrit, do not simply look at the correctness of the code. Review the commit message itself and request improvements to its content. Look out for commits which are mixing multiple logical changes and require the submitter to split them into separate commits. Ensure whitespace changes are not mixed in with functional changes. Ensure no-op code refactoring is done separately from functional changes. And so on. [...] Nice work, and agreed on all points ! I particularly hate the single-line Fixes bug 1234566-type commit messages. Is there a way a concise version of this advice could find its way into HACKING.rst ? And/Or into http://wiki.openstack.org/ReviewChecklist ? Sure, MarkMc suggested to me that I put this doc up on the wiki somewhere. I'll do that and then submit a concise version for HACKING.rst and the ReviewChecklist page, with a cross-reference to the full thing. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
First off, I wanted to say I think these are a great set of recommendations. On Wed, Jun 27, 2012, Daniel P. Berrange berra...@redhat.com wrote: Fixes: bug #1003373 Implements: blueprint libvirt-xml-cpu-model Change-Id: I4946a16d27f712ae2adf8441ce78e6c0bb0bb657 Signed-off-by: Daniel P. Berrange berra...@redhat.com As well as the 'Signed-off-by' tag, there are various other ad-hoc tags that can be used to credit other people involved in a patch who aren't the author. What is the Signed-off-by tag used for? Your examples have yourself, but isn't that kind of implied by submitting the patch for review in the first place? JE ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
Can we set a location to the Authoritative HACKING.rst? There are fundamental and conflicting differences between the HACKING.rst in some of the projects. -Matt On Thu, Jun 28, 2012 at 3:15 AM, Daniel P. Berrange berra...@redhat.comwrote: On Thu, Jun 28, 2012 at 12:01:10PM +0200, Thierry Carrez wrote: Daniel P. Berrange wrote: [...] In other words, when reviewing a change in Gerrit, do not simply look at the correctness of the code. Review the commit message itself and request improvements to its content. Look out for commits which are mixing multiple logical changes and require the submitter to split them into separate commits. Ensure whitespace changes are not mixed in with functional changes. Ensure no-op code refactoring is done separately from functional changes. And so on. [...] Nice work, and agreed on all points ! I particularly hate the single-line Fixes bug 1234566-type commit messages. Is there a way a concise version of this advice could find its way into HACKING.rst ? And/Or into http://wiki.openstack.org/ReviewChecklist ? Sure, MarkMc suggested to me that I put this doc up on the wiki somewhere. I'll do that and then submit a concise version for HACKING.rst and the ReviewChecklist page, with a cross-reference to the full thing. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/:| |: http://libvirt.org -o- http://virt-manager.org:| |: http://autobuild.org -o- http://search.cpan.org/~danberr/:| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc:| ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
On Thu, Jun 28, 2012 at 09:21:20AM -0700, Johannes Erdfelt wrote: First off, I wanted to say I think these are a great set of recommendations. On Wed, Jun 27, 2012, Daniel P. Berrange berra...@redhat.com wrote: Fixes: bug #1003373 Implements: blueprint libvirt-xml-cpu-model Change-Id: I4946a16d27f712ae2adf8441ce78e6c0bb0bb657 Signed-off-by: Daniel P. Berrange berra...@redhat.com As well as the 'Signed-off-by' tag, there are various other ad-hoc tags that can be used to credit other people involved in a patch who aren't the author. What is the Signed-off-by tag used for? Your examples have yourself, but isn't that kind of implied by submitting the patch for review in the first place? Yes, you are technically correct in this respect. It is an idiom originating from the Linux Kernel community, which is auto-added by GIT if you pass the '-s' arg to 'git commit'. Basically it is a statement that you have read are complying with the projects contributor guidelines (eg Developer's Certificate of Origin[1] in the kernel), or a more formal contributor license agreement such as that used by OpenStack. OpenStack obviously has a formal CLA which all contributors *must* agree with prior to submitting patches, which serves the same purpose. Thus using a Signed-off-by: line is pretty much redundant for any contributions to OpenStack, since you must have signed the OpenStack CLA before you can even get access to post patches to Gerrit. The only case where I see that it might be considered relevant, is if the person submitting the patch to OpenStack, is not the same as the person who wrote the patch. For example, if someone in Red Hat's QA team (who isn't an OpenStack contributor) writes a patch for OpenStack gives it to me, then they'd typically include their own email addr in a 'Signed-off-by' tag to indicate that they are the author they understand the contribution requirements of OpenStack. This indicates to me that I can trust their patch thus I'd be happy to add my own email 'signed-off-by' line submit it to OpenStack in my role as someone who has agreed to the formal CLA. Since this tagging is a standard feature of GIT, it is quite typical for people to add Signed-off-by tags on all their commits, to any project, regardless of whether the project actually mandates this as their submission policy. I certainly just do it out of habit for all projects I contribute to. So in summary, you are perfectly ok to just ignore the whole Signed-off-by concept in OpenStack, given the formal CLA reqired for contributors already. Regards, Daniel [1] https://www.kernel.org/doc/Documentation/SubmittingPatches -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
I have to agree with Daniel on this one. I frequently work on issues where multiple people contribute to the code in a patch and therefore you wind up with multiple Signed-off-by's. Yes, it's a little redundant when it's exclusively your code but you need these tags when there are multiple contributors and if you don't provide the tag then you don't know if the code came exclusively from the submitter or if the Signed-off tag was left off by mistake. -- Don Dugger Censeo Toto nos in Kansa esse decisse. - D. Gale Ph: 303/443-3786 -Original Message- From: openstack-bounces+donald.d.dugger=intel@lists.launchpad.net [mailto:openstack-bounces+donald.d.dugger=intel@lists.launchpad.net] On Behalf Of Johannes Erdfelt Sent: Thursday, June 28, 2012 10:21 AM To: openstack@lists.launchpad.net Subject: Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history First off, I wanted to say I think these are a great set of recommendations. On Wed, Jun 27, 2012, Daniel P. Berrange berra...@redhat.com wrote: Fixes: bug #1003373 Implements: blueprint libvirt-xml-cpu-model Change-Id: I4946a16d27f712ae2adf8441ce78e6c0bb0bb657 Signed-off-by: Daniel P. Berrange berra...@redhat.com As well as the 'Signed-off-by' tag, there are various other ad-hoc tags that can be used to credit other people involved in a patch who aren't the author. What is the Signed-off-by tag used for? Your examples have yourself, but isn't that kind of implied by submitting the patch for review in the first place? JE ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
On Thu, 2012-06-28 at 09:24 -0700, Matt Joyce wrote: Can we set a location to the Authoritative HACKING.rst? There are fundamental and conflicting differences between the HACKING.rst in some of the projects. The HACKING.rst in each project is authoritative for that project. There are slight stylistic differences between the different project, and there is resistance to adopting an openstack-wide HACKING style guide. -- Kevin L. Mitchell kevin.mitch...@rackspace.com ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
On Thu, Jun 28, 2012, Daniel P. Berrange berra...@redhat.com wrote: On Thu, Jun 28, 2012 at 09:21:20AM -0700, Johannes Erdfelt wrote: What is the Signed-off-by tag used for? Your examples have yourself, but isn't that kind of implied by submitting the patch for review in the first place? [snip] The only case where I see that it might be considered relevant, is if the person submitting the patch to OpenStack, is not the same as the person who wrote the patch. For example, if someone in Red Hat's QA team (who isn't an OpenStack contributor) writes a patch for OpenStack gives it to me, then they'd typically include their own email addr in a 'Signed-off-by' tag to indicate that they are the author they understand the contribution requirements of OpenStack. This indicates to me that I can trust their patch thus I'd be happy to add my own email 'signed-off-by' line submit it to OpenStack in my role as someone who has agreed to the formal CLA. Since this tagging is a standard feature of GIT, it is quite typical for people to add Signed-off-by tags on all their commits, to any project, regardless of whether the project actually mandates this as their submission policy. I certainly just do it out of habit for all projects I contribute to. So in summary, you are perfectly ok to just ignore the whole Signed-off-by concept in OpenStack, given the formal CLA reqired for contributors already. That sounds reasonable. Thanks for the explanation. JE ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
I particularly hate the single-line Fixes bug 1234566-type commit messages. I assume your concern was regarding commits where Fixes bug 1234566 is the first and ONLY line. Fixes bug 1234566 comes from Wiki. Plus there is restriction on how long the first line of the commit message can be. Not everyone is able to describe their change in one short sentence. So typically *I* put Fixes bug 1234567 on the *first* line followed by additional lines describing the change. http://wiki.openstack.org/GerritWorkflow#Committing_Changes should be updated when this discussion is concluded. -Mandar __ Disclaimer:This email and any attachments are sent in strictest confidence for the sole use of the addressee and may contain legally privileged, confidential, and proprietary data. If you are not the intended recipient, please advise the sender by replying promptly to this email and then delete and destroy this email and any attachments without any further use, copying or forwarding ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
[Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
The following document presented for discussion is based on my experiance over the past few months getting involved with OpenStack Nova through learning the codebase, examining its history, writing code and participating in reviews. I want to stress, that I don't intend this is a criticism of any individuals. My desire is that this be taken as constructive feedback to help the project as a whole over the long term, since I believe OpenStack can benefit from stricter practices commonly followed in other mainstream opensource projects using GIT. Enjoy the rather long doc that follows GIT Commit Good Practice The following document is based on experiance doing code development, bug troubleshooting and code review across a number of projects using GIT, including libvirt, QEMU and OpenStack Nova. Examination of other open source projects such as the Kernel, CoreUtils, GNULIB and more suggested they all follow a fairly common practice. It is motivated by a desire to improve the quality of the Nova GIT history. Quality is a hard term to define in computing; One man's Thing of Beauty is another's man's Evil Hack. We can, however, come up with some general guidelines for what todo, or conversely what not todo, when publishing GIT commits for merge with a project, in this case, OpenStack. This topic can be split into two areas of concern 1. The structured set/split of the code changes 2. The information provided in the commit message Executive Summary = The points and examples that will be raised in this document ought clearly demonstrate the value in splitting up changes into a sequence of individual commits, and the importance in writing good commit messages to go along with them. If these guidelines were widely applied it would result in a significant improvement in the quality of the OpenStack GIT history. Both a carrot stick will be required to effect changes. This document intends to be the carrot by alerting people to the benefits, while anyone doing Gerrit code review can act as the stick ;-P In other words, when reviewing a change in Gerrit, do not simply look at the correctness of the code. Review the commit message itself and request improvements to its content. Look out for commits which are mixing multiple logical changes and require the submitter to split them into separate commits. Ensure whitespace changes are not mixed in with functional changes. Ensure no-op code refactoring is done separately from functional changes. And so on. It might be mentioned that Gerrit's handling of patch series is not entirely perfect. This is a not a valid reason to avoid creating patch series. The tools being used should be subservient to developers needs, and since they are open source they can be fixed / improved. Software source code is read mostly, write occassionally and thus the most important criteria is the improve the long term maintainability by the large pool of developers in the community, and not to sacrifice too much for the sake of the single author who may never touch the code again. And now the long detailed guidelines examples of good bad practice Structural split of changes === The cardinal rule for creating good commits is to ensure there is only one logical change per commit. There are many reasons why this is an important rule: * The smaller the amount of code being changed, the quicker easier it to review identify potential flaws. * If a change is found to be flawed later, it make be neccessary to revert the broken commit. This is much easier todo if there are not other unrelated code changes entangled with the original commit. * When troubleshooting problems using GIT's bisect capability, small well defined changes will aid in isolating exactly where the code problem was introduced. * When browsing history using GIT annotate/blame, small well defined changes also aid in isolating exactly where why a piece of code came from. Things to avoid when creating commits - With that in mind, there are some commonly encountered examples of bad things to avoid * Mixing whitespace changes with functional code changes The whitespace changes will obscure the important functional changes, making it harder for a reviewer to correctly determine whether the change is correct. Solution: Create 2 commits, one with the whitespace changes, one with the functional changes. Typically the whitespace change would be done first, but that need not be a hard rule. * Mixing two unrelated functional changes Again the reviewer will find it harder to identify flaws if two unrelated changes are mixed together. If it becomes neccessary to later revert a broken commit the two unrelated changes will need to be untangled, with further risk of bug creation. * Sending large new features in a
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
Hi, As a recent contributor to OpenStack, but with experience in other projects, I think moving in the directions you document would be good. And as you wrote, it's common practice in many many projects, which is another argument for this :-) However, one comment: Le mercredi 27 juin 2012, à 11:52 +0100, Daniel P. Berrange a écrit : It might be mentioned that Gerrit's handling of patch series is not entirely perfect. This is a not a valid reason to avoid creating patch series. It'd be really great if we could first improve Gerrit to handle the patch series workflow in a better way. Without such a change, pushing patch series to Gerrit is really no fun for anyone :/ I've no idea if this is currently being worked on (at least, I don't really se an issue reported in Gerrit's issue tracker). Maybe we should sit down and at least document how we'd like to improve this specific workflow? Cheers, Vincent -- Les gens heureux ne sont pas pressés. ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
On Wed, Jun 27, 2012 at 03:24:21PM +0200, Vincent Untz wrote: Hi, As a recent contributor to OpenStack, but with experience in other projects, I think moving in the directions you document would be good. And as you wrote, it's common practice in many many projects, which is another argument for this :-) However, one comment: Le mercredi 27 juin 2012, à 11:52 +0100, Daniel P. Berrange a écrit : It might be mentioned that Gerrit's handling of patch series is not entirely perfect. This is a not a valid reason to avoid creating patch series. It'd be really great if we could first improve Gerrit to handle the patch series workflow in a better way. Without such a change, pushing patch series to Gerrit is really no fun for anyone :/ I've no idea if this is currently being worked on (at least, I don't really se an issue reported in Gerrit's issue tracker). Maybe we should sit down and at least document how we'd like to improve this specific workflow? Yep, no argument that Gerrit could do with some improvements, but having submitted a number of non-trivial patch series to Nova, I don't think current Gerrit UI is a complete blocker to adoption. It is not ideal, but it isn't too painful if you're aware of what to look for. I think the main problem is that since the patch dependancies are not obvious in the UI, reviewers tend to miss the fact that they're reviewing a patch that's part of a series. I submitted one bug against Gerrit already to improve the way it deals with bug resolution wrt patch series https://bugs.launchpad.net/openstack-ci/+bug/1018013 One think people might not be aware of is that if you create a patch series on a branch and push that branch using 'git review', then the branch name becomes the Gerrit topic which provides an easy way to see the entire series. Alternatively the blueprint or bug IDs might be chosen to form the topic. For example my most recent series: https://review.openstack.org/#/q/status:open+project:openstack/nova+branch:master+topic:bug/1003373,n,z I think the main patch display UI though needs to make it much more obvious that a particular patch is part of a series so that reviewers know to review the work as a whole. The UI does display dependencies. eg see the depends on and needed by links: https://review.openstack.org/#/c/8694/ but I'd suggest that UI be changed so that instead of showing only the previous and next, it displays all patches in the series at once. Gerrit could also be less stupid about repeatedly trying failing to merge a patch due to missing dependent patches. In fact I'll file a bug about that flaw now too. Regards, Daniel -- |: http://berrange.com -o-http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :| ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
Re: [Openstack] RFC: Thoughts on improving OpenStack GIT commit practice/history
On Jun 27, 2012, at 3:52 AM, Daniel P. Berrange wrote: The following document presented for discussion is based on my experiance over the past few months getting involved with OpenStack Nova through learning the codebase, examining its history, writing code and participating in reviews. I want to stress, that I don't intend this is a criticism of any individuals. My desire is that this be taken as constructive feedback to help the project as a whole over the long term, since I believe OpenStack can benefit from stricter practices commonly followed in other mainstream opensource projects using GIT. snip Thank you for your detailed instructions and examples. I think we could definitely could be more careful about keeping our history clean. My general thoughts about commit messages has been that I want them to to be concise and to the point and the details are in the code itself. If more information is needed then there are bug reports and review discussions that are available. Your points about offline browsing are well taken however, so I will start including more details in my commit messages about the original problem and rationale for the solution. I recommend everyone follow suit. Vish ___ Mailing list: https://launchpad.net/~openstack Post to : openstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~openstack More help : https://help.launchpad.net/ListHelp
