Hi! Please skip my two previous letters with patch because there all tabs were changed to spaces. These diffs will be hard to use. I'll send good ones soon. Thanks! Boris
On Fri, 9 Jan 2015 22:32:05 +0300 Boris Baykov dev@borisbaykov.com wrote:
Hi! Please skip my two previous letters with patch because there all tabs were changed to spaces. These diffs will be hard to use. I'll send good ones soon.
Hello,
thanks for this big effort. If you could try to split the patch in smaller pieces, it would make reviewing them way more easy. Also, there is no need to send a diff against 0.9.7... a diff on top of the current revision only is fine.
Hi! Please skip my two previous letters with patch because there all tabs were changed to spaces. These diffs will be hard to use. I'll send good ones soon.
Hello,
thanks for this big effort. If you could try to split the patch in smaller pieces, it would make reviewing them way more easy. Also, there is no need to send a diff against 0.9.7... a diff on top of the current revision only is fine.
Ok, in my next letter I'll sent the patch to the current revision as attach in bzip2 because my mail system completely doesn't want to send the patch without changing tabs to spaces or wrapping strings.
Please explain how is better to split this patch to make it easy for review.
On Fri, 9 Jan 2015 23:54:28 +0300 Boris Baykov dev@borisbaykov.com wrote:
Ok, in my next letter I'll sent the patch to the current revision as attach in bzip2 because my mail system completely doesn't want to send the patch without changing tabs to spaces or wrapping strings.
OK!
Please explain how is better to split this patch to make it easy for review.
We want to verify all changes to flashrom in quite some detail to keep it as stable as it is. A single huge patch like yours takes a very long time to fully understand. It is probably not even possible to review such a big change in one go without making some mistakes or overlook error of the original author. Smaller independent patches could be reviewed entirely without getting too much of a headache. Does that make sense to you?
Of course it is a lot of work to split an existing patch into smaller standalone bits... so maybe, if you would really like to do that, it would be better to wait for a first feedback from us... we will probably like you to change a few things anyway. Of course you are not obliged to do anything at all if you don't want to, we are grateful for your patch as it is... :)
On Fri, 9 Jan 2015 23:54:28 +0300 Boris Baykov dev@borisbaykov.com wrote:
Ok, in my next letter I'll sent the patch to the current revision as attach in bzip2 because my mail system completely doesn't want to send the patch without changing tabs to spaces or wrapping strings.
OK!
Please explain how is better to split this patch to make it easy for review.
We want to verify all changes to flashrom in quite some detail to keep it as stable as it is. A single huge patch like yours takes a very long time to fully understand. It is probably not even possible to review such a big change in one go without making some mistakes or overlook error of the original author. Smaller independent patches could be reviewed entirely without getting too much of a headache. Does that make sense to you?
Of course it is a lot of work to split an existing patch into smaller standalone bits... so maybe, if you would really like to do that, it would be better to wait for a first feedback from us... we will probably like you to change a few things anyway. Of course you are not obliged to do anything at all if you don't want to, we are grateful for your patch as it is... :)
I've just sent the patch to Mailing List as an attach and checked it there.
To make the review easy I've carefully inspected all my changes and noticed each change in the patch description. Also I added detailed comments near almost all of changes. There we have 10 files changed and 2 files added.
Unfortunately I can't imagine how to split this patch in parts because all the patch logically has one idea and that's why my changes in different files are connected to other changes and they work together.
Probably I might split it by files and produce 10 patch files but I don't really think that it's a good idea. In other case we can have a situation were some files would be changed and others don't. It can make source uncompilable and cause more troubles.
Also we can split this long text by several letters as it is without rebuild of the patch. It's significantly easier then to divide the patch into several patches :-)
I'm happy to add my part to such good and well known project like Flashrom. So, any questions about the patch are welcome :-)
Boris
On Sat, 10 Jan 2015 01:34:34 +0300 Boris Baykov dev@borisbaykov.com wrote:
On Fri, 9 Jan 2015 22:49:56 +0100 Stefan Tauner stefan.tauner@alumni.tuwien.ac.at
On Fri, 9 Jan 2015 23:54:28 +0300 Boris Baykov dev@borisbaykov.com wrote:
[...]
Please explain how is better to split this patch to make it easy for review.
We want to verify all changes to flashrom in quite some detail to keep it as stable as it is. A single huge patch like yours takes a very long time to fully understand. It is probably not even possible to review such a big change in one go without making some mistakes or overlook error of the original author. Smaller independent patches could be reviewed entirely without getting too much of a headache. Does that make sense to you?
[...]
Unfortunately I can't imagine how to split this patch in parts because all the patch logically has one idea and that's why my changes in different files are connected to other changes and they work together.
These are just examples of how you _could_ logically split you patch: - base support for the JEDEC extensions can go into a separate patch; - flashrom hooking for the extensions and for 4 byte addressing can go into a dedicated patch; - the simulation code may go into a followup patch, optionality is a hint for identifying a splitting point; - programmers specific support for 4ba can go into a dedicated patch; - new flash chip definitions can go into a dedicated patch; - cosmetic fixes, if any, go into a dedicated patch; - standalone UI changes (strings in console messages) can go into a dedicated patch; - Implementation of the progress meter can go into a separate patch.
As you said, splitting by files in general does not make sense, but splitting by "feature" does.
The idea is that an incremental patchset is a coarse (and cleaned up) version of your development history.
If you want details of examples of _how_ this can be achieved in practice starting from a single big patch I can elaborate more.
The tools you use influence the way you operate: svn, when used locally, tends to encourage big patches, where a tool with better support for local branches (e.g. git) could make it easier to track locally the development history and review it before public submission. No flame intended whatsoever.
Ciao, Antonio
-
Antonio Ospite -
Boris Baykov -
Stefan Tauner