| Message ID | 20230519114824.12482-1-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | maintainers: add regex matching for xsm | expand |
On 19.05.2023 13:48, Daniel P. Smith wrote: > XSM is a subsystem where it is equally important of how and where its hooks are > called as is the implementation of the hooks. The people best suited for > evaluating the how and where are the XSM maintainers and reviewers. This > creates a challenge as the hooks are used throughout the hypervisor for which > the XSM maintainers and reviewers are not, and should not be, a reviewer for > each of these subsystems in the MAINTAINERS file. Though the MAINTAINERS file > does support the use of regex matches, 'K' identifier, that are applied to both > the commit message and the commit delta. Adding the 'K' identifier will declare > that any patch relating to XSM require the input from the XSM maintainers and > reviewers. For those that use the get_maintianers script, the 'K' identifier > will automatically add the XSM maintainers and reviewers. With, aiui, a fair chance of false positives when e.g. XSM hook invocations are only in patch context. Much like ... > Any one not using > get_maintainers, it will be their responsibility to ensure that if their work > touches and XSM hook, to ensure the XSM maintainers and reviewers are copied. ... manual intervention is needed in the case of not using the script, I think people should also be at least asked to see about avoiding stray Cc-s in that case. Unless of course I'm misreading get_maintainers.pl (my Perl isn't really great) or the script would be adjusted to only look at added/ removed lines (albeit even that would leave a certain risk of false positives). > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -674,6 +674,8 @@ F: tools/flask/ > F: xen/include/xsm/ > F: xen/xsm/ > F: docs/misc/xsm-flask.txt > +K: xsm_.* > +K: \b(xsm|XSM)\b Nit: Please make padding match that of adjacent lines. Jan
Hi, On 22/05/2023 10:23, Jan Beulich wrote: > On 19.05.2023 13:48, Daniel P. Smith wrote: >> XSM is a subsystem where it is equally important of how and where its hooks are >> called as is the implementation of the hooks. The people best suited for >> evaluating the how and where are the XSM maintainers and reviewers. This >> creates a challenge as the hooks are used throughout the hypervisor for which >> the XSM maintainers and reviewers are not, and should not be, a reviewer for >> each of these subsystems in the MAINTAINERS file. Though the MAINTAINERS file >> does support the use of regex matches, 'K' identifier, that are applied to both >> the commit message and the commit delta. Adding the 'K' identifier will declare >> that any patch relating to XSM require the input from the XSM maintainers and >> reviewers. For those that use the get_maintianers script, the 'K' identifier >> will automatically add the XSM maintainers and reviewers. > > With, aiui, a fair chance of false positives when e.g. XSM hook invocations > are only in patch context. Much like ... > >> Any one not using >> get_maintainers, it will be their responsibility to ensure that if their work >> touches and XSM hook, to ensure the XSM maintainers and reviewers are copied. > > ... manual intervention is needed in the case of not using the script, I > think people should also be at least asked to see about avoiding stray Cc-s > in that case. I don't particularly like this suggestion because the sender may mistakenly believe this is a "stray CC". Personally, I would prefer to be in CC more often than less often. I think we should give the choice to Daniel to decide whether he is happy to be in CC potentially more often. If it is becoming too much then we can decide to adjust the script. > Unless of course I'm misreading get_maintainers.pl (my Perl > isn't really great) or the script would be adjusted to only look at added/ > removed lines (albeit even that would leave a certain risk of false > positives). > >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -674,6 +674,8 @@ F: tools/flask/ >> F: xen/include/xsm/ >> F: xen/xsm/ >> F: docs/misc/xsm-flask.txt >> +K: xsm_.* >> +K: \b(xsm|XSM)\b Aside the NIT from Jan, this change is only affecting the number of e-mails the XSM maintainers will receive. So: Acked-by: Julien Grall <jgrall@amazon.com> Cheers,
On 5/22/23 05:23, Jan Beulich wrote: > On 19.05.2023 13:48, Daniel P. Smith wrote: >> XSM is a subsystem where it is equally important of how and where its hooks are >> called as is the implementation of the hooks. The people best suited for >> evaluating the how and where are the XSM maintainers and reviewers. This >> creates a challenge as the hooks are used throughout the hypervisor for which >> the XSM maintainers and reviewers are not, and should not be, a reviewer for >> each of these subsystems in the MAINTAINERS file. Though the MAINTAINERS file >> does support the use of regex matches, 'K' identifier, that are applied to both >> the commit message and the commit delta. Adding the 'K' identifier will declare >> that any patch relating to XSM require the input from the XSM maintainers and >> reviewers. For those that use the get_maintianers script, the 'K' identifier >> will automatically add the XSM maintainers and reviewers. > > With, aiui, a fair chance of false positives when e.g. XSM hook invocations > are only in patch context. Much like ... I was torn between matching lines with `xsm_` and matching lines with the first non-whitespace character being a `+` and having `xsm_`. In the end, I opted for the former because the concern is not just a change to the line with the XSM hook, but the changing context around the hook. As a result, yes, there will be false positives as well as the potentially false negatives as a relevant context change may happen far enough outside the diff scope. Regardless, the end result will be an increased awareness at the cost of some noise. INHO I find this to be a better situation than the current place we are at today. >> Any one not using >> get_maintainers, it will be their responsibility to ensure that if their work >> touches and XSM hook, to ensure the XSM maintainers and reviewers are copied. > > ... manual intervention is needed in the case of not using the script, I > think people should also be at least asked to see about avoiding stray Cc-s > in that case. Unless of course I'm misreading get_maintainers.pl (my Perl > isn't really great) or the script would be adjusted to only look at added/ > removed lines (albeit even that would leave a certain risk of false > positives). > >> --- a/MAINTAINERS >> +++ b/MAINTAINERS >> @@ -674,6 +674,8 @@ F: tools/flask/ >> F: xen/include/xsm/ >> F: xen/xsm/ >> F: docs/misc/xsm-flask.txt >> +K: xsm_.* >> +K: \b(xsm|XSM)\b > > Nit: Please make padding match that of adjacent lines. s Apologies, I didn't catch expandtab was on, will resubmit with hard tabs in place. v/r, dps
On 5/22/23 06:27, Julien Grall wrote: > Hi, > > On 22/05/2023 10:23, Jan Beulich wrote: >> On 19.05.2023 13:48, Daniel P. Smith wrote: >>> XSM is a subsystem where it is equally important of how and where its >>> hooks are >>> called as is the implementation of the hooks. The people best suited for >>> evaluating the how and where are the XSM maintainers and reviewers. This >>> creates a challenge as the hooks are used throughout the hypervisor >>> for which >>> the XSM maintainers and reviewers are not, and should not be, a >>> reviewer for >>> each of these subsystems in the MAINTAINERS file. Though the >>> MAINTAINERS file >>> does support the use of regex matches, 'K' identifier, that are >>> applied to both >>> the commit message and the commit delta. Adding the 'K' identifier >>> will declare >>> that any patch relating to XSM require the input from the XSM >>> maintainers and >>> reviewers. For those that use the get_maintianers script, the 'K' >>> identifier >>> will automatically add the XSM maintainers and reviewers. >> >> With, aiui, a fair chance of false positives when e.g. XSM hook >> invocations >> are only in patch context. Much like ... >> >>> Any one not using >>> get_maintainers, it will be their responsibility to ensure that if >>> their work >>> touches and XSM hook, to ensure the XSM maintainers and reviewers are >>> copied. >> >> ... manual intervention is needed in the case of not using the script, I >> think people should also be at least asked to see about avoiding stray >> Cc-s >> in that case. > > I don't particularly like this suggestion because the sender may > mistakenly believe this is a "stray CC". > > Personally, I would prefer to be in CC more often than less often. I > think we should give the choice to Daniel to decide whether he is happy > to be in CC potentially more often. > > If it is becoming too much then we can decide to adjust the script. > >> Unless of course I'm misreading get_maintainers.pl (my Perl >> isn't really great) or the script would be adjusted to only look at >> added/ >> removed lines (albeit even that would leave a certain risk of false >> positives). >> >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -674,6 +674,8 @@ F: tools/flask/ >>> F: xen/include/xsm/ >>> F: xen/xsm/ >>> F: docs/misc/xsm-flask.txt >>> +K: xsm_.* >>> +K: \b(xsm|XSM)\b > > Aside the NIT from Jan, this change is only affecting the number of > e-mails the XSM maintainers will receive. So: > > Acked-by: Julien Grall <jgrall@amazon.com> Apologies, I missed your ack before I sent v2 out. v/r, dps
diff --git a/MAINTAINERS b/MAINTAINERS index f2f1881b32..f7c8961dbb 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -674,6 +674,8 @@ F: tools/flask/ F: xen/include/xsm/ F: xen/xsm/ F: docs/misc/xsm-flask.txt +K: xsm_.* +K: \b(xsm|XSM)\b THE REST M: Andrew Cooper <andrew.cooper3@citrix.com>
XSM is a subsystem where it is equally important of how and where its hooks are called as is the implementation of the hooks. The people best suited for evaluating the how and where are the XSM maintainers and reviewers. This creates a challenge as the hooks are used throughout the hypervisor for which the XSM maintainers and reviewers are not, and should not be, a reviewer for each of these subsystems in the MAINTAINERS file. Though the MAINTAINERS file does support the use of regex matches, 'K' identifier, that are applied to both the commit message and the commit delta. Adding the 'K' identifier will declare that any patch relating to XSM require the input from the XSM maintainers and reviewers. For those that use the get_maintianers script, the 'K' identifier will automatically add the XSM maintainers and reviewers. Any one not using get_maintainers, it will be their responsibility to ensure that if their work touches and XSM hook, to ensure the XSM maintainers and reviewers are copied. This patch adds a pair of regex expressions to the XSM section. The first is `xsm_.*` which seeks to match XSM hooks in the commit's delta. The second is `\b(xsm|XSM)\b` which seeks to match strictly the words xsm or XSM and should not capture words with a substring of "xsm". Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+)