Hi,
various big code changes/additions have been committed as trivial by others in the past, so I am considering to follow the same policy and declare all of my future patches as trivial and commit them instantly if I feel like it. That would surely speed up development for me.
Comments/Flames/Applause welcome.
Regards, Carl-Daniel
On Nov 20, 2007 10:20 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
various big code changes/additions have been committed as trivial by others in the past, so I am considering to follow the same policy and declare all of my future patches as trivial and commit them instantly if I feel like it. That would surely speed up development for me.
Comments/Flames/Applause welcome.
no, you should take us to task when we make that mistake, and I'm sorry if I have done it too much myself.
Let's stick to the process, and try to flag violations of the process.
thanks
ron
On 20.11.2007 19:20, ron minnich wrote:
On Nov 20, 2007 10:20 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
various big code changes/additions have been committed as trivial by others in the past, so I am considering to follow the same policy and declare all of my future patches as trivial and commit them instantly if I feel like it. That would surely speed up development for me.
Comments/Flames/Applause welcome.
no, you should take us to task when we make that mistake, and I'm sorry if I have done it too much myself.
Let's stick to the process, and try to flag violations of the process.
OK, can we decide on what should be (not) allowed, preferably as regexp for the diff?
Suggestion for NOT allowed stuff: * Adding files (if they were forgotten in the previous non-trivial commit, reuse the Ack from there) * Changing code unless it is a build fix and has "build fix" in the commit log
Checking for added files in the commit hook is easy. Finding out whether a patch touches code or comments is difficult. My idea is to strip comments from the file before and after modification, then run "diff -uw" on both versions.
Thoughts?
Regards, Carl-Daniel
On Tue, Nov 20, 2007 at 07:40:26PM +0100, Carl-Daniel Hailfinger wrote:
On 20.11.2007 19:20, ron minnich wrote:
On Nov 20, 2007 10:20 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
various big code changes/additions have been committed as trivial by others in the past, so I am considering to follow the same policy and declare all of my future patches as trivial and commit them instantly if I feel like it. That would surely speed up development for me.
Comments/Flames/Applause welcome.
no, you should take us to task when we make that mistake, and I'm sorry if I have done it too much myself.
Let's stick to the process, and try to flag violations of the process.
OK, can we decide on what should be (not) allowed, preferably as regexp for the diff?
Please don't over-engineer this. It's fine to just flame the committer who did a trivial commit without it being really trivial (yeah, I know, I'm guilty of this sometimes too).
In the worst case, if the commit really _breaks_ something or is wrong and there's opposition, we can just revert it (which I did in the past, too, with one of my "trivial" fixes).
Suggestion for NOT allowed stuff:
- Adding files (if they were forgotten in the previous non-trivial
commit, reuse the Ack from there)
Why? I think this should be allowed. If the whole patch was acked and you simply forget to 'svn add' a file (i.e. commit only parts of the patch) it's perfectly valid to commit the forgotten file with that ACK.
- Changing code unless it is a build fix and has "build fix" in the
commit log
No, I don't think we want this to be _that_ daunting. Even code changes can be "trivial" (wrong word maybe, "obviously correct" might be better) sometimes. I don't think we should restrict this to comment changes only.
Good examples are the "constify" patches, using PCI ID #defines instead of the hard-coded numbers, and similar stuff.
Checking for added files in the commit hook is easy. Finding out whether a patch touches code or comments is difficult. My idea is to strip comments from the file before and after modification, then run "diff -uw" on both versions.
Overkill, IMO. Just flame whoever did crap, in the worst case we revert the patch.
Uwe.
Uwe Hermann wrote:
On Tue, Nov 20, 2007 at 07:40:26PM +0100, Carl-Daniel Hailfinger wrote:
On 20.11.2007 19:20, ron minnich wrote:
On Nov 20, 2007 10:20 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
various big code changes/additions have been committed as trivial by others in the past, so I am considering to follow the same policy and declare all of my future patches as trivial and commit them instantly if I feel like it. That would surely speed up development for me.
Comments/Flames/Applause welcome.
no, you should take us to task when we make that mistake, and I'm sorry if I have done it too much myself.
Let's stick to the process, and try to flag violations of the process.
OK, can we decide on what should be (not) allowed, preferably as regexp for the diff?
Please don't over-engineer this. It's fine to just flame the committer who did a trivial commit without it being really trivial (yeah, I know, I'm guilty of this sometimes too).
In the worst case, if the commit really _breaks_ something or is wrong and there's opposition, we can just revert it (which I did in the past, too, with one of my "trivial" fixes).
Suggestion for NOT allowed stuff:
- Adding files (if they were forgotten in the previous non-trivial
commit, reuse the Ack from there)
Why? I think this should be allowed. If the whole patch was acked and you simply forget to 'svn add' a file (i.e. commit only parts of the patch) it's perfectly valid to commit the forgotten file with that ACK.
I THINK Carl-Daniel meant that was an exception, that if the files were forgotten in the *commit*, not the original patch, then to use the ack from the patch. If so, I fully agree.
- Changing code unless it is a build fix and has "build fix" in the
commit log
No, I don't think we want this to be _that_ daunting. Even code changes can be "trivial" (wrong word maybe, "obviously correct" might be better) sometimes. I don't think we should restrict this to comment changes only.
Good examples are the "constify" patches, using PCI ID #defines instead of the hard-coded numbers, and similar stuff.
These might be exceptions, they don't make any major change to the way the code builds/runs. IMO, that should be the deciding factor: if the patch makes any change to the way the code builds and runs, then it's not trivial. The ONLY exception to that rule would be small fixes to make abuild happy.
Checking for added files in the commit hook is easy. Finding out whether a patch touches code or comments is difficult. My idea is to strip comments from the file before and after modification, then run "diff -uw" on both versions.
Overkill, IMO. Just flame whoever did crap, in the worst case we revert the patch.
Uwe.
I think if the commit hooks are easy enough to create, then we should consider it. I think we've discussed this enough times, although this may be the first time we've laid down rules. Should this go into the wiki?
-Corey
On Tuesday 20 November 2007, Uwe Hermann wrote:
On Tue, Nov 20, 2007 at 07:40:26PM +0100, Carl-Daniel Hailfinger wrote:
OK, can we decide on what should be (not) allowed, preferably as regexp for the diff?
Please don't over-engineer this. It's fine to just flame the committer who did a trivial commit without it being really trivial (yeah, I know, I'm guilty of this sometimes too).
In the worst case, if the commit really _breaks_ something or is wrong and there's opposition, we can just revert it (which I did in the past, too, with one of my "trivial" fixes).
Checking for added files in the commit hook is easy. [...]
Overkill, IMO. Just flame whoever did crap, in the worst case we revert the patch.
Seconded. A "trivial" patch must _never_ break anything. Leave that basically to each committer's judgement. If it does break something, flame at will; we all make mistakes, but the blame must hurt ;-) In the long run, someone incapable of forseeing such breakage should not retain commit rights, IMHO.
Besides that, do we agree that at least adding a new function or macro is non-trivial (by definition, if you like)? This would also cover refactoring and the design of new subsystems and would allow to split out a new file from an existing big one OTOH.
Torsten
On Tue, Nov 20, 2007 at 07:57:51PM +0100, Uwe Hermann wrote:
On Tue, Nov 20, 2007 at 07:40:26PM +0100, Carl-Daniel Hailfinger wrote:
OK, can we decide on what should be (not) allowed, preferably as regexp for the diff?
Please don't over-engineer this.
I am with Uwe. Let's use common sense and respect our peers.
//Peter
On 20.11.2007 21:51, Corey Osgood wrote:
Uwe Hermann wrote:
On Tue, Nov 20, 2007 at 07:40:26PM +0100, Carl-Daniel Hailfinger wrote:
On 20.11.2007 19:20, ron minnich wrote:
On Nov 20, 2007 10:20 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
various big code changes/additions have been committed as trivial by others in the past, so I am considering to follow the same policy and declare all of my future patches as trivial and commit them instantly if I feel like it. That would surely speed up development for me.
Comments/Flames/Applause welcome.
no, you should take us to task when we make that mistake, and I'm sorry if I have done it too much myself.
Let's stick to the process, and try to flag violations of the process.
OK, can we decide on what should be (not) allowed, preferably as regexp for the diff?
Please don't over-engineer this. It's fine to just flame the committer who did a trivial commit without it being really trivial (yeah, I know, I'm guilty of this sometimes too).
In the worst case, if the commit really _breaks_ something or is wrong and there's opposition, we can just revert it (which I did in the past, too, with one of my "trivial" fixes).
I'd like to place the burden of possibly problematic patches on the committer, not on somebody who has to decide after the commit was made. Reverting a patch is done much less lightly than committing a patch, even if that patch is big and invasive.
With your reasoning, I should have committed a patch to enforce our rules for trivial commits in the commit hooks and you would have to argue that it should be reverted. I think adding such a commit hook is trivial.
Suggestion for NOT allowed stuff:
- Adding files (if they were forgotten in the previous non-trivial
commit, reuse the Ack from there)
Why? I think this should be allowed. If the whole patch was acked and you simply forget to 'svn add' a file (i.e. commit only parts of the patch) it's perfectly valid to commit the forgotten file with that ACK.
I THINK Carl-Daniel meant that was an exception, that if the files were forgotten in the *commit*, not the original patch, then to use the ack from the patch. If so, I fully agree.
Thanks, Corey, that's what I meant. If you forgot a svn add in the original acked patch, simply carry over signoff and ack from the previous patch.
- Changing code unless it is a build fix and has "build fix" in the
commit log
No, I don't think we want this to be _that_ daunting. Even code changes can be "trivial" (wrong word maybe, "obviously correct" might be better) sometimes. I don't think we should restrict this to comment changes only.
Good examples are the "constify" patches, using PCI ID #defines instead of the hard-coded numbers, and similar stuff.
These might be exceptions, they don't make any major change to the way the code builds/runs. IMO, that should be the deciding factor: if the patch makes any change to the way the code builds and runs, then it's not trivial. The ONLY exception to that rule would be small fixes to make abuild happy.
So what would be the way? Check for non-whitespace differences after preprocessing?
Checking for added files in the commit hook is easy. Finding out whether a patch touches code or comments is difficult. My idea is to strip comments from the file before and after modification, then run "diff -uw" on both versions.
Overkill, IMO. Just flame whoever did crap, in the worst case we revert the patch.
I think if the commit hooks are easy enough to create, then we should consider it. I think we've discussed this enough times, although this may be the first time we've laid down rules. Should this go into the wiki?
I have modified the commit hooks before. As long as we can agree on what needs an ack (and in case of doubt, require one), modifying the hook is easy.
Regards, Carl-Daniel
On 21.11.2007 00:30, Torsten Duwe wrote:
On Tuesday 20 November 2007, Uwe Hermann wrote:
On Tue, Nov 20, 2007 at 07:40:26PM +0100, Carl-Daniel Hailfinger wrote:
OK, can we decide on what should be (not) allowed, preferably as regexp for the diff?
Checking for added files in the commit hook is easy. [...]
Overkill, IMO. Just flame whoever did crap, in the worst case we revert the patch.
Seconded. A "trivial" patch must _never_ break anything. Leave that basically to each committer's judgement. If it does break something, flame at will; we all make mistakes, but the blame must hurt ;-) In the long run, someone incapable of forseeing such breakage should not retain commit rights, IMHO.
Removing commit rights of a person is surely a drastic measure, and the only one that works in case we don't enforce sane behaviour in the commit hooks. If I had my commit rights revoked, I'd probably feel offended very much.
Besides that, do we agree that at least adding a new function or macro is non-trivial (by definition, if you like)? This would also cover refactoring and the design of new subsystems and would allow to split out a new file from an existing big one OTOH.
By that logic, adding a new file is non-trivial as well. Nice. The conditions above surely can be checked in a commit hook.
Regards, Carl-Daniel
On 21.11.2007 06:19, Peter Stuge wrote:
On Tue, Nov 20, 2007 at 07:57:51PM +0100, Uwe Hermann wrote:
On Tue, Nov 20, 2007 at 07:40:26PM +0100, Carl-Daniel Hailfinger wrote:
OK, can we decide on what should be (not) allowed, preferably as regexp for the diff?
Please don't over-engineer this.
I am with Uwe. Let's use common sense and respect our peers.
We already check for Acked-by and Signed-off-by in the commit hooks. Nobody has complained about it. Nobody has stamped out that check as an example of disrespect.
I produce two classes of patches: * Patches where I'm not sure they will work or where I feel others would want to have a say in a design decision as well. These patches are either without a signoff or carry the explicit label [RFC] or some other strong wording against committing. * Patches which are IMO obviously correct. They can involve pretty large code changes or rewriting fragile (and long-time unchanged) code everybody uses. Obviously correct patches are trivial.
I have no problem committing a rewrite of the CAR setup assembly code. It is obviously correct for me even without having ever looked at any of the relevant data sheets or having tested it on real hardware. Do you really want me to commit that? The patch is ready.
Regards, Carl-Daniel
On Saturday 24 November 2007, Carl-Daniel Hailfinger wrote:
On 21.11.2007 00:30, Torsten Duwe wrote:
Seconded. A "trivial" patch must _never_ break anything. Leave that basically to each committer's judgement. If it does break something, flame at will; we all make mistakes, but the blame must hurt ;-) In the long run, someone incapable of forseeing such breakage should not retain commit rights, IMHO.
Removing commit rights of a person is surely a drastic measure, and the only one that works in case we don't enforce sane behaviour in the commit hooks. If I had my commit rights revoked, I'd probably feel offended very much.
Think about the amount of offense you had to cause before that happens. Again: >>A "trivial" patch must _never_ break anything<<. When in doubt, ask for ACK, better safe than sorry. There must be some ultima ratio if constant blaming does not help, that's my point. Commit hooks are of very little help, and tend to get abused in some environments.
Besides that, do we agree that at least adding a new function or macro is non-trivial (by definition, if you like)? This would also cover refactoring and the design of new subsystems and would allow to split out a new file from an existing big one OTOH.
By that logic, adding a new file is non-trivial as well. Nice. The conditions above surely can be checked in a commit hook.
If it contains either new functions or new macros. Pulling definitions from a .c-file into a new header can be considered trivial sometimes. If you try to automate the recognition of all "trivial" patches you'll surely generate false rejects.
Torsten
On Saturday 24 November 2007, Carl-Daniel Hailfinger wrote:
I produce two classes of patches:
- Patches where I'm not sure they will work [...]
- Patches which are IMO obviously correct. They can involve pretty large
code changes or rewriting fragile (and long-time unchanged) code everybody uses. Obviously correct patches are trivial.
That is a courageous assumption. Code that looks correct is trivial? I miss the third class of patches like comment reformatting, re-indent, naming constant values and the like, changes every one of us would consider trivial.
I have no problem committing a rewrite of the CAR setup assembly code. It is obviously correct for me even without having ever looked at any of the relevant data sheets or having tested it on real hardware. Do you really want me to commit that? The patch is ready.
If that code does work in all cases I might consider you a genius, depending on the new code. If it breaks under some circumstances and you don't care, then this would indeed be the first step to have your commit rights removed, IMHO. If you're not feeling lucky, ask for a second opinion. It's that simple.
Torsten Duwe schrieb:
On Saturday 24 November 2007, Carl-Daniel Hailfinger wrote:
- Patches which are IMO obviously correct. They can involve pretty large
code changes or rewriting fragile (and long-time unchanged) code everybody uses. Obviously correct patches are trivial.
That is a courageous assumption. Code that looks correct is trivial? I miss the third class of patches like comment reformatting, re-indent, naming constant values and the like, changes every one of us would consider trivial.
It also shows Carl-Daniel is trying to make a point by being sarcastic or polemic. Not sure what this is going to be good for. Certainly not the project.
I have no problem committing a rewrite of the CAR setup assembly code. It is obviously correct for me even without having ever looked at any of the relevant data sheets or having tested it on real hardware. Do you really want me to commit that? The patch is ready.
If that code does work in all cases I might consider you a genius, depending on the new code. If it breaks under some circumstances and you don't care, then this would indeed be the first step to have your commit rights removed, IMHO. If you're not feeling lucky, ask for a second opinion. It's that simple.
There was a time when a committer would break 50 boards in one night and then vanish and NEVER come back to fix it again.
Back then we agreed on rules to avoid such stupidity. Then we started getting more and more strict with our rules. So much that we started annoying every external contributor with those rules.
I assume there is some common sense in between those two extremes. Something all of us can live with, without becoming a nitpicker paralyzing the project's progress, nor having code quality drop significantly.
Stefan
Carl-Daniel Hailfinger schrieb:
It is obviously correct for me even without having ever looked at any of the relevant data sheets or having tested it on real hardware.
So you are asking to have your commit rights removed because you deny any common sense whatsoever?
Or are you saying that rewriting CAR is about as complex as adding a new superio in the superiotool or a new mainboard down in src/mainboard/X/Y and can break as much?
I don't care if someone checks in a broken mainboard and is going to fix it later. I do care if someone wastes my time and breaks central components though.
What kind of problem that has been there in the past are you trying to fix with this discussion?
Stefan
I would define a "trivial patch" as follows:
1. Compile source. 2. Apply patch. 3. Compile source. 4. If the object code from (1) is the same as the object code from (3) then the patch is trivial.
my .02 Russ
-
Carl-Daniel Hailfinger -
Corey Osgood -
Peter Stuge -
ron minnich -
Russell Whitaker -
Stefan Reinauer -
Torsten Duwe -
Uwe Hermann