user/dev discussion of public-inbox itself
 help / color / Atom feed
* [RFC PATCH] watchmaildir: support multiple watchheader values
@ 2020-04-12  4:44 Kyle Meyer
  2020-04-12 21:59 ` Eric Wong
  0 siblings, 1 reply; 8+ messages in thread
From: Kyle Meyer @ 2020-04-12  4:44 UTC (permalink / raw)
  To: meta; +Cc: Kyle Meyer

The watchheader key supports only a single value.  In discussion [1]
of 8d3e3bd8 (doc: explain publicinbox.<name>.watchheader, 2019-10-09),
two concerns were raised about supporting multiple watchheader values:

  * doing so would invite confusion about whether the acceptance
    condition is "all" or "any"

  * it's not clear there's a need

The second point seems to be the crucial one because, provided there
is a need, the first can be addressed with documentation (as already
done for listid, which accepts the message if there are any matches).

Here is one scenario where support for matching multiple headers would
be convenient.  Someone wants to set up public-inbox archives for some
small projects but does _not_ want to run mailing lists for them,
instead allowing others to follow the project by any of the pull
mechanisms.  Using a common underlying address, an address alias for
each project is set up via a third-party email provider, with messages
for each alias being exposed as a separate public-inbox archive.

In this scenario, if all messages are downloaded to a common maildir,
the problem with using watchheader to match the project address is
that the address may be in either the To: or Cc: header.  One way to
avoid this issue would be to filter things upstream so that messages
for different projects got to different maildirs, ideally in a way
that duplicates any cross-posted messages in each maildir.

Teach watchheader to support multiple values, avoiding the need to
direct message with no List-ID to separate maildirs.

[1] https://public-inbox.org/meta/20191010085118.r3amey4cayazfycb@dcvr/
---

  I'm not entirely convinced that the above provides a compelling
  reason to make this change, but I think it's worth considering if it
  makes it easier for some to set up archives for their projects
  without setting up mailings lists.  Thoughts?

  (If this ends up being a direction we decide to go, I'd be happy to
  work on adding tests to this patch.)

 Documentation/public-inbox-config.pod | 3 ++-
 lib/PublicInbox/Config.pm             | 4 ++--
 lib/PublicInbox/WatchMaildir.pm       | 8 +++++---
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/Documentation/public-inbox-config.pod b/Documentation/public-inbox-config.pod
index 53926ef4..f9871d4b 100644
--- a/Documentation/public-inbox-config.pod
+++ b/Documentation/public-inbox-config.pod
@@ -88,7 +88,8 @@ Default: none; only for L<public-inbox-watch(1)> users
 		watchheader = List-Id:<test.example.com>
 
 If specified, L<public-inbox-watch(1)> will only process mail matching
-the given header.  Multiple values are not currently supported.
+the given header.  If specified multiple times, mail will be processed
+if it matches any of the values.
 
 Default: none; only for L<public-inbox-watch(1)> users
 
diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm
index 917939ca..458f29b2 100644
--- a/lib/PublicInbox/Config.pm
+++ b/lib/PublicInbox/Config.pm
@@ -367,7 +367,7 @@ sub _fill {
 	my $ibx = {};
 
 	foreach my $k (qw(inboxdir filter newsgroup
-			watch watchheader httpbackendmax
+			watch httpbackendmax
 			replyto feedmax nntpserver indexlevel)) {
 		my $v = $self->{"$pfx.$k"};
 		$ibx->{$k} = $v if defined $v;
@@ -388,7 +388,7 @@ sub _fill {
 	# TODO: more arrays, we should support multi-value for
 	# more things to encourage decentralization
 	foreach my $k (qw(address altid nntpmirror coderepo hide listid url
-			infourl)) {
+			infourl watchheader)) {
 		if (defined(my $v = $self->{"$pfx.$k"})) {
 			$ibx->{$k} = _array($v);
 		}
diff --git a/lib/PublicInbox/WatchMaildir.pm b/lib/PublicInbox/WatchMaildir.pm
index e2024640..bdaacf62 100644
--- a/lib/PublicInbox/WatchMaildir.pm
+++ b/lib/PublicInbox/WatchMaildir.pm
@@ -59,9 +59,11 @@ sub new {
 		my $watch = $ibx->{watch} or return;
 		if (is_maildir($watch)) {
 			my $watch_hdrs = [];
-			if (my $wh = $ibx->{watchheader}) {
-				my ($k, $v) = split(/:/, $wh, 2);
-				push @$watch_hdrs, [ $k, qr/\Q$v\E/ ];
+			if (my $whs = $ibx->{watchheader}) {
+				for (@$whs) {
+					my ($k, $v) = split(/:/, $_, 2);
+					push @$watch_hdrs, [ $k, qr/\Q$v\E/ ];
+				}
 			}
 			if (my $list_ids = $ibx->{listid}) {
 				for (@$list_ids) {

base-commit: d7270ec79e6347eb1c35ebcdc197e887bb4ef075
-- 
2.26.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [RFC PATCH] watchmaildir: support multiple watchheader values
  2020-04-12  4:44 [RFC PATCH] watchmaildir: support multiple watchheader values Kyle Meyer
@ 2020-04-12 21:59 ` Eric Wong
  2020-04-20  0:13   ` [PATCH v2] " Kyle Meyer
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Wong @ 2020-04-12 21:59 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: meta

Kyle Meyer <kyle@kyleam.com> wrote:
> The watchheader key supports only a single value.  In discussion [1]
> of 8d3e3bd8 (doc: explain publicinbox.<name>.watchheader, 2019-10-09),
> two concerns were raised about supporting multiple watchheader values:
> 
>   * doing so would invite confusion about whether the acceptance
>     condition is "all" or "any"
> 
>   * it's not clear there's a need
> 
> The second point seems to be the crucial one because, provided there
> is a need, the first can be addressed with documentation (as already
> done for listid, which accepts the message if there are any matches).
> 
> Here is one scenario where support for matching multiple headers would
> be convenient.  Someone wants to set up public-inbox archives for some
> small projects but does _not_ want to run mailing lists for them,
> instead allowing others to follow the project by any of the pull
> mechanisms.  Using a common underlying address, an address alias for
> each project is set up via a third-party email provider, with messages
> for each alias being exposed as a separate public-inbox archive.

Yeah.  This seems like it could be useful for organizing
personal mail.

> In this scenario, if all messages are downloaded to a common maildir,
> the problem with using watchheader to match the project address is
> that the address may be in either the To: or Cc: header.  One way to
> avoid this issue would be to filter things upstream so that messages
> for different projects got to different maildirs, ideally in a way
> that duplicates any cross-posted messages in each maildir.
> 
> Teach watchheader to support multiple values, avoiding the need to
> direct message with no List-ID to separate maildirs.
> 
> [1] https://public-inbox.org/meta/20191010085118.r3amey4cayazfycb@dcvr/
> ---
> 
>   I'm not entirely convinced that the above provides a compelling
>   reason to make this change, but I think it's worth considering if it
>   makes it easier for some to set up archives for their projects
>   without setting up mailings lists.  Thoughts?

Seems reasonable for the 1.5 release (hoping to have 1.4 out soon).
I think the "any" match is reasonable considering it's analogous
to the behavior of the current `address' matching.

