On Fri, Oct 28, 2022 at 8:49 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > On Fri, Oct 28, 2022, Andrew Jones wrote: > > On Thu, Oct 27, 2022 at 06:27:39PM +0000, Sean Christopherson wrote: > > > On Thu, Oct 27, 2022, David Matlack wrote: > > > > On Thu, Oct 27, 2022 at 8:44 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote: > > > > > I like the idea in theory, but that'd be a daunting task to set up, and quite the > > > > > maintenance nightmare. There are probably thousands of file => scope mappings > > > > > throughout the kernel, with any number of exceptions and arbitrary rules. > > > > > > > > I was thinking about proposing this in checkpatch.pl, or in some > > > > KVM-specific check script. It seems like the following rule: If a > > > > commit only modifies files in tools/testing/selftests/kvm/*, then > > > > requires the shortlog match the regex "KVM: selftests: .*". That would > > > > handle the vast majority of cases without affecting other subsystems. > > > > > > > > Sean are you more concerned that if we start validating shortlogs in > > > > checkpatch.pl then eventually it will get too out of hand? (i.e. not > > > > so concerned with this specific case, but the general problem?) > > > > > > Ya, the general problem. Hardcoding anything KVM specific in checkpatch.pl isn't > > > going to fly. The checkpatch maintainers most definitely don't want to take on > > > the burden of maintaining subsystem rules. Letting one subsystem add custom rules > > > effectively opens the flood gates to all subsystems adding custom rules. And from > > > a KVM perspective, I don't want to have to get an Acked-by from a checkpatch > > > maintiainer just to tweak a KVM rule. > > > > > > The only somewhat feasible approach I can think of would be to provide a generic > > > "language" for shortlog scope rules, and have checkpatch look for a well-known > > > file in relevant directories, e.g. add arch/x86/kvm/SCOPES or whatever. But even > > > that is a non-trivial problem to solve, as it means coming up with a "language" > > > that has a reasonable chance of working for many subsystems without generating too > > > many false positives. > > > > > > It's definitely doable, and likely not actually a maintenance nightmare (I wrote > > > that thinking of modifying a common rules file). But it's still fairly daunting > > > as getting buy-in on something that affects the kernel at large tends to be easier > > > said then done. Then again, I'm probably being pessimistic due to my sub-par > > > regex+scripting skills :-) > > > > How about adding support for checkpatch extension plugins? If we could add > > a plugin script, e.g. tools/testing/selftests/kvm/.checkpatch, and modify > > checkpatch to run .checkpatch scripts in the patched files' directories > > (and recursively in the parent directories) when found, then we'd get > > custom checkpatch behaviors. The scripts wouldn't even have to be written > > in Perl (but I say that a bit sadly, because I like Perl). > > That will work for simple cases, but patches that touch files in multiple directories > will be messy. E.g. a patch that touches virt/kvm/ and arch/x86/kvm/ will have > two separate custom rules enforcing two different scopes. > > Recursively executing plugins will also be problematic, e.g. except for KVM, arch/x86/ > is maintained by the tip-tree folks, and the tip-tree is quite opinionated on all > sorts of things, whereas KVM tends to be a bit more relaxed. > > Enforcing scope through plugins would also lead to some amount of duplicate code > throught subsystems. > > Anyways, if someone wants to pursue this, these ideas and the "requirement" should > be run by the checkpatch maintainers. They have far more experience and authority > in this area, and I suspect we aren't the first people to want checkpatch to get > involved in enforcing shortlog scope. Documenting would at least be an improvement over what we have today since it would eliminate the need to re-explain the preferred rules every time. We can just point to the documentation when reviewing patches. `git log --pretty=oneline` is not a great way to document shortlog scopes because it does not explain the rules (e.g. when to use "KVM: x86: " vs "KVM: x86/mmu: "), does not explain why things the way they are, and is inconsistent since we don't always catch every patch that goes by with a non-preferred shortlog scope.
