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.
To view, visit change 38662. To unsubscribe, or for help writing mail filters, visit settings.