>   (If this ends up being a direction we decide to go, I'd be happy to
>   work on adding tests to this patch.)

Yes, thanks in advance :)

> diff --git a/Documentation/public-inbox-config.pod b/Documentation/public-inbox-config.pod
> index 53926ef4..f9871d4b 100644
> --- a/Documentation/public-inbox-config.pod
> +++ b/Documentation/public-inbox-config.pod
> @@ -88,7 +88,8 @@ Default: none; only for L<public-inbox-watch(1)> users
>  		watchheader = List-Id:<test.example.com>
>  
>  If specified, L<public-inbox-watch(1)> will only process mail matching
> -the given header.  Multiple values are not currently supported.
> +the given header.  If specified multiple times, mail will be processed
> +if it matches any of the values.
>  
>  Default: none; only for L<public-inbox-watch(1)> users
>  
> diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm
> index 917939ca..458f29b2 100644
> --- a/lib/PublicInbox/Config.pm
> +++ b/lib/PublicInbox/Config.pm
> @@ -367,7 +367,7 @@ sub _fill {
>  	my $ibx = {};
>  
>  	foreach my $k (qw(inboxdir filter newsgroup
> -			watch watchheader httpbackendmax
> +			watch httpbackendmax
>  			replyto feedmax nntpserver indexlevel)) {
>  		my $v = $self->{"$pfx.$k"};
>  		$ibx->{$k} = $v if defined $v;
> @@ -388,7 +388,7 @@ sub _fill {
>  	# TODO: more arrays, we should support multi-value for
>  	# more things to encourage decentralization
>  	foreach my $k (qw(address altid nntpmirror coderepo hide listid url
> -			infourl)) {
> +			infourl watchheader)) {
>  		if (defined(my $v = $self->{"$pfx.$k"})) {
>  			$ibx->{$k} = _array($v);
>  		}
> diff --git a/lib/PublicInbox/WatchMaildir.pm b/lib/PublicInbox/WatchMaildir.pm
> index e2024640..bdaacf62 100644
> --- a/lib/PublicInbox/WatchMaildir.pm
> +++ b/lib/PublicInbox/WatchMaildir.pm
> @@ -59,9 +59,11 @@ sub new {
>  		my $watch = $ibx->{watch} or return;
>  		if (is_maildir($watch)) {
>  			my $watch_hdrs = [];
> -			if (my $wh = $ibx->{watchheader}) {
> -				my ($k, $v) = split(/:/, $wh, 2);
> -				push @$watch_hdrs, [ $k, qr/\Q$v\E/ ];
> +			if (my $whs = $ibx->{watchheader}) {
> +				for (@$whs) {
> +					my ($k, $v) = split(/:/, $_, 2);
> +					push @$watch_hdrs, [ $k, qr/\Q$v\E/ ];
> +				}
>  			}
>  			if (my $list_ids = $ibx->{listid}) {
>  				for (@$list_ids) {

The change itself looks fine to my somewhat sleepy eyes.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v2] watchmaildir: support multiple watchheader values
  2020-04-12 21:59 ` Eric Wong
@ 2020-04-20  0:13   ` Kyle Meyer
  2020-04-20  0:45     ` Eric Wong
  0 siblings, 1 reply; 8+ messages in thread
From: Kyle Meyer @ 2020-04-20  0:13 UTC (permalink / raw)
  To: Eric Wong; +Cc: Kyle Meyer, meta

The watchheader key supports only a single value.  Supporting multiple
watchheader values was mentioned in discussion [1] of 8d3e3bd8 (doc:
explain publicinbox.<name>.watchheader, 2019-10-09), and it wasn't
clear if there was a need.

One scenario in which matching multiple headers would be convenient is
when someone wants to set up public-inbox archives for some small
projects but does _not_ want to run mailing lists for them, instead
allowing others to follow the project by any of the pull mechanisms.
Using a common underlying address, an address alias for each project
is configured via a third-party email provider, with messages for each
alias being exposed as a separate public-inbox archive.  In this
setup, messages for an inbox cannot be selected by a List-ID header
but can be identified by the inbox's address in either the To or Cc
header.

To support such a use case, update the watchheader handling to
consider multiple values, accepting a message if it matches any value.
While selecting a message based on matching _any_ rather than _all_
values is motivated by the above scenario, it's worth noting that the
"any" behavior is consistent with how multiple listid config values
are handled.

[1] https://public-inbox.org/meta/20191010085118.r3amey4cayazfycb@dcvr/
---

   Changes since v1:

   * Add a test for matching multiple watchheader values.
   * Adjust WwwText::inbox_config to handle watchheader as an array.
   * Condense commit message.

 Documentation/public-inbox-config.pod |  3 +-
 MANIFEST                              |  1 +
 lib/PublicInbox/Config.pm             |  4 +-
 lib/PublicInbox/WatchMaildir.pm       |  8 +--
 lib/PublicInbox/WwwText.pm            |  4 +-
 t/watch_muliple_headers.t             | 76 +++++++++++++++++++++++++++
 6 files changed, 88 insertions(+), 8 deletions(-)
 create mode 100644 t/watch_muliple_headers.t

diff --git a/Documentation/public-inbox-config.pod b/Documentation/public-inbox-config.pod
index 4c9994dc..708a785f 100644
--- a/Documentation/public-inbox-config.pod
+++ b/Documentation/public-inbox-config.pod
@@ -88,7 +88,8 @@ Default: none; only for L<public-inbox-watch(1)> users
 		watchheader = List-Id:<test.example.com>
 
 If specified, L<public-inbox-watch(1)> will only process mail matching
-the given header.  Multiple values are not currently supported.
+the given header.  If specified multiple times, mail will be processed
+if it matches any of the values.
 
 Default: none; only for L<public-inbox-watch(1)> users
 
diff --git a/MANIFEST b/MANIFEST
index 8b724352..bfcf26b4 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -307,6 +307,7 @@ t/view.t
 t/watch_filter_rubylang.t
 t/watch_maildir.t
 t/watch_maildir_v2.t
+t/watch_muliple_headers.t
 t/www_altid.t
 t/www_listing.t
 t/www_static.t
diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm
index 917939ca..458f29b2 100644
--- a/lib/PublicInbox/Config.pm
+++ b/lib/PublicInbox/Config.pm
@@ -367,7 +367,7 @@ sub _fill {
 	my $ibx = {};
 
 	foreach my $k (qw(inboxdir filter newsgroup
-			watch watchheader httpbackendmax
+			watch httpbackendmax
 			replyto feedmax nntpserver indexlevel)) {
 		my $v = $self->{"$pfx.$k"};
 		$ibx->{$k} = $v if defined $v;
@@ -388,7 +388,7 @@ sub _fill {
 	# TODO: more arrays, we should support multi-value for
 	# more things to encourage decentralization
 	foreach my $k (qw(address altid nntpmirror coderepo hide listid url
-			infourl)) {
+			infourl watchheader)) {
 		if (defined(my $v = $self->{"$pfx.$k"})) {
 			$ibx->{$k} = _array($v);
 		}
diff --git a/lib/PublicInbox/WatchMaildir.pm b/lib/PublicInbox/WatchMaildir.pm
index bea2ed2a..7b9e8915 100644
--- a/lib/PublicInbox/WatchMaildir.pm
+++ b/lib/PublicInbox/WatchMaildir.pm
@@ -59,9 +59,11 @@ sub new {
 		my $watch = $ibx->{watch} or return;
 		if (is_maildir($watch)) {
 			my $watch_hdrs = [];
-			if (my $wh = $ibx->{watchheader}) {
-				my ($k, $v) = split(/:/, $wh, 2);
-				push @$watch_hdrs, [ $k, qr/\Q$v\E/ ];
+			if (my $whs = $ibx->{watchheader}) {
+				for (@$whs) {
+					my ($k, $v) = split(/:/, $_, 2);
+					push @$watch_hdrs, [ $k, qr/\Q$v\E/ ];
+				}
 			}
 			if (my $list_ids = $ibx->{listid}) {
 				for (@$list_ids) {
diff --git a/lib/PublicInbox/WwwText.pm b/lib/PublicInbox/WwwText.pm
index 2008ba09..b23a415e 100644
--- a/lib/PublicInbox/WwwText.pm
+++ b/lib/PublicInbox/WwwText.pm
@@ -151,7 +151,7 @@ sub inbox_config ($$$) {
 	url = https://example.com/$name/
 	url = http://example.onion/$name/
 EOS
-	for my $k (qw(address listid infourl)) {
+	for my $k (qw(address listid infourl watchheader)) {
 		defined(my $v = $ibx->{$k}) or next;
 		$$txt .= "\t$k = $_\n" for @$v;
 	}
@@ -171,7 +171,7 @@ EOF
 		}
 	}
 
-	for my $k (qw(filter newsgroup obfuscate replyto watchheader)) {
+	for my $k (qw(filter newsgroup obfuscate replyto)) {
 		defined(my $v = $ibx->{$k}) or next;
 		$$txt .= "\t$k = $v\n";
 	}
diff --git a/t/watch_muliple_headers.t b/t/watch_muliple_headers.t
new file mode 100644
index 00000000..8ea4b306
--- /dev/null
+++ b/t/watch_muliple_headers.t
@@ -0,0 +1,76 @@
+# Copyright (C) 2020 all contributors <meta@public-inbox.org>
+# License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
+use strict;
+use Test::More;
+use PublicInbox::Config;
+use PublicInbox::TestCommon;
+require_git(2.6);
+require_mods(qw(Search::Xapian DBD::SQLite Filesys::Notify::Simple));
+my ($tmpdir, $for_destroy) = tmpdir();
+my $inboxdir = "$tmpdir/v2";
+my $maildir = "$tmpdir/md";
+use_ok 'PublicInbox::WatchMaildir';
+use_ok 'PublicInbox::Emergency';
+my $cfgpfx = "publicinbox.test";
+my $addr = 'test-public@example.com';
+my @cmd = ('-init', '-V2', 'test', $inboxdir,
+	'http://example.com/list', $addr);
+local $ENV{PI_CONFIG} = "$tmpdir/pi_config";
+ok(run_script(\@cmd), 'public-inbox init OK');
+
+my $msg_to = <<EOF;
+From: user\@a.com
+To: $addr
+Subject: address is in to
+Message-Id: <to\@a.com>
+Date: Sat, 18 Apr 2020 00:00:00 +0000
+
+content1
+EOF
+
+my $msg_cc = <<EOF;
+From: user1\@a.com
+To: user2\@a.com
+Cc: $addr
+Subject: address is in cc
+Message-Id: <cc\@a.com>
+Date: Sat, 18 Apr 2020 00:01:00 +0000
+
+content2
+EOF
+
+my $msg_none = <<EOF;
+From: user1\@a.com
+To: user2\@a.com
+Cc: user3\@a.com
+Subject: address is not in to or cc
+Message-Id: <none\@a.com>
+Date: Sat, 18 Apr 2020 00:02:00 +0000
+
+content3
+EOF
+
+PublicInbox::Emergency->new($maildir)->prepare(\$msg_to);
+PublicInbox::Emergency->new($maildir)->prepare(\$msg_cc);
+PublicInbox::Emergency->new($maildir)->prepare(\$msg_none);
+
+my $cfg = <<EOF;
+$cfgpfx.address=$addr
+$cfgpfx.inboxdir=$inboxdir
+$cfgpfx.watch=maildir:$maildir
+$cfgpfx.watchheader=To:$addr
+$cfgpfx.watchheader=Cc:$addr
+EOF
+my $config = PublicInbox::Config->new(\$cfg);
+PublicInbox::WatchMaildir->new($config)->scan('full');
+my $ibx = $config->lookup_name('test');
+ok($ibx, 'found inbox by name');
+
+my $num = $ibx->mm->num_for('to@a.com');
+ok(defined $num, 'Matched for address in To:');
+my $num = $ibx->mm->num_for('cc@a.com');
+ok(defined $num, 'Matched for address in Cc:');
+$num = $ibx->mm->num_for('none@a.com');
+is($num, undef, 'No match without address in To: or Cc:');
+
+done_testing;

base-commit: b3f81ce0c71d5d4eca347f259b5ae69660a2cb13
-- 
2.26.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] watchmaildir: support multiple watchheader values
  2020-04-20  0:13   ` [PATCH v2] " Kyle Meyer
@ 2020-04-20  0:45     ` Eric Wong
  2020-04-20  1:13       ` Kyle Meyer
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Wong @ 2020-04-20  0:45 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: meta

Kyle Meyer <kyle@kyleam.com> wrote:

<snip> Thanks.  Most everything looks good, some minor issues..

> +++ b/t/watch_muliple_headers.t

speling :>

> +my $num = $ibx->mm->num_for('to@a.com');
> +ok(defined $num, 'Matched for address in To:');
> +my $num = $ibx->mm->num_for('cc@a.com');

That `$num' shadows/masks

Will squash the following in before pushing:

diff --git a/MANIFEST b/MANIFEST
index bfcf26b4..b06aa679 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -307,7 +307,7 @@ t/view.t
 t/watch_filter_rubylang.t
 t/watch_maildir.t
 t/watch_maildir_v2.t
-t/watch_muliple_headers.t
+t/watch_multiple_headers.t
 t/www_altid.t
 t/www_listing.t
 t/www_static.t
diff --git a/t/watch_muliple_headers.t b/t/watch_multiple_headers.t
similarity index 97%
rename from t/watch_muliple_headers.t
rename to t/watch_multiple_headers.t
index 8ea4b306..3a39eba9 100644
--- a/t/watch_muliple_headers.t
+++ b/t/watch_multiple_headers.t
@@ -68,7 +68,7 @@ ok($ibx, 'found inbox by name');
 
 my $num = $ibx->mm->num_for('to@a.com');
 ok(defined $num, 'Matched for address in To:');
-my $num = $ibx->mm->num_for('cc@a.com');
+$num = $ibx->mm->num_for('cc@a.com');
 ok(defined $num, 'Matched for address in Cc:');
 $num = $ibx->mm->num_for('none@a.com');
 is($num, undef, 'No match without address in To: or Cc:');


On a side note, some of my private scripts use neither
-w, warnings, nor strict.  It's quite liberating, especially
when combined with `do' instead of `require' :>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] watchmaildir: support multiple watchheader values
  2020-04-20  0:45     ` Eric Wong
@ 2020-04-20  1:13       ` Kyle Meyer
  2020-04-20  1:32         ` Eric Wong
  0 siblings, 1 reply; 8+ messages in thread
From: Kyle Meyer @ 2020-04-20  1:13 UTC (permalink / raw)
  To: Eric Wong; +Cc: meta

Eric Wong <e@80x24.org> writes:

> Kyle Meyer <kyle@kyleam.com> wrote:
>
> <snip> Thanks.  Most everything looks good, some minor issues..
>
>> +++ b/t/watch_muliple_headers.t
>
> speling :>

Oops, thanks.

>> +my $num = $ibx->mm->num_for('to@a.com');
>> +ok(defined $num, 'Matched for address in To:');
>> +my $num = $ibx->mm->num_for('cc@a.com');
>
> That `$num' shadows/masks

Ah, sorry.  Aside from, er, actually knowing what I'm doing in perl and
seeing that obvious mistake, is there a way I could see the warning when
I run the tests?  To run only the tests in that file as I was working on
it, I started with the command I saw when I called 'make test' and
restricted it to just the file, So, with typo included for historical
accuracy :), I was running

  PERL_DL_NONLAZY=1 "/usr/bin/perl" "-MExtUtils::Command::MM" \
    "-MTest::Harness" "-e" \
    "undef *Test::Harness::Switches; test_harness(0, 'blib/lib', 'blib/arch')" \
    t/watch_muliple_headers.t

> Will squash the following in before pushing:

Thanks!

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v2] watchmaildir: support multiple watchheader values
  2020-04-20  1:13       ` Kyle Meyer
@ 2020-04-20  1:32         ` Eric Wong
  2020-04-20  7:05           ` [PATCH] doc: HACKING: add a bit about faster testing Eric Wong
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Wong @ 2020-04-20  1:32 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: meta

Kyle Meyer <kyle@kyleam.com> wrote:
> Eric Wong <e@80x24.org> writes:
> 
> > Kyle Meyer <kyle@kyleam.com> wrote:
> >
> > <snip> Thanks.  Most everything looks good, some minor issues..
> >
> >> +++ b/t/watch_muliple_headers.t
> >
> > speling :>
> 
> Oops, thanks.
> 
> >> +my $num = $ibx->mm->num_for('to@a.com');
> >> +ok(defined $num, 'Matched for address in To:');
> >> +my $num = $ibx->mm->num_for('cc@a.com');
> >
> > That `$num' shadows/masks
> 
> Ah, sorry.  Aside from, er, actually knowing what I'm doing in perl and
> seeing that obvious mistake, is there a way I could see the warning when
> I run the tests?  To run only the tests in that file as I was working on
> it, I started with the command I saw when I called 'make test' and
> restricted it to just the file, So, with typo included for historical
> accuracy :), I was running

Ah, oops.  I normally use `make check' (or `check-run' after
being primed by `check').  HACKING needs to be patched, and
maybe a pointer to it in INSTALL.

I definitely want warnings enabled for hackers.  However, since
rare new warnings will surface from Perl(*), I'm less and less
enthusiastic about enabling them for non-hackers.

So I'm moving away from `use warnings' and may even consider
removing `-w' from the default shebangs and rely on PERL5OPT
or `-w' being propagated via `check-run'.


(*) Fwiw, I've hit only hit this 2-3 times in two decades.
    1) each iterator reuse (it was intentional :P)
    2) `{' and `}' in regexps
    I think there was another, but that's all I can think of atm.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] doc: HACKING: add a bit about faster testing
  2020-04-20  1:32         ` Eric Wong
@ 2020-04-20  7:05           ` Eric Wong
  2020-04-20 13:26             ` Kyle Meyer
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Wong @ 2020-04-20  7:05 UTC (permalink / raw)
  To: meta; +Cc: Kyle Meyer

Eric Wong <e@80x24.org> wrote:
> Kyle Meyer <kyle@kyleam.com> wrote:
> > Ah, sorry.  Aside from, er, actually knowing what I'm doing in perl and
> > seeing that obvious mistake, is there a way I could see the warning when
> > I run the tests?  To run only the tests in that file as I was working on
> > it, I started with the command I saw when I called 'make test' and
> > restricted it to just the file, So, with typo included for historical
> > accuracy :), I was running
> 
> Ah, oops.  I normally use `make check' (or `check-run' after
> being primed by `check').  HACKING needs to be patched, and
> maybe a pointer to it in INSTALL.

------8<-----
Subject: [PATCH] doc: HACKING: add a bit about faster testing

`make test' is annoyingly slow, and `make check-run' works
wonders for improving the edit && test cycle.
---
 Documentation/txt2pre |  5 +++--
 HACKING               | 18 ++++++++++++++++++
 INSTALL               |  2 +-
 3 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/Documentation/txt2pre b/Documentation/txt2pre
