[Twisted-Python] Re: [Twisted-commits] r17325 - merge sob-491, fixes #491
On Wed, 21 Jun 2006 11:23:28 -0400, Itamar Shtull-Trauring <itamar@itamarst.org> wrote:
To clarify, all commits to trunk should take on this form: <line of 78 or fewer characters summarizing change> Author: <names of people who wrote the code> Reviewer: <names of people who reviewed the branch> Fixes <whatever tickets are being fixed> [Re <whatever other tickets are related>] <longer description of what changes are taking place> Jean-Paul
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Oh, for pete's sake. I'm all for a structured development process, but this is just being pedantic for the sake of being pedantic. Please tell me, how does following this rigid log message structure help speed twisted development? How does it contribute to the overall quality of Twisted in any substantive way?
<line of 78 or fewer characters summarizing change>
Be serious. The haranguing about this kind of stuff is getting so tiresome that I've stopped making any serious attempts to commit to the trunk. As long as the necessary information is in there, who the heck cares how it's formatted? I see that glyph looked at the bug and pronounced it acceptable. It was possible to look this up in the bug, which is how I found out, spending all of 10 seconds on it. C Jean-Paul Calderone wrote:
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFEmXQK3A5SrXAiHQcRAjjmAJ96P0FRjVLFiTpE5uWviwx3jrbcMACdE1zB RJhcljV+ICD9UDvYVuEbNNw= =M3HX -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 If so, this is the first time I've heard about it. And what are we doing about the masses of commit messages that aren't? C Mike Pelletier wrote:
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFEmXe43A5SrXAiHQcRAgH7AJ4lzv5V8IsKIxuXsKXF+9O/QUXKogCgg563 9oogCVAHg6fC154dTQ4DL44= =cpKI -----END PGP SIGNATURE-----
On Wed, 21 Jun 2006 12:41:19 -0400, Mike Pelletier <mike@mkp.ca> wrote:
We're posting messages on the mailing list reminding people of the appropriate format, and of other rules related to the development process. Of course, some people have raised objections to this practice, but I think overall it is a net win that people are finding out.
Excuse me, just realised I was thinking of the plone SVN, which scrapes "fixes #xxx" an so on.
This is a trac feature, and Twisted is using it too. That's the reason for the required "Fixes #xxx" line. For an example, see the ticket in question: http://twistedmatrix.com/trac/ticket/491 Despite lacking the other required bits of the commit message, the merge did include the ticket number.
On 6/21/06, Cory Dodt <corydodt@twistedmatrix.com> wrote:
While I don't care so much how it's 78 characters, I *do* care about all the missing information in that commit message, given that I'm one of the guys who has to read through commit logs while building a NEWS file for the release. This is a good policy and everyone's been following it beautifully for a while now. -- Christopher Armstrong International Man of Twistery http://radix.twistedmatrix.com/ http://twistedmatrix.com/ http://canonical.com/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 I don't object to asking for the information to be there. I don't object to emailing people to ask them not to leave out important stuff. I don't even *necessarily* object to pedantic formatting, although I think it's far more important that the information be there, not that it be typed in RFC 822*. I do object to aspects of how this is handled publicly. For example, why does the only document I can find on the subject: <http://twistedmatrix.com/trac/wiki/BranchPolicy> not mention a rigid format for commit messages? In fact, it doesn't mention the commit messages at all. In fact, the document is labelled "some guy's opinion", although it is in facted titled "Policy", which is a pretty mixed message. This page <http://twistedmatrix.com/trac/wiki/TwistedDevelopment> doesn't mention it either. (It also doesn't mention the coding standard, documentation standard, etc. but that's the subject of another rant.) If you're going to have an official policy, and you're going to publicly chastise people on the mailing list, you'd better make sure the whole world knows what the rules are before you do so. I do not consider "Re: [Twisted-commits] r17325 ..." to be the title of an official policy document. And here's where I'm coming from. We are creating a higher standard for committers, I understand that. We believe we are getting higher quality code and better turnaround for releases out of it, I understand that. But the approach being taken is to just blast people for not doing it right; bad enough on its own, doubly bad for not having warned them first. This is discouraging to those of us who are occasional committers, and it's even more discouraging to people who love to work in Twisted and hope to get the "commit" level of trust someday themselves. I think the correct approach is to discuss the policy on IRC, then to waste NO TIME posting the contents of the discussion to the Wiki, then to announce the wiki page on the mailing list. If anyone objects, and probably very few will, you might have to revise the policy a bit. *then* you can ding people, but at least try to work with them in private first. C *(I did know about the trac integration thing, but I consider that to be almost useless as it's so easy to add this information to the bug manually.) Christopher Armstrong wrote:
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFEmYwt3A5SrXAiHQcRAip5AJkB1376KCVWbdyx+ME0JuXqAEPvnQCgouvR e5N7nCVs6axTM97WR09b4gs= =s31I -----END PGP SIGNATURE-----
On 6/21/06, Cory Dodt <corydodt@twistedmatrix.com> wrote:
"chastise"? You're overreacting, Cory. Nobody has been chastised in this thread apart from those you chastised for pointing out the commit format. While you're definitely right about not having much public documentation about it (please contribute, you are a wiki editor), the main point of your messages seem to be based on the mistaken assumption that someone is being *abused*. -- Christopher Armstrong International Man of Twistery http://radix.twistedmatrix.com/ http://twistedmatrix.com/ http://canonical.com/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Christopher Armstrong wrote:
Usually I object to this kind of policy change; documents of policy should be written by a person who holds the majority opinion. In this case, and in others, I was not around to discuss it when it was decided on. (I suspect this was a Divmod policy that has migrated to Twisted; while this is not always bad, Divmod is not Twisted, and the communication avenues are not the same, so I don't assume the same rules apply to both.) C -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFEmZH43A5SrXAiHQcRAgb4AJ0QYoqfAQyS5SyBULJU63+QeTKtiwCgmplI 0JIeQHwUDXavOeQQXV0udHc= =Psyl -----END PGP SIGNATURE-----
On Wed, 21 Jun 2006 11:13:01 -0700, Cory Dodt <corydodt@twistedmatrix.com> wrote:
I do disagree pretty strongly about this particular case being a "chastisement", but I agree with the general problem, so that's not a productive avenue of discussion. :) The policy is documented, but in fragments, counting just now across the documents *I* happen to know about, spread across 5 different documents on 3 servers and in at least 4 different document-management systems. The UQDS page, which is probably the best source of information, is only linked (indirectly) at the bottom of the BBD page, which isn't linked from the front page. Clearly this is a serious issue. Since we have a place for issues, I have created one: http://twistedmatrix.com/trac/ticket/1840 I've assigned the ticket for you, but I feel I should explain why - it's not because I think that, because you have mentioned a problem, you are now responsible for fixing it. At the very least, you can't do this on your own, I recognize that to really fix the problem you will need to get participation from the people making (me) and enforcing (jp, apparently) the policies. I've assigned it to you because you obviously care a good deal about this issue and are more likely than anyone else to agitate for it to get done right now. I've personally known about this for a while, of course, but I am fairly interrupt-driven on Twisted stuff these days; I'm overtaxed and I can't muster the time and energy to fix an issue until it (for example) becomes an interesting thread on the mailing list that I have to deal with as I'm going through my inbox :). I do check my trac tickets with some frequency though, and I'm always trying to resolve simple stuff. If you can think of simple tasks that #1840 depends on please feel free to assign tickets to me.
I'm sorry you weren't available but we did bring it up on IRC numerous times, it was mentioned in a few threads on this list before indirectly, and there was an "official" thread on the mailing list for a while dedicated to this to avoid excluding non-IRC contributors. I think this particular exclusion may have had more to do with your having less free time available when we were talking about it. I am, however, sensitive to the fact that it is damn hard to find maintainers for something like Twisted's windows installer (especially those that are interested in adhering to the rather high standards for quality we have been requiring), so we need to communicate requirements and such more clearly and avoid pissing you off any more than necessary. ;-) I suspect that Twisted.Quotes will be getting more frequent updates once all of our IRC bandwidth is no longer taken up answering undocumented FAQs about unit tests, coding style and commit messages.
On Wed, 21 Jun 2006 09:30:02 -0700, Cory Dodt <corydodt@twistedmatrix.com> wrote:
I'm all for a structured development process, but
I love the way that everyone who objects to a structured development process begins their criticism with "I love a structured development process, but".
this is just being pedantic for the sake of being pedantic.
Nope, there are good reasons for all of it. The commit message was incorrectly formatted. Nobody reverted the commit or anything, and I certainly didn't interpret JP's informative post to be "haranguing" anyone about the process; merely reminding. "Jerub is such a jackass, he didn't follow the required process for commits, I am reverting everything he's done in the last 3 months" -- now that would be a harangue. (BTW jerub, you are a jackass: it's rude to the release manager to include helpful information in the commit, and I am already so rude to him that he's going to start killing people if he doesn't get some respect around here :)) Even the 78-characters thing has a purpose. The commit messages on the IRC channel and commits list are generated from the message. Some mailer software chokes on >78-character subject lines, and the IRC message will be truncated if it's longer than that, and it will be omitted entirely if the message doesn't appear on the first line. The format for commit messages, among other things, is documented here: http://divmod.org/trac/wiki/UltimateQualityDevelopmentSystem I've written a bit more about the philosophical underpinnings of the constant striving to improve our development process through simple procedures like this one here: http://glyf.livejournal.com/58626.html
Be serious.
We are serious. What's with the tone of your message?
It was possible to look this up in the bug, which is how I found out, spending all of 10 seconds on it.
It wouldn't have been if he forgot the "fixes" field instead of some other random bits of information from the message. That field is also required for a reason. Even if it is literally ten seconds (and I doubt that, it's probably more like 40 seconds, once you've mixed in trac latency) multiply that ten seconds by every change made to trunk in six months, add a few minutes for each relevant message, and that's how long it takes to write the NEWS file. There's no (reasonable) way to correct commit messages, so it's important to write them correctly the first time.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 I'll start by apologizing for the tone of my message, and I am aware that I was being chastising. JP is just doing his job. JP, I have the utmost respect for you and what you do for Twisted, which is far more than I have space to enumerate in this email. There was a reason for my tone. This has been accumulating for a while, with lots of these little dings for this or that on this public mailing list. It gets exasperating. People read this list, and they get a feeling for the attitude of the developers on it when they do. I don't think "pedantic twits" is the feeling we want to project, do you? This is no single person's fault, it just felt like the right time to say, "Step back. Think about the direction this is going. We don't want to go there." It changes the tone of a project when Twisted.Quotes stops being updated but the list of policy updates keeps growing and growing and growing without warning. My last email contains suggestions of some better ways to handle this. C glyph@divmod.com wrote:
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFEmZDU3A5SrXAiHQcRAgE3AKCH0EB2aYpjnoGr0AguEK5pq0hq2wCeO8Ed o5tSAmEt9ePtPDsJuX1JMaw= =20FS -----END PGP SIGNATURE-----
On Wed, 21 Jun 2006 13:32:52 -0500, Cory Dodt <corydodt@twistedmatrix.com> wrote:
Hear hear. I don't think anyone really interpreted you as denying JP his deserved respect, but it never hurts to remind people that you respect them. [snip]
Well, to me, when a project moves away from frequent updates to the very amusing Quotes file, and towards policies and procedures, it means that the project is maturing and recognizing that a broader audience requires such steps. The ecosystem may no longer be as fun for many of the people who were in at the beginning, but I consider this a change for the better, whereas you see it as a change for the worse. Different strokes, I guess.
My last email contains suggestions of some better ways to handle this.
Those suggestions look fine to me, as a person who files a ticket now and again, and once in a great while posts a patch. That said, I also feel that as a very minimal contributor, it doesn't behoove me to take offense when overworked full-time contributors are terse in their communications. L. Daniel Burr
On Wed, 21 Jun 2006 12:15:03 -0400, Jean-Paul Calderone <exarkun@divmod.com>
Thanks for that, my commits to trunk will take this form from now on. I had managed to avoid finding and reading http://divmod.org/trac/wiki/UltimateQualityDevelopmentSystem - I've read found it now, and hopefully I will make fewer process errors. Stephen.
On 6/22/06, Stephen Thorne <stephen@thorne.id.au> wrote:
I had managed to avoid finding and reading http://divmod.org/trac/wiki/UltimateQualityDevelopmentSystem
Assisted, no doubt, by the fact that there is no mention of it on the Twisted website. Although see #1222, #1697. jml
On Thu, 22 Jun 2006 10:21:17 +1000, Jonathan Lange <jml@mumak.net> wrote:
There's no mention, but it is linked. http://twistedmatrix.com/trac/wiki/TwistedDevelopment links http://divmod.org/trac/wiki/BranchBasedDevelopment links http://divmod.org/trac/wiki/UltimateQualityDevelopmentSystem Stephen.
On Thu, 22 Jun 2006 12:31:59 +1000, Andrew Bennetts <andrew-twisted@puzzling.org> wrote:
I've received this suggestion a few times privately and I'm still mulling it over. On the face of it, it's a good idea, buuuuuut: 1. I've had a series of disastrous experiences with commit hooks rejecting changes. The guy who wrote the commit hook goes on vacation for 3 days, there's a bug in it, and nobody can figure out how to merge code or turn it off for 8 hours; meanwhile the guy trying to commit in bangalore goes to sleep, then doesn't show up again for a week, and the changes are lost. (Having changes on a branch prior to trunk would alleviate this somewhat, I think, but it would be just as much of a pain to be blocked on some stupid merge-script bug.) 2. The most important thing, really, is a sufficiently descriptive explanation for the change. Any programmatic attempt to enforce this is sure to drive artificial gaming of the metric; i.e. if we require 3 sentences, "fixed random junk in foo module" will become "Fixed random. Junk in. Foo module." 3. 99% of the time (e.g. all "normal" commits) should be in this format; however, the underlying tools, to be honest, are still kind of primitive, and we still have to leave a little wiggle room to deal with their foibles. At least it isn't the masochistic chewing-gum-and-duct-tape perpetual catastrophe of CVS, but SVN still does annoying things like screwing up merges. If there is a commit with a nice long description and a 'fixes' and everything, which needs to be reverted and reapplied for some dumb technical reason, I'd like to see r1234: "author: foo; reviewer: bar; wonderful wonderful description ..." r1235: "crap, reverting r1234 because of svn bug #9813741" r1236: "reapplying r1234 merged in utterly retarded way to work around svn bug #9813741" not the entire description of r1234 repeated twice -- or, more realistically, three times, since the revert in the middle would also have to have a conformant commit message. 4. precommit hooks keep the SVN SSH transport alive to report errors to the client for the duration of the hook. When I last investigated (although this was in an SVN pre-release, a good 2+ years ago) if the connection drops while you're validating the commit message, it rejects the commit. This would be especially annoying if you were committing an important fix over some intermittent link, like a GPRS modem. That's not a common occurrance, but then again, if something were important enough that someone needed to commit code on a *GPRS modem* that is probably the time they would _least_ like something technical to go wrong with the process. This is basically just the same reason that, despite our hardcore attitude towards tests, we don't have buildbot automatically reject or revert commits based on automated test failures. The tools (in that case, buildbot, trial, and our test suite) are too primitive to rely on without some level of human judgement. We've been moving to a stricter model so that the human judgements can be as mechanical and apolitical as possible (e.g. "was this a test failure due to too much load on the buildbot / known intermittent failure", not "is this an important enough area of code to worry about test failures in") but we still need it there. I am eager to be corrected however; the more work our tools can reliably do for us the better. Has anybody had a good experience with automatically-rejected commits in another reasonably sized project before?
On 6/22/06, glyph@divmod.com <glyph@divmod.com> wrote:
This is basically just the same reason that, despite our hardcore attitude towards tests, we don't haThe tools (in that case, buildbot, trial, and our test suite) are too primitive to rely on without some level of human judgement.
Yeah?! Well your face is primitive! :) jml
On Wed, Jun 21, 2006 at 11:00:05PM -0400, glyph@divmod.com wrote: [...]
Two things make this a non-issue, hopefully: - a script that checks that a commit message matches a regex is pretty damn simple. - in case of emergency, it shouldn't be hard to just disable the offending commit-hook until such time as it can be fixed properly.
Well, I was really only suggesting that the presence of mandatory metadata is checked for, not that we write an AI to check that the text of the messages make sense :)
Good point. So there either needs to be a way to explicitly flag that this message is ok despite not being in the usual format, or we need to anticipate all possible forms of exceptional commits. Requiring e.g. "[override=reason]" in the text of the commit message somewhere could do this. This of course complicates the checker slightly, which relates to point 1, but not impossibly so.
The time it takes to validate a commit message against a regex should be negligible. I doubt this will be a real problem.
There are important differences between a commit message checker and our test suite. Our test suite gives way too many false positives, and does so unpredictably. Our test suite takes many minutes to run, at best. Our test suite should be run on multiple platforms for full effectiveness. Our test suite is extremely complex. A simple "does the commit message match this regex" check has none of these problems, assuming the regex isn't insane.
Yes, the Launchpad development process automatically rejects commits, both for malformed commit messages[1] and for test failures. It's generally worked quite well. The test suite takes similar lengths of time to Twisted's to run (but without the portability concerns, thankfully), but we submit requests to merge to trunk asynchronously (via gpg-signed email), so this isn't a killer for poor connections. We have occasionally had problems where a system upgrade or other supposedly unrelated change causes the test suite on trunk to start failing, effectively blocking all merges, requiring admin intervention. Old processes from previous test runs can also be an issue. It's generally worked very well for us, though. And it means we are forced to have zero tolerance of failing tests. Developers get unhappy if there's an intermittently failing test that's blocking their merges :) We use PQM ("Patch Queue Manager", bzr branch available here: http://people.ubuntu.com/~robertc/pqm/trunk/, bugs etc here: https://launchpad.net/products/pqm) to do this. -Andrew. [1] We require either "[trivial]", "[r=reviewer]", or "[rs=approver]" to be in the message. "r=" means "reviewed and approved by" and "rs=" means "rubber stamped by", i.e. not closely reviewed by the person (generally a manager) approving it anyway is happy with it being merged despite that and is responsible for the result. "trivial" should be obvious ;). Regardless of commit message, the full test suite is always run.
On Wed, 21 Jun 2006 11:23:28 -0400, Itamar Shtull-Trauring <itamar@itamarst.org> wrote:
To clarify, all commits to trunk should take on this form: <line of 78 or fewer characters summarizing change> Author: <names of people who wrote the code> Reviewer: <names of people who reviewed the branch> Fixes <whatever tickets are being fixed> [Re <whatever other tickets are related>] <longer description of what changes are taking place> Jean-Paul
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Oh, for pete's sake. I'm all for a structured development process, but this is just being pedantic for the sake of being pedantic. Please tell me, how does following this rigid log message structure help speed twisted development? How does it contribute to the overall quality of Twisted in any substantive way?
<line of 78 or fewer characters summarizing change>
Be serious. The haranguing about this kind of stuff is getting so tiresome that I've stopped making any serious attempts to commit to the trunk. As long as the necessary information is in there, who the heck cares how it's formatted? I see that glyph looked at the bug and pronounced it acceptable. It was possible to look this up in the bug, which is how I found out, spending all of 10 seconds on it. C Jean-Paul Calderone wrote:
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFEmXQK3A5SrXAiHQcRAjjmAJ96P0FRjVLFiTpE5uWviwx3jrbcMACdE1zB RJhcljV+ICD9UDvYVuEbNNw= =M3HX -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 If so, this is the first time I've heard about it. And what are we doing about the masses of commit messages that aren't? C Mike Pelletier wrote:
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFEmXe43A5SrXAiHQcRAgH7AJ4lzv5V8IsKIxuXsKXF+9O/QUXKogCgg563 9oogCVAHg6fC154dTQ4DL44= =cpKI -----END PGP SIGNATURE-----
On Wed, 21 Jun 2006 12:41:19 -0400, Mike Pelletier <mike@mkp.ca> wrote:
We're posting messages on the mailing list reminding people of the appropriate format, and of other rules related to the development process. Of course, some people have raised objections to this practice, but I think overall it is a net win that people are finding out.
Excuse me, just realised I was thinking of the plone SVN, which scrapes "fixes #xxx" an so on.
This is a trac feature, and Twisted is using it too. That's the reason for the required "Fixes #xxx" line. For an example, see the ticket in question: http://twistedmatrix.com/trac/ticket/491 Despite lacking the other required bits of the commit message, the merge did include the ticket number.
On 6/21/06, Cory Dodt <corydodt@twistedmatrix.com> wrote:
While I don't care so much how it's 78 characters, I *do* care about all the missing information in that commit message, given that I'm one of the guys who has to read through commit logs while building a NEWS file for the release. This is a good policy and everyone's been following it beautifully for a while now. -- Christopher Armstrong International Man of Twistery http://radix.twistedmatrix.com/ http://twistedmatrix.com/ http://canonical.com/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 I don't object to asking for the information to be there. I don't object to emailing people to ask them not to leave out important stuff. I don't even *necessarily* object to pedantic formatting, although I think it's far more important that the information be there, not that it be typed in RFC 822*. I do object to aspects of how this is handled publicly. For example, why does the only document I can find on the subject: <http://twistedmatrix.com/trac/wiki/BranchPolicy> not mention a rigid format for commit messages? In fact, it doesn't mention the commit messages at all. In fact, the document is labelled "some guy's opinion", although it is in facted titled "Policy", which is a pretty mixed message. This page <http://twistedmatrix.com/trac/wiki/TwistedDevelopment> doesn't mention it either. (It also doesn't mention the coding standard, documentation standard, etc. but that's the subject of another rant.) If you're going to have an official policy, and you're going to publicly chastise people on the mailing list, you'd better make sure the whole world knows what the rules are before you do so. I do not consider "Re: [Twisted-commits] r17325 ..." to be the title of an official policy document. And here's where I'm coming from. We are creating a higher standard for committers, I understand that. We believe we are getting higher quality code and better turnaround for releases out of it, I understand that. But the approach being taken is to just blast people for not doing it right; bad enough on its own, doubly bad for not having warned them first. This is discouraging to those of us who are occasional committers, and it's even more discouraging to people who love to work in Twisted and hope to get the "commit" level of trust someday themselves. I think the correct approach is to discuss the policy on IRC, then to waste NO TIME posting the contents of the discussion to the Wiki, then to announce the wiki page on the mailing list. If anyone objects, and probably very few will, you might have to revise the policy a bit. *then* you can ding people, but at least try to work with them in private first. C *(I did know about the trac integration thing, but I consider that to be almost useless as it's so easy to add this information to the bug manually.) Christopher Armstrong wrote:
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFEmYwt3A5SrXAiHQcRAip5AJkB1376KCVWbdyx+ME0JuXqAEPvnQCgouvR e5N7nCVs6axTM97WR09b4gs= =s31I -----END PGP SIGNATURE-----
On 6/21/06, Cory Dodt <corydodt@twistedmatrix.com> wrote:
"chastise"? You're overreacting, Cory. Nobody has been chastised in this thread apart from those you chastised for pointing out the commit format. While you're definitely right about not having much public documentation about it (please contribute, you are a wiki editor), the main point of your messages seem to be based on the mistaken assumption that someone is being *abused*. -- Christopher Armstrong International Man of Twistery http://radix.twistedmatrix.com/ http://twistedmatrix.com/ http://canonical.com/
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Christopher Armstrong wrote:
Usually I object to this kind of policy change; documents of policy should be written by a person who holds the majority opinion. In this case, and in others, I was not around to discuss it when it was decided on. (I suspect this was a Divmod policy that has migrated to Twisted; while this is not always bad, Divmod is not Twisted, and the communication avenues are not the same, so I don't assume the same rules apply to both.) C -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFEmZH43A5SrXAiHQcRAgb4AJ0QYoqfAQyS5SyBULJU63+QeTKtiwCgmplI 0JIeQHwUDXavOeQQXV0udHc= =Psyl -----END PGP SIGNATURE-----
On Wed, 21 Jun 2006 11:13:01 -0700, Cory Dodt <corydodt@twistedmatrix.com> wrote:
I do disagree pretty strongly about this particular case being a "chastisement", but I agree with the general problem, so that's not a productive avenue of discussion. :) The policy is documented, but in fragments, counting just now across the documents *I* happen to know about, spread across 5 different documents on 3 servers and in at least 4 different document-management systems. The UQDS page, which is probably the best source of information, is only linked (indirectly) at the bottom of the BBD page, which isn't linked from the front page. Clearly this is a serious issue. Since we have a place for issues, I have created one: http://twistedmatrix.com/trac/ticket/1840 I've assigned the ticket for you, but I feel I should explain why - it's not because I think that, because you have mentioned a problem, you are now responsible for fixing it. At the very least, you can't do this on your own, I recognize that to really fix the problem you will need to get participation from the people making (me) and enforcing (jp, apparently) the policies. I've assigned it to you because you obviously care a good deal about this issue and are more likely than anyone else to agitate for it to get done right now. I've personally known about this for a while, of course, but I am fairly interrupt-driven on Twisted stuff these days; I'm overtaxed and I can't muster the time and energy to fix an issue until it (for example) becomes an interesting thread on the mailing list that I have to deal with as I'm going through my inbox :). I do check my trac tickets with some frequency though, and I'm always trying to resolve simple stuff. If you can think of simple tasks that #1840 depends on please feel free to assign tickets to me.
I'm sorry you weren't available but we did bring it up on IRC numerous times, it was mentioned in a few threads on this list before indirectly, and there was an "official" thread on the mailing list for a while dedicated to this to avoid excluding non-IRC contributors. I think this particular exclusion may have had more to do with your having less free time available when we were talking about it. I am, however, sensitive to the fact that it is damn hard to find maintainers for something like Twisted's windows installer (especially those that are interested in adhering to the rather high standards for quality we have been requiring), so we need to communicate requirements and such more clearly and avoid pissing you off any more than necessary. ;-) I suspect that Twisted.Quotes will be getting more frequent updates once all of our IRC bandwidth is no longer taken up answering undocumented FAQs about unit tests, coding style and commit messages.
On Wed, 21 Jun 2006 09:30:02 -0700, Cory Dodt <corydodt@twistedmatrix.com> wrote:
I'm all for a structured development process, but
I love the way that everyone who objects to a structured development process begins their criticism with "I love a structured development process, but".
this is just being pedantic for the sake of being pedantic.
Nope, there are good reasons for all of it. The commit message was incorrectly formatted. Nobody reverted the commit or anything, and I certainly didn't interpret JP's informative post to be "haranguing" anyone about the process; merely reminding. "Jerub is such a jackass, he didn't follow the required process for commits, I am reverting everything he's done in the last 3 months" -- now that would be a harangue. (BTW jerub, you are a jackass: it's rude to the release manager to include helpful information in the commit, and I am already so rude to him that he's going to start killing people if he doesn't get some respect around here :)) Even the 78-characters thing has a purpose. The commit messages on the IRC channel and commits list are generated from the message. Some mailer software chokes on >78-character subject lines, and the IRC message will be truncated if it's longer than that, and it will be omitted entirely if the message doesn't appear on the first line. The format for commit messages, among other things, is documented here: http://divmod.org/trac/wiki/UltimateQualityDevelopmentSystem I've written a bit more about the philosophical underpinnings of the constant striving to improve our development process through simple procedures like this one here: http://glyf.livejournal.com/58626.html
Be serious.
We are serious. What's with the tone of your message?
It was possible to look this up in the bug, which is how I found out, spending all of 10 seconds on it.
It wouldn't have been if he forgot the "fixes" field instead of some other random bits of information from the message. That field is also required for a reason. Even if it is literally ten seconds (and I doubt that, it's probably more like 40 seconds, once you've mixed in trac latency) multiply that ten seconds by every change made to trunk in six months, add a few minutes for each relevant message, and that's how long it takes to write the NEWS file. There's no (reasonable) way to correct commit messages, so it's important to write them correctly the first time.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 I'll start by apologizing for the tone of my message, and I am aware that I was being chastising. JP is just doing his job. JP, I have the utmost respect for you and what you do for Twisted, which is far more than I have space to enumerate in this email. There was a reason for my tone. This has been accumulating for a while, with lots of these little dings for this or that on this public mailing list. It gets exasperating. People read this list, and they get a feeling for the attitude of the developers on it when they do. I don't think "pedantic twits" is the feeling we want to project, do you? This is no single person's fault, it just felt like the right time to say, "Step back. Think about the direction this is going. We don't want to go there." It changes the tone of a project when Twisted.Quotes stops being updated but the list of policy updates keeps growing and growing and growing without warning. My last email contains suggestions of some better ways to handle this. C glyph@divmod.com wrote:
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (MingW32) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org iD8DBQFEmZDU3A5SrXAiHQcRAgE3AKCH0EB2aYpjnoGr0AguEK5pq0hq2wCeO8Ed o5tSAmEt9ePtPDsJuX1JMaw= =20FS -----END PGP SIGNATURE-----
On Wed, 21 Jun 2006 13:32:52 -0500, Cory Dodt <corydodt@twistedmatrix.com> wrote:
Hear hear. I don't think anyone really interpreted you as denying JP his deserved respect, but it never hurts to remind people that you respect them. [snip]
Well, to me, when a project moves away from frequent updates to the very amusing Quotes file, and towards policies and procedures, it means that the project is maturing and recognizing that a broader audience requires such steps. The ecosystem may no longer be as fun for many of the people who were in at the beginning, but I consider this a change for the better, whereas you see it as a change for the worse. Different strokes, I guess.
My last email contains suggestions of some better ways to handle this.
Those suggestions look fine to me, as a person who files a ticket now and again, and once in a great while posts a patch. That said, I also feel that as a very minimal contributor, it doesn't behoove me to take offense when overworked full-time contributors are terse in their communications. L. Daniel Burr
On Wed, 21 Jun 2006 12:15:03 -0400, Jean-Paul Calderone <exarkun@divmod.com>
Thanks for that, my commits to trunk will take this form from now on. I had managed to avoid finding and reading http://divmod.org/trac/wiki/UltimateQualityDevelopmentSystem - I've read found it now, and hopefully I will make fewer process errors. Stephen.
On 6/22/06, Stephen Thorne <stephen@thorne.id.au> wrote:
I had managed to avoid finding and reading http://divmod.org/trac/wiki/UltimateQualityDevelopmentSystem
Assisted, no doubt, by the fact that there is no mention of it on the Twisted website. Although see #1222, #1697. jml
On Thu, 22 Jun 2006 10:21:17 +1000, Jonathan Lange <jml@mumak.net> wrote:
There's no mention, but it is linked. http://twistedmatrix.com/trac/wiki/TwistedDevelopment links http://divmod.org/trac/wiki/BranchBasedDevelopment links http://divmod.org/trac/wiki/UltimateQualityDevelopmentSystem Stephen.
On Thu, 22 Jun 2006 12:31:59 +1000, Andrew Bennetts <andrew-twisted@puzzling.org> wrote:
I've received this suggestion a few times privately and I'm still mulling it over. On the face of it, it's a good idea, buuuuuut: 1. I've had a series of disastrous experiences with commit hooks rejecting changes. The guy who wrote the commit hook goes on vacation for 3 days, there's a bug in it, and nobody can figure out how to merge code or turn it off for 8 hours; meanwhile the guy trying to commit in bangalore goes to sleep, then doesn't show up again for a week, and the changes are lost. (Having changes on a branch prior to trunk would alleviate this somewhat, I think, but it would be just as much of a pain to be blocked on some stupid merge-script bug.) 2. The most important thing, really, is a sufficiently descriptive explanation for the change. Any programmatic attempt to enforce this is sure to drive artificial gaming of the metric; i.e. if we require 3 sentences, "fixed random junk in foo module" will become "Fixed random. Junk in. Foo module." 3. 99% of the time (e.g. all "normal" commits) should be in this format; however, the underlying tools, to be honest, are still kind of primitive, and we still have to leave a little wiggle room to deal with their foibles. At least it isn't the masochistic chewing-gum-and-duct-tape perpetual catastrophe of CVS, but SVN still does annoying things like screwing up merges. If there is a commit with a nice long description and a 'fixes' and everything, which needs to be reverted and reapplied for some dumb technical reason, I'd like to see r1234: "author: foo; reviewer: bar; wonderful wonderful description ..." r1235: "crap, reverting r1234 because of svn bug #9813741" r1236: "reapplying r1234 merged in utterly retarded way to work around svn bug #9813741" not the entire description of r1234 repeated twice -- or, more realistically, three times, since the revert in the middle would also have to have a conformant commit message. 4. precommit hooks keep the SVN SSH transport alive to report errors to the client for the duration of the hook. When I last investigated (although this was in an SVN pre-release, a good 2+ years ago) if the connection drops while you're validating the commit message, it rejects the commit. This would be especially annoying if you were committing an important fix over some intermittent link, like a GPRS modem. That's not a common occurrance, but then again, if something were important enough that someone needed to commit code on a *GPRS modem* that is probably the time they would _least_ like something technical to go wrong with the process. This is basically just the same reason that, despite our hardcore attitude towards tests, we don't have buildbot automatically reject or revert commits based on automated test failures. The tools (in that case, buildbot, trial, and our test suite) are too primitive to rely on without some level of human judgement. We've been moving to a stricter model so that the human judgements can be as mechanical and apolitical as possible (e.g. "was this a test failure due to too much load on the buildbot / known intermittent failure", not "is this an important enough area of code to worry about test failures in") but we still need it there. I am eager to be corrected however; the more work our tools can reliably do for us the better. Has anybody had a good experience with automatically-rejected commits in another reasonably sized project before?
On 6/22/06, glyph@divmod.com <glyph@divmod.com> wrote:
This is basically just the same reason that, despite our hardcore attitude towards tests, we don't haThe tools (in that case, buildbot, trial, and our test suite) are too primitive to rely on without some level of human judgement.
Yeah?! Well your face is primitive! :) jml
On Wed, Jun 21, 2006 at 11:00:05PM -0400, glyph@divmod.com wrote: [...]
Two things make this a non-issue, hopefully: - a script that checks that a commit message matches a regex is pretty damn simple. - in case of emergency, it shouldn't be hard to just disable the offending commit-hook until such time as it can be fixed properly.
Well, I was really only suggesting that the presence of mandatory metadata is checked for, not that we write an AI to check that the text of the messages make sense :)
Good point. So there either needs to be a way to explicitly flag that this message is ok despite not being in the usual format, or we need to anticipate all possible forms of exceptional commits. Requiring e.g. "[override=reason]" in the text of the commit message somewhere could do this. This of course complicates the checker slightly, which relates to point 1, but not impossibly so.
The time it takes to validate a commit message against a regex should be negligible. I doubt this will be a real problem.
There are important differences between a commit message checker and our test suite. Our test suite gives way too many false positives, and does so unpredictably. Our test suite takes many minutes to run, at best. Our test suite should be run on multiple platforms for full effectiveness. Our test suite is extremely complex. A simple "does the commit message match this regex" check has none of these problems, assuming the regex isn't insane.
Yes, the Launchpad development process automatically rejects commits, both for malformed commit messages[1] and for test failures. It's generally worked quite well. The test suite takes similar lengths of time to Twisted's to run (but without the portability concerns, thankfully), but we submit requests to merge to trunk asynchronously (via gpg-signed email), so this isn't a killer for poor connections. We have occasionally had problems where a system upgrade or other supposedly unrelated change causes the test suite on trunk to start failing, effectively blocking all merges, requiring admin intervention. Old processes from previous test runs can also be an issue. It's generally worked very well for us, though. And it means we are forced to have zero tolerance of failing tests. Developers get unhappy if there's an intermittently failing test that's blocking their merges :) We use PQM ("Patch Queue Manager", bzr branch available here: http://people.ubuntu.com/~robertc/pqm/trunk/, bugs etc here: https://launchpad.net/products/pqm) to do this. -Andrew. [1] We require either "[trivial]", "[r=reviewer]", or "[rs=approver]" to be in the message. "r=" means "reviewed and approved by" and "rs=" means "rubber stamped by", i.e. not closely reviewed by the person (generally a manager) approving it anyway is happy with it being merged despite that and is responsible for the result. "trivial" should be obvious ;). Regardless of commit message, the full test suite is always run.
participants (10)
-
Andrew Bennetts -
Christopher Armstrong -
Cory Dodt -
glyph＠divmod.com -
Itamar Shtull-Trauring -
Jean-Paul Calderone -
Jonathan Lange -
L. Daniel Burr -
Mike Pelletier -
Stephen Thorne