Re: [RFC PATCH v2 02/10] MAINTAINERS: Introduce V: entry for tests
On 12/6/23 18:38, Joe Perches wrote: On Wed, 2023-12-06 at 18:23 +0200, Nikolai Kondrashov wrote: On 12/6/23 10:12, David Gow wrote: I'm pretty happy with this personally, though I definitely think we need the support for tests which aren't just executable scripts (e.g. the docs in patch 6). The get_maintailer.pl bits, and hence the requirement to not include '@', feel a little bit 'off': I'd rather get_maintainer.pl kept emails and tests separate by some other means (either having --test _only_ print tests, not emails at all, or by giving them a prefix like 'TEST:' or something). But that is diverging more from the existing behaviour of get_maintainer.pl, so I could go either way. Otherwise, this looks pretty good. I'll give it a proper test tomorrow alongside the other patches. Thanks for the review, David! Yeah, I don't like the '@' bit myself, but it seems to be the path of least resistance right now (not necessarily the best one, of course). I'm up for adding an option to get_maintainer.pl that disables email output, if people like that, though. That already exists though I don't understand the specific requirement here --nom --nol --nor from $ ./scripts/get_maintainer.pl --help [] --m => include maintainer(s) if any --r => include reviewer(s) if any --n => include name 'Full Name ' --l => include list(s) if any [] Most options have both positive and negative forms. The negative forms for -- are --no and --no-. Thanks, Joe! Yeah, I already explored that way, but it seems to be explicitly forbidden: $ scripts/get_maintainer.pl --nom --nol --nor 0001-dt-bindings-mailbox-convert-bcm2835-mbox-bindings-to.patch scripts/get_maintainer.pl: Please select at least 1 email option So, I assumed there is a reason and an intention behind this behavior and went the other way. Nick
Re: [RFC PATCH v2 02/10] MAINTAINERS: Introduce V: entry for tests
On Wed, 2023-12-06 at 18:23 +0200, Nikolai Kondrashov wrote: > On 12/6/23 10:12, David Gow wrote: > > I'm pretty happy with this personally, though I definitely think we > > need the support for tests which aren't just executable scripts (e.g. > > the docs in patch 6). > > > > The get_maintailer.pl bits, and hence the requirement to not include > > '@', feel a little bit 'off': I'd rather get_maintainer.pl kept emails > > and tests separate by some other means (either having --test _only_ > > print tests, not emails at all, or by giving them a prefix like > > 'TEST:' or something). But that is diverging more from the existing > > behaviour of get_maintainer.pl, so I could go either way. > > > > Otherwise, this looks pretty good. I'll give it a proper test tomorrow > > alongside the other patches. > > Thanks for the review, David! > > Yeah, I don't like the '@' bit myself, but it seems to be the path of least > resistance right now (not necessarily the best one, of course). > > I'm up for adding an option to get_maintainer.pl that disables email output, > if people like that, though. That already exists though I don't understand the specific requirement here --nom --nol --nor from $ ./scripts/get_maintainer.pl --help [] --m => include maintainer(s) if any --r => include reviewer(s) if any --n => include name 'Full Name ' --l => include list(s) if any [] Most options have both positive and negative forms. The negative forms for -- are --no and --no-.
Re: [RFC PATCH v2 02/10] MAINTAINERS: Introduce V: entry for tests
On 12/6/23 10:12, David Gow wrote: I'm pretty happy with this personally, though I definitely think we need the support for tests which aren't just executable scripts (e.g. the docs in patch 6). The get_maintailer.pl bits, and hence the requirement to not include '@', feel a little bit 'off': I'd rather get_maintainer.pl kept emails and tests separate by some other means (either having --test _only_ print tests, not emails at all, or by giving them a prefix like 'TEST:' or something). But that is diverging more from the existing behaviour of get_maintainer.pl, so I could go either way. Otherwise, this looks pretty good. I'll give it a proper test tomorrow alongside the other patches. Thanks for the review, David! Yeah, I don't like the '@' bit myself, but it seems to be the path of least resistance right now (not necessarily the best one, of course). I'm up for adding an option to get_maintainer.pl that disables email output, if people like that, though. Nick
Re: [RFC PATCH v2 02/10] MAINTAINERS: Introduce V: entry for tests
On 12/5/23 20:58, Joe Perches wrote: On Tue, 2023-12-05 at 20:02 +0200, Nikolai Kondrashov wrote: Require the entry values to not contain the '@' character, so they could be distinguished from emails (always) output by get_maintainer.pl. Why is this useful? Why the need to distinguish? This is because get_maintainer.pl seems to insist on *always* outputting emails. I guess that was its sole original purpose, and it stuck to it? It kinda makes sense to me, especially given the name of the script, but at the same time, as a consequence you can't query *only* the tests (or anything but emails, really). Therefore we have to be able to somehow filter out the emails from the get_maintainer.pl output, to get only the tests. Email addresses *have* to have the '@' character (seems to be the only reliable thing about them :D), so naturally I chose that as the way to distinguish them from the tests. It's ugly, but considering the get_maintainer.pl legacy, it's good enough. I don't mind changing get_maintainer.pl, though, to stop it from outputting emails (e.g. given an option), if that works for everyone involved. Nick
Re: [RFC PATCH v2 02/10] MAINTAINERS: Introduce V: entry for tests
On Wed, 6 Dec 2023 at 02:45, Nikolai Kondrashov
wrote:
>
> Introduce a new 'V:' ("Verify") entry to MAINTAINERS. The entry accepts
> a test suite command which is proposed to be executed for each
> contribution to the subsystem.
>
> Extend scripts/get_maintainer.pl to support retrieving the V: entries
> when '--test' option is specified.
>
> Require the entry values to not contain the '@' character, so they could
> be distinguished from emails (always) output by get_maintainer.pl. Make
> scripts/checkpatch.pl check that they don't.
>
> Update entry ordering in both scripts/checkpatch.pl and
> scripts/parse-maintainers.pl.
>
> Signed-off-by: Nikolai Kondrashov
> ---
I'm pretty happy with this personally, though I definitely think we
need the support for tests which aren't just executable scripts (e.g.
the docs in patch 6).
The get_maintailer.pl bits, and hence the requirement to not include
'@', feel a little bit 'off': I'd rather get_maintainer.pl kept emails
and tests separate by some other means (either having --test _only_
print tests, not emails at all, or by giving them a prefix like
'TEST:' or something). But that is diverging more from the existing
behaviour of get_maintainer.pl, so I could go either way.
Otherwise, this looks pretty good. I'll give it a proper test tomorrow
alongside the other patches.
Cheers,
-- David
smime.p7s
Description: S/MIME Cryptographic Signature
Re: [RFC PATCH v2 02/10] MAINTAINERS: Introduce V: entry for tests
On Tue, 2023-12-05 at 20:02 +0200, Nikolai Kondrashov wrote: > Require the entry values to not contain the '@' character, so they could > be distinguished from emails (always) output by get_maintainer.pl. Why is this useful? Why the need to distinguish?
