On 2019-04-23 21:09:30 +0000 (+0000), Fox, Kevin M wrote:
Yeah, pushing a patch following the patch going in for review is more what I was thinking. So, a patch that fixes the previous patch, not actually rewriting the patch.
Sure, that's essentially another of the options I mentioned in an earlier message:
There is also the possibility that you simply run a periodic autopep8 job against the codebase and have that job propose a commit to the repository to apply code style changes if there are any, but that's a bit of a messy hack as well.
The main reasons I consider it a "messy hack" is that it runs the potential of increasing the number of commits in the repository significantly depending on whether and how often you batch up the autofixes, and makes it harder to `git blame` effectively since a lot of changes are immediately followed by changes to those same lines by a generic automation account. It also means we give up on whatever benefits we might hope to gain from having consistency of style in proposed changes, as folks can propose code in any old format they like without regard for readability and it only gets cleaned up and made readable after it's been reviewed and approved. This strikes me as a slippery slope towards "it's okay if your code has obvious errors, we can find and fix them after it's merged."
Is it a problem for the commit to "come from" and be "signed off" by the automation itself?
No, we have automation propose other sorts of changes already. From a legal standpoint nobody's ever complained that the machine hasn't agreed to the ICLA or acknowledged the DCO. I think once we're successful enough with artificial intelligence for our computers to start filing patent infringement claims against us for the work they've written, we can revisit that position. If we wanted to have our existing auto-proposed changes use `git commit --gpg-sign` I think that would be able to take advantage of much of the same automated signing infrastructure we already leverage for our Git tags and related release artifacts. That much seems to me like it could be a good idea, if someone wants to do the work to add it. It would in theory allow any observer to confirm that the patches which claim to have originated within our automation actually have.
It surfaces the cleanup patches as just cleanup patches. And in the very end of the ps, we squash the pr on merge anyway? If the user wants, they can also just flatten the PS themselves and it would automatically merge in the automation provided patches. [...]
Now it's starting to sound like you're suggesting auto-proposing follow-up changes to each user-proposed change while it's still under review. I think it's probably technically possible, though likely to ~double our overall change volume insofar as review and testing go (especially when folks inevitably ignore them and so they get rebased and updated every time the parent change is updated). And before you say "it's okay to skip running CI jobs on those" it's really not, since we're not likely to blindly trust autopep8 not to propose a subtly broken modification. -- Jeremy Stanley