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.
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.
Thanks, Antonio for the excellent explanation! It looks like a set of nesting dolls from traditional russian culture where each next doll goes inside of previous one :-)
Basing on your list I developed the following order of 10 + 2 patches, where basically 10 patches and added extra 2 small patches 3a and 4a for explanation purposes only to provide an example how to use new features. This means that to make 4BA version we should patch 1+2+3+4+5+6+7+8+9+10, and 3a and 4a can be after 3 and 4 to show code examples. Is it ok?
Please explain why do you think to split the first patch into two parts ?
- 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;
From one side the idea is good from another side we'll receive the patch (first one) which doesn't make any sense besides of source size increase.
1. 4BA: Basic support for 4-bytes addressing mode 2. 4BA: Winbond W25Q256.V chip declaration 3. 4BA: Support of Extended Address Register 3a.4BA: Temporary W25Q256.V declaration update to work using Ext.Addr.Reg 4. 4BA: Support for new direct 4BA instructions + W25Q256.V declaration update 4a.4BA: Temporary W25Q256.V declaration update to work via direct funcs 5. 4BA: Progress visualization 6. 4BA: SFDP 1.6 parser for compliance with JESD216B 7. 4BA: SFDP 1.6 simulation (for testing purposes) 8. 4BA: SFDP downgrade to 1.5 to support Micron chips 9. 4BA: Some small visual changes for 4-byte addresses 10.4BA: Temporary banner fix about the 4BA patch
So, I can split the patch today and resubmit the patches set here.
We have now build 1866 already. Do I need to update my patch for this build?
On Sat, 10 Jan 2015 13:31:08 +0300 Boris Baykov dev@borisbaykov.com wrote:
We have now build 1866 already. Do I need to update my patch for this build?
And there will probably more commits before we can commit your patch(es). That is in general no problem as long as the commits do not conflict with your patches... in that case *someone* *has* to resolve the conflicts and rebase the patches on top of the new ones.
When there are no conflicts with other patches the rebasing still needs to be done before committing, but that is trivial and can easily be done by the committer.
If you split your patch then it would make sense to rebase that work on the newest flashrom revision, else I/we will manage that on our own anyway.
On Sat, 10 Jan 2015 13:31:08 +0300 Boris Baykov dev@borisbaykov.com wrote:
[...]
Basing on your list I developed the following order of 10 + 2 patches, where basically 10 patches and added extra 2 small patches 3a and 4a for explanation purposes only to provide an example how to use new features. This means that to make 4BA version we should patch 1+2+3+4+5+6+7+8+9+10, and 3a and 4a can be after 3 and 4 to show code examples. Is it ok?
Great, more than OK.
Please explain why do you think to split the first patch into two parts ?
- 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;
From one side the idea is good from another side we'll receive the patch (first one) which doesn't make any sense besides of source size increase.
It was just an example, I had only skimmed through the code not actually read it. You know the code better, you should decide what the best way to split the patch is.
- 4BA: Basic support for 4-bytes addressing mode
- 4BA: Winbond W25Q256.V chip declaration
- 4BA: Support of Extended Address Register
3a.4BA: Temporary W25Q256.V declaration update to work using Ext.Addr.Reg 4. 4BA: Support for new direct 4BA instructions + W25Q256.V declaration update 4a.4BA: Temporary W25Q256.V declaration update to work via direct funcs 5. 4BA: Progress visualization 6. 4BA: SFDP 1.6 parser for compliance with JESD216B 7. 4BA: SFDP 1.6 simulation (for testing purposes) 8. 4BA: SFDP downgrade to 1.5 to support Micron chips 9. 4BA: Some small visual changes for 4-byte addresses 10.4BA: Temporary banner fix about the 4BA patch
Sounds pretty good to me, and in the future using a similar approach from the start will make things easier for you too.
In the final submission the numeration will be just incremental (like in a list): 3a becomes 4 and so on; but in principle a development history looks more like a graph indeed, just like you pictured it.
One thing to remember is that, as a general rule, the code should still "work" after each patch, at least it should still compile.
So, I can split the patch today and resubmit the patches set here.
If you feel like it, and you have some time, try using "git": play with local branches and with the "git rebase" command, you can clone the flashrom svn repository as a git repository using the "git svn" command.
Since your patch was based on top of r1861, a possible way to operate can be this:
# Clone the svn repository (this will get the _latest_ commit) git svn clone svn://flashrom.org/flashrom/trunk flashrom cd flashrom
# Start a local temporary branch named "temp-add-4BA-support", # starting from r1861 git checkout -b temp-add-4BA-support $(git svn find-rev r1861)
# Apply your patch to this branch, and commit it git apply flashrom-0.9.7-r1861_4ba.patch git commit -a -m "Huge patch"
# Rebase the branch on top of the latest revision git rebase master
# Export the rebased patch, the file will be named 0001-Huge-patch.patch git format-patch -1
# Switch to the master branch and delete the temp branch git checkout master git branch -D temp-add-4BA-support
# Start the actual branch from the latest revision, that will be # the one you'll be working on git checkout -b add-4BA-support
# Apply the patch (you will see warning about trailing spaces, you can # fix those at the end) and check the diff git apply 0001-Huge-patch.patch git diff
# Use "git add -p" to select the pieces of the huge patch targeted for # the _first_ dedicated patch, when finished with the first patch, commit. git add -p git commit
# "git commit" will present you an editor to write your commit message, # for the first patch it could be something like this # (ignore the -----... lines): ------------------------------------------------------------ Basic support for 4-bytes addressing mode
Signed-off-by: Boris Baykov dev@borisbaykov.com ------------------------------------------------------------
Repeat the "git add -p" thing for the other patches, until "git add -p" does not do anything anymore.
After that you can see the split patchset by comparing the master branch with the add-4BA-support branch: git whatchanged --reverse master..
Note that "git add -p" gets some time to get used to, that may discourage you, but again I am just proposing a _possible_ way to operate, you should use what is more comfortable for you.
If you don't want to learn git now, just ignore what I wrote, it may even be more confusing to newcomers, I know. :)
Ciao, Antonio
On Sat, 10 Jan 2015 13:31:08 +0300 Boris Baykov dev@borisbaykov.com wrote:
[...]
Basing on your list I developed the following order of 10 + 2 patches, where basically 10 patches and added extra 2 small patches 3a and 4a for explanation purposes only to provide an example how to use new features. This means that to make 4BA version we should patch 1+2+3+4+5+6+7+8+9+10, and 3a and 4a can be after 3 and 4 to show code examples. Is it ok?
Great, more than OK.
Please explain why do you think to split the first patch into two parts ?
- 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;
From one side the idea is good from another side we'll receive the patch (first one) which doesn't make any sense besides of source size increase.
It was just an example, I had only skimmed through the code not actually read it. You know the code better, you should decide what the best way to split the patch is.
- 4BA: Basic support for 4-bytes addressing mode
- 4BA: Winbond W25Q256.V chip declaration
- 4BA: Support of Extended Address Register
3a.4BA: Temporary W25Q256.V declaration update to work using Ext.Addr.Reg 4. 4BA: Support for new direct 4BA instructions + W25Q256.V declaration update 4a.4BA: Temporary W25Q256.V declaration update to work via direct funcs 5. 4BA: Progress visualization 6. 4BA: SFDP 1.6 parser for compliance with JESD216B 7. 4BA: SFDP 1.6 simulation (for testing purposes) 8. 4BA: SFDP downgrade to 1.5 to support Micron chips 9. 4BA: Some small visual changes for 4-byte addresses 10.4BA: Temporary banner fix about the 4BA patch
Sounds pretty good to me, and in the future using a similar approach from the start will make things easier for you too.
In the final submission the numeration will be just incremental (like in a list): 3a becomes 4 and so on; but in principle a development history looks more like a graph indeed, just like you pictured it.
One thing to remember is that, as a general rule, the code should still "work" after each patch, at least it should still compile.
So, I can split the patch today and resubmit the patches set here.
If you feel like it, and you have some time, try using "git": play with local branches and with the "git rebase" command, you can clone the flashrom svn repository as a git repository using the "git svn" command.
Since your patch was based on top of r1861, a possible way to operate can be this:
# Clone the svn repository (this will get the _latest_ commit) git svn clone svn://flashrom.org/flashrom/trunk flashrom cd flashrom
# Start a local temporary branch named "temp-add-4BA-support", # starting from r1861 git checkout -b temp-add-4BA-support $(git svn find-rev r1861)
# Apply your patch to this branch, and commit it git apply flashrom-0.9.7-r1861_4ba.patch git commit -a -m "Huge patch"
# Rebase the branch on top of the latest revision git rebase master
# Export the rebased patch, the file will be named 0001-Huge-patch.patch git format-patch -1
# Switch to the master branch and delete the temp branch git checkout master git branch -D temp-add-4BA-support
# Start the actual branch from the latest revision, that will be # the one you'll be working on git checkout -b add-4BA-support
# Apply the patch (you will see warning about trailing spaces, you can # fix those at the end) and check the diff git apply 0001-Huge-patch.patch git diff
# Use "git add -p" to select the pieces of the huge patch targeted for # the _first_ dedicated patch, when finished with the first patch, commit. git add -p git commit
# "git commit" will present you an editor to write your commit message, # for the first patch it could be something like this
# (ignore the -----... lines):
Basic support for 4-bytes addressing mode
Signed-off-by: Boris Baykov dev@borisbaykov.com
Repeat the "git add -p" thing for the other patches, until "git add -p" does not do anything anymore.
After that you can see the split patchset by comparing the master branch with the add-4BA-support branch: git whatchanged --reverse master..
Note that "git add -p" gets some time to get used to, that may discourage you, but again I am just proposing a _possible_ way to operate, you should use what is more comfortable for you.
If you don't want to learn git now, just ignore what I wrote, it may even be more confusing to newcomers, I know. :)
Ciao, Antonio
Antonio, I appreciate your help with git. It's very useful. I thought about making new branch for this patch. Right now I have no time for this and I've already split this patch into 10+2 parts (+2 just examples, don't apply) and diffed it by Linux diff(1) instead of SVN or Git. It's enough for review. Later I probably add the branch and prepare scripts to do git diff.
My big thanks to you for ideas and explanation! It seems that the patch will be available for review soon. I've sent all parts here already.
Besides of 10+2 parts I've done, packed and sent the full SVN diff of last version of the patch because I fixed some small points while splitting. So, to apply the patch now we can use this.
Also I should tell that all parts of the patch is sent with spaces instead of tabs. To go over this trouble I packed and attacked each part to its letter. Use .patch files from attached .bzips.
-
Antonio Ospite -
Boris Baykov -
Stefan Tauner