Attention is currently required from: Felix Singer, Nico Huber, Edward O'Callaghan, Anastasia Klimchuk, Nikolai Artemiev.
Patch set 2:Code-Review +1
4 comments:
File ichspi.c:
Patch Set #1, Line 1836: arg);
Another unnecessary line break.
This is addressed in the follow-up commit, can we leave this as-is in this commit?
Drop line break?
yeah, the line break before `);` looks really odd.
File ichspi.c:
Patch Set #2, Line 1822: arg = extract_programmer_param("ich_spi_mode");
I use a very stupid trick to minimise diffstat noise when reindenting blocks of code during refactoring: I change the indentation in a separate, reproducible commit, so that the diffstat of the non-reproducible part is much smaller.
I've done this several times when refactoring coreboot code. For example, I wanted to refactor some chipset code, but all mainboards would need to be adapted. I started with CB:46702 CB:46703 CB:46704 CB:46705 to do nearly all changes to mainboard code reproducibly. I also had to make CB:46769 to ensure things would work properly, and CB:46700 did the actual refactoring.
Another example of how I break down refactoring changes to hopefully ease review: CB:49398 changes behavior but is very small, CB:49399 is not reproducible but doesn't change behavior, and CB:49400 is quite large but reproducible.
There's no need to change this commit, it's just that it reminded me of similar situations I've encountered, and I wanted to describe an approach I came up with. However, if you think this idea might make this change easier to review, feel free to use it. 😊
I'd normally complain about unrelated whitespace changes (this seems accidental), but addressing this would mean having to manually rebase the follow-up change as well. If you don't mind, I'd appreciate if you could undo this.
To view, visit change 58735. To unsubscribe, or for help writing mail filters, visit settings.