index c3a7657e..cf58bad8 100755
--- a/Documentation/txt2pre
+++ b/Documentation/txt2pre
@@ -39,9 +39,9 @@ for (qw[copydatabase(1) xapian-compact(1)]) {
 	$xurls{$_} = ".$n.1.html"
 }
 
-for (qw[flock(2) setrlimit(2) vfork(2)]) {
+for (qw[make(1) flock(2) setrlimit(2) vfork(2) tmpfs(5)]) {
 	my ($n, $s) = (/([\w\-]+)\((\d)\)/);
-	$xurls{$_} = "http://www.man7.org/linux/man-pages/man2/$n.$s.html";
+	$xurls{$_} = "http://www.man7.org/linux/man-pages/man$s/$n.$s.html";
 }
 
 for (qw[git(1)
@@ -82,6 +82,7 @@ $xurls{'git-filter-repo(1)'} = 'https://github.com/newren/git-filter-repo'.
 			'./blob/master/Documentation/git-filter-repo.txt';
 $xurls{'ssoma(1)'} = 'https://ssoma.public-inbox.org/ssoma.txt';
 $xurls{'cgitrc(5)'} = 'https://git.zx2c4.com/cgit/tree/cgitrc.5.txt';
+$xurls{'prove(1)'} = 'https://perldoc.perl.org/prove.html';
 
 my $str = do { local $/; <STDIN> };
 my ($title) = ($str =~ /\A([^\n]+)/);
diff --git a/HACKING b/HACKING
index cceb686f..74a3096f 100644
--- a/HACKING
+++ b/HACKING
@@ -59,6 +59,24 @@ directory for design decisions made during development.
 See Documentation/technical/ in the source tree for more details
 on specific topics, in particular data_structures.txt
 
+Faster tests
+------------
+
+The `make test' target provided by MakeMaker does not run in
+parallel.  Our `make check' target supports parallel runs, and
+it also creates a `.prove' file to optimize `make check-run'.
+
+The prove(1) command (distributed with Perl) may also be used
+for finer-grained testing: prove -bvw t/foo.t
+
+If using a make(1) (e.g. GNU make) with `include' support, the
+`config.mak' Makefile snippet can be used to set environment
+variables such as PERL_INLINE_DIRECTORY and TMPDIR.
+
+With PERL_INLINE_DIRECTORY set to enable Inline::C support and
+TMPDIR pointed to a tmpfs(5) mount, `make check-run' takes 6-10s
+(load-dependent) on a busy workstation built in 2010.
+
 Perl notes
 ----------
 
diff --git a/INSTALL b/INSTALL
index 3984df71..2dd7dcff 100644
--- a/INSTALL
+++ b/INSTALL
@@ -191,7 +191,7 @@ install the system (into /usr/local) with:
 
         perl Makefile.PL
         make
-        make test
+        make test    # see HACKING for faster tests for hackers
         make install # root permissions may be needed
 
 When installing Search::Xapian, make sure the underlying Xapian

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] doc: HACKING: add a bit about faster testing
  2020-04-20  7:05           ` [PATCH] doc: HACKING: add a bit about faster testing Eric Wong
@ 2020-04-20 13:26             ` Kyle Meyer
  0 siblings, 0 replies; 8+ messages in thread
From: Kyle Meyer @ 2020-04-20 13:26 UTC (permalink / raw)
  To: Eric Wong; +Cc: meta

Eric Wong <e@80x24.org> writes:

> `make test' is annoyingly slow, and `make check-run' works
> wonders for improving the edit && test cycle.
> ---
>  Documentation/txt2pre |  5 +++--
>  HACKING               | 18 ++++++++++++++++++
>  INSTALL               |  2 +-
>  3 files changed, 22 insertions(+), 3 deletions(-)

Great, thank you for the information!

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-12  4:44 [RFC PATCH] watchmaildir: support multiple watchheader values Kyle Meyer
2020-04-12 21:59 ` Eric Wong
2020-04-20  0:13   ` [PATCH v2] " Kyle Meyer
2020-04-20  0:45     ` Eric Wong
2020-04-20  1:13       ` Kyle Meyer
2020-04-20  1:32         ` Eric Wong
2020-04-20  7:05           ` [PATCH] doc: HACKING: add a bit about faster testing Eric Wong
2020-04-20 13:26             ` Kyle Meyer

user/dev discussion of public-inbox itself

Archives are clonable:
	git clone --mirror http://public-inbox.org/meta
	git clone --mirror http://czquwvybam4bgbro.onion/meta
	git clone --mirror http://hjrcffqmbrq6wope.onion/meta
	git clone --mirror http://ou63pmih66umazou.onion/meta

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.mail.public-inbox.meta
	nntp://ou63pmih66umazou.onion/inbox.comp.mail.public-inbox.meta
	nntp://czquwvybam4bgbro.onion/inbox.comp.mail.public-inbox.meta
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.mail.public-inbox.meta
	nntp://news.gmane.io/gmane.mail.public-inbox.general

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git