Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/38662 )
Change subject: meson_options.txt: Align with tabs ......................................................................
meson_options.txt: Align with tabs
Aligned text clearly shows that all options use the same format.
Change-Id: Idf0881859416e3d824228a0f78447da0bf8604b2 Signed-off-by: Angel Pons th3fanbus@gmail.com --- M meson_options.txt 1 file changed, 35 insertions(+), 35 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/62/38662/1
diff --git a/meson_options.txt b/meson_options.txt index ea87311..f81e605 100644 --- a/meson_options.txt +++ b/meson_options.txt @@ -1,36 +1,36 @@ -option('pciutils', type : 'boolean', value : true, description : 'use pciutils') -option('usb', type : 'boolean', value : true, description : 'use libusb1') +option('pciutils', type : 'boolean', value : true, description : 'use pciutils') +option('usb', type : 'boolean', value : true, description : 'use libusb1')
-option('config_atahpt', type : 'boolean', value : false, description : 'Highpoint (HPT) ATA/RAID controllers') -option('config_atapromise', type : 'boolean', value : false, description : 'Promise ATA controller') -option('config_atavia', type : 'boolean', value : true, description : 'VIA VT6421A LPC memory') -option('config_buspirate_spi', type : 'boolean', value : true, description : 'Bus Pirate SPI') -option('config_ch341a_spi', type : 'boolean', value : true, description : 'Winchiphead CH341A') -option('config_dediprog', type : 'boolean', value : true, description : 'Dediprog SF100') -option('config_developerbox_spi', type : 'boolean', value : true, description : 'Developerbox emergency recovery') -option('config_digilent_spi', type : 'boolean', value : true, description : 'Digilent Development board JTAG') -option('config_drkaiser', type : 'boolean', value : true, description : 'Dr. Kaiser') -option('config_dummy', type : 'boolean', value : true, description : 'dummy tracing') -option('config_ft2232_spi', type : 'boolean', value : true, description : 'FT2232 SPI dongles') -option('config_gfxnvidia', type : 'boolean', value : true, description : 'NVIDIA graphics cards') -option('config_internal', type : 'boolean', value : true, description : 'internal/onboard') -option('config_internal_dmi', type : 'boolean', value : true, description : 'Use internal DMI parser') -option('config_it8212', type : 'boolean', value : true, description : 'ITE IT8212F PATA') -option('config_linux_mtd', type : 'boolean', value : true, description : 'Linux MTD interfaces') -option('config_linux_spi', type : 'boolean', value : true, description : 'Linux spidev interfaces') -option('config_mstarddc_spi', type : 'boolean', value : false, description : 'MSTAR DDC support') -option('config_nic3com', type : 'boolean', value : true, description : '3Com NICs') -option('config_nicintel_eeprom', type : 'boolean', value : true, description : 'EEPROM on Intel NICs') -option('config_nicintel_spi', type : 'boolean', value : true, description : 'SPI on Intel NICs') -option('config_nicintel', type : 'boolean', value : true, description : 'Intel NICs') -option('config_nicnatsemi', type : 'boolean', value : false, description : 'National Semiconductor NICs') -option('config_nicrealtek', type : 'boolean', value : true, description : 'Realtek NICs') -option('config_ogp_spi', type : 'boolean', value : true, description : 'SPI on OGP cards') -option('config_pickit2_spi', type : 'boolean', value : true, description : 'PICkit2 SPI') -option('config_pony_spi', type : 'boolean', value : true, description : 'PonyProg2000 SPI') -option('config_rayer_spi', type : 'boolean', value : true, description : 'RayeR SPIPGM') -option('config_satamv', type : 'boolean', value : true, description : 'Marvell SATA controllers') -option('config_satasii', type : 'boolean', value : true, description : 'SiI SATA controllers') -option('config_serprog', type : 'boolean', value : true, description : 'serprog') -option('config_usbblaster_spi', type : 'boolean', value : true, description : 'Altera USB-Blaster dongles') -option('config_stlinkv3_spi', type : 'boolean', value : true, description : 'STMicroelectronics STLINK-V3') +option('config_atahpt', type : 'boolean', value : false, description : 'Highpoint (HPT) ATA/RAID controllers') +option('config_atapromise', type : 'boolean', value : false, description : 'Promise ATA controller') +option('config_atavia', type : 'boolean', value : true, description : 'VIA VT6421A LPC memory') +option('config_buspirate_spi', type : 'boolean', value : true, description : 'Bus Pirate SPI') +option('config_ch341a_spi', type : 'boolean', value : true, description : 'Winchiphead CH341A') +option('config_dediprog', type : 'boolean', value : true, description : 'Dediprog SF100') +option('config_developerbox_spi', type : 'boolean', value : true, description : 'Developerbox emergency recovery') +option('config_digilent_spi', type : 'boolean', value : true, description : 'Digilent Development board JTAG') +option('config_drkaiser', type : 'boolean', value : true, description : 'Dr. Kaiser') +option('config_dummy', type : 'boolean', value : true, description : 'dummy tracing') +option('config_ft2232_spi', type : 'boolean', value : true, description : 'FT2232 SPI dongles') +option('config_gfxnvidia', type : 'boolean', value : true, description : 'NVIDIA graphics cards') +option('config_internal', type : 'boolean', value : true, description : 'internal/onboard') +option('config_internal_dmi', type : 'boolean', value : true, description : 'Use internal DMI parser') +option('config_it8212', type : 'boolean', value : true, description : 'ITE IT8212F PATA') +option('config_linux_mtd', type : 'boolean', value : true, description : 'Linux MTD interfaces') +option('config_linux_spi', type : 'boolean', value : true, description : 'Linux spidev interfaces') +option('config_mstarddc_spi', type : 'boolean', value : false, description : 'MSTAR DDC support') +option('config_nic3com', type : 'boolean', value : true, description : '3Com NICs') +option('config_nicintel_eeprom', type : 'boolean', value : true, description : 'EEPROM on Intel NICs') +option('config_nicintel_spi', type : 'boolean', value : true, description : 'SPI on Intel NICs') +option('config_nicintel', type : 'boolean', value : true, description : 'Intel NICs') +option('config_nicnatsemi', type : 'boolean', value : false, description : 'National Semiconductor NICs') +option('config_nicrealtek', type : 'boolean', value : true, description : 'Realtek NICs') +option('config_ogp_spi', type : 'boolean', value : true, description : 'SPI on OGP cards') +option('config_pickit2_spi', type : 'boolean', value : true, description : 'PICkit2 SPI') +option('config_pony_spi', type : 'boolean', value : true, description : 'PonyProg2000 SPI') +option('config_rayer_spi', type : 'boolean', value : true, description : 'RayeR SPIPGM') +option('config_satamv', type : 'boolean', value : true, description : 'Marvell SATA controllers') +option('config_satasii', type : 'boolean', value : true, description : 'SiI SATA controllers') +option('config_serprog', type : 'boolean', value : true, description : 'serprog') +option('config_usbblaster_spi', type : 'boolean', value : true, description : 'Altera USB-Blaster dongles') +option('config_stlinkv3_spi', type : 'boolean', value : true, description : 'STMicroelectronics STLINK-V3')
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38662 )
Change subject: meson_options.txt: Align with tabs ......................................................................
Patch Set 1: Code-Review+2
Looks great, thanks!
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38662 )
Change subject: meson_options.txt: Align with tabs ......................................................................
Patch Set 1: Code-Review+2
Carl-Daniel Hailfinger has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38662 )
Change subject: meson_options.txt: Align with tabs ......................................................................
Patch Set 1: Code-Review-1
Please don't. This breaks git blame and IMO makes the file harder to read. None of the meson files use tabs, and I'd rather have a mixed tabs/spaces style there.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38662 )
Change subject: meson_options.txt: Align with tabs ......................................................................
Patch Set 1:
Patch Set 1: Code-Review-1
Please don't. This breaks git blame and IMO makes the file harder to read. None of the meson files use tabs, and I'd rather have a mixed tabs/spaces style there.
Well, git blame on this rather recent part of the code isn't too critical. What about the reordering of flashchips.c, which also broke git blame? In any case, one can checkout the parent of the offending commit and run git blame on that.
With regard to tabs, would this be better if done with spaces instead? It would make the lines shorter.
Carl-Daniel Hailfinger has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38662 )
Change subject: meson_options.txt: Align with tabs ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1: Code-Review-1
Please don't. This breaks git blame and IMO makes the file harder to read. None of the meson files use tabs, and I'd rather have a mixed tabs/spaces style there.
Well, git blame on this rather recent part of the code isn't too critical. What about the reordering of flashchips.c, which also broke git blame? In any case, one can checkout the parent of the offending commit and run git blame on that.
With regard to tabs, would this be better if done with spaces instead? It would make the lines shorter.
You're right, the flashchips.c reordering should never have happened. Unfortunately, I didn't notice it before it was committed, otherwise I would have complained. I've done too much blame-chasing in my time as flashrom maintainer and would rather not repeat that ever again. When you're trying to track down a bug, the age of the code is less relevant than the number of no-op commits you have to wade through.
With regard to reformatting, both a reformat with tabs and a reformat with spaces do not make much sense. Once a new programmer with a longer name than expected comes along, someone will reformat the whole file again. And again. Not having this aligned is actually a feature, because then you don't have to realign it.
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38662 )
Change subject: meson_options.txt: Align with tabs ......................................................................
Patch Set 1: -Code-Review
I've done too much blame-chasing in my time as flashrom maintainer and would rather not repeat that ever again. When you're trying to track down a bug, the age of the code is less relevant than the number of no-op commits you have to wade through.
With regard to reformatting, both a reformat with tabs and a reformat with spaces do not make much sense. Once a new programmer with a longer name than expected comes along, someone will reformat the whole file again. And again. Not having this aligned is actually a feature, because then you don't have to realign it.
Carl-Daniel raises a good point about relying on spaces/tabs being fragile... I guess we could impose a length limit for programmer name, or put each element on its own line? Or just leave it alone.
I'm not sold on the blame argument since that can be used to argue against just about any non-critical refactoring or cleanup task. However I sympathize with wanting to make blame-chasing simpler.
Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38662 )
Change subject: meson_options.txt: Align with tabs ......................................................................
Patch Set 1: Code-Review+1
Patch Set 1: -Code-Review
I've done too much blame-chasing in my time as flashrom maintainer and would rather not repeat that ever again. When you're trying to track down a bug, the age of the code is less relevant than the number of no-op commits you have to wade through.
With regard to reformatting, both a reformat with tabs and a reformat with spaces do not make much sense. Once a new programmer with a longer name than expected comes along, someone will reformat the whole file again. And again. Not having this aligned is actually a feature, because then you don't have to realign it.
Carl-Daniel raises a good point about relying on spaces/tabs being fragile... I guess we could impose a length limit for programmer name, or put each element on its own line? Or just leave it alone.
That's fair. I think ultimately we just lack a standard on these things and a pre-submit hook to check said lint issues. Maybe we should only refactor with a hook coupled into the same commit to avoid regressions in stylistic changes and to keep things consistent for whatever we agree on.
Thanks Angel for putting in the work into flashrom and the suggestion regardless!
I'm not sold on the blame argument since that can be used to argue against just about any non-critical refactoring or cleanup task. However I sympathize with wanting to make blame-chasing simpler.
I agree, that is a problem with the tool not the code. Although you can avoid this with `git blame ~<refactor_hash> path/file`, a little unfortunate however shouldn't block a refactor where it makes sense.
David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38662 )
Change subject: meson_options.txt: Align with tabs ......................................................................
Patch Set 1:
Patch Set 1: Code-Review+1
Patch Set 1: -Code-Review
I've done too much blame-chasing in my time as flashrom maintainer and would rather not repeat that ever again. When you're trying to track down a bug, the age of the code is less relevant than the number of no-op commits you have to wade through.
With regard to reformatting, both a reformat with tabs and a reformat with spaces do not make much sense. Once a new programmer with a longer name than expected comes along, someone will reformat the whole file again. And again. Not having this aligned is actually a feature, because then you don't have to realign it.
Carl-Daniel raises a good point about relying on spaces/tabs being fragile... I guess we could impose a length limit for programmer name, or put each element on its own line? Or just leave it alone.
That's fair. I think ultimately we just lack a standard on these things and a pre-submit hook to check said lint issues. Maybe we should only refactor with a hook coupled into the same commit to avoid regressions in stylistic changes and to keep things consistent for whatever we agree on.
Yeah, we've played whack-a-mole with style issues for too long and trying to maintain/enforce style manually is very difficult in the long term for a project that grows organically like flashrom. Stuff like CB:38208 to a `.clang-format` seems like the right approach, though it needs some work (I'm experimenting in CB:38673).
I agree, that is a problem with the tool not the code. Although you can avoid this with `git blame ~<refactor_hash> path/file`, a little unfortunate however shouldn't block a refactor where it makes sense.
That's a helpful tip, thanks! There's also `-w` which tells `git blame` to ignores whitespace changes, and `-M` and `-C` to account for moving lines around, though they might be more difficult to use.
Angel Pons has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/38662 )
Change subject: meson_options.txt: Align with tabs ......................................................................
Abandoned
Carl-Daniel Hailfinger has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/38662 )
Change subject: meson_options.txt: Align with tabs ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1: Code-Review+1
Patch Set 1: -Code-Review
I've done too much blame-chasing in my time as flashrom maintainer and would rather not repeat that ever again. When you're trying to track down a bug, the age of the code is less relevant than the number of no-op commits you have to wade through.
With regard to reformatting, both a reformat with tabs and a reformat with spaces do not make much sense. Once a new programmer with a longer name than expected comes along, someone will reformat the whole file again. And again. Not having this aligned is actually a feature, because then you don't have to realign it.
Carl-Daniel raises a good point about relying on spaces/tabs being fragile... I guess we could impose a length limit for programmer name, or put each element on its own line? Or just leave it alone.
That's fair. I think ultimately we just lack a standard on these things and a pre-submit hook to check said lint issues. Maybe we should only refactor with a hook coupled into the same commit to avoid regressions in stylistic changes and to keep things consistent for whatever we agree on.
Yeah, we've played whack-a-mole with style issues for too long and trying to maintain/enforce style manually is very difficult in the long term for a project that grows organically like flashrom. Stuff like CB:38208 to a `.clang-format` seems like the right approach, though it needs some work (I'm experimenting in CB:38673).
Back when flashrom was still small enough to take care of coding style manually, we established different coding styles for different files: e.g. board_enable.c board lists had an exception from the line length limit to make these lists more readable. Similar choices were made for chipset_enable.c and other files. Documenting all these rules is pretty much a hard requirement before we can comfortably run tools to enforce the existing style. Should we continue the coding style discussion there?
I agree, that is a problem with the tool not the code. Although you can avoid this with `git blame ~<refactor_hash> path/file`, a little unfortunate however shouldn't block a refactor where it makes sense.
That's a helpful tip, thanks! There's also `-w` which tells `git blame` to ignores whitespace changes, and `-M` and `-C` to account for moving lines around, though they might be more difficult to use.
Thanks for the helful tips. 'git blame -M' is probably too fragile if moving lines around can change the meaning. I'm currently trying to automatically fix a subset of these issues (namely, alphabetically ordering programmer options in meson related files) with CB:38488. It's a bit harder than it looks at first glance, but I'm making progress. It might make sense to turn parts of that script into a commit hook.
-
Angel Pons (Code Review) -
Carl-Daniel Hailfinger (Code Review)
-
David Hendricks (Code Review)
-
Edward O'Callaghan (Code Review)