On Thu, Oct 27, 2022, Wang, Wei W wrote: > On Thursday, October 27, 2022 5:23 AM, David Matlack: > > I haven't dug too much into the actual code yet, but I have some high level > > feedback based on a quick look through the series: > > > > - Use the format "KVM: selftests: <Decsription>" for the shortlog. > > I know it's not common to see so far, but curious is this the required format? It's definitely the preferred format. > I didn't find where it's documented. Heh, for all shortlog scopes, the "documentation" is `git log --pretty=oneline` :-) > If it's indeed a requirement, probably we also need to enhance checkpatch.pl > to detect this. 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. > If it's not required, I think it is more obvious to have /sub_field in the title, > e.g. selftests/hardware_disable_test, to outline which specific part of > selftests the patch is changing. (the selftests are growing larger with many > usages independent of each other). I agree that "KVM: selftests:" is a rather large umbrella, but it's not obvious that "KVM: selfetest/<test>" is unequivocally better, e.g. if someone is making a change to x86_64/vmx_exception_with_invalid_guest_state.c, the scope will be KVM: selftests/x86_64/vmx_exception_with_invalid_guest_state: or KVM: selftests/vmx_exception_with_invalid_guest_state: which doesn't leave a whole lot of room for an actual shortlog. When reviewing selftests patches, I do find myself pausing sometimes to see exactly what file/test is being modified, but in almost all cases a quick glance at the diffstat provides the answer. And on the flip side, having all selftests patches exactly match "KVM: selftests:" makes it super easy to identify selftest changes, i.e. it's mostly a wash overall in terms of efficiency (for me at least).
