user/dev discussion of public-inbox itself
 help / color / mirror / code / Atom feed
* [PATCH 0/2] Make NNTP Xrefs work better
@ 2018-09-04 15:28 Jonathan Corbet
  2018-09-04 15:28 ` [PATCH 1/2] Put the NNTP server name into Xref lines Jonathan Corbet
  2018-09-04 15:28 ` [PATCH 2/2] Add Xrefs to over/xover lines Jonathan Corbet
  0 siblings, 2 replies; 6+ messages in thread
From: Jonathan Corbet @ 2018-09-04 15:28 UTC (permalink / raw)
  To: meta; +Cc: Jonathan Corbet

I finally found some time to figure out why the Xref changes weren't having
the desired effect for me; the result is the following two changes.

As always, these patches are likely to reveal that I really haven't done
any perl in decades and that I'm still kind of groping my way around the
public-inbox source.  Hopefully they aren't too bad...


Jonathan Corbet (2):
  Put the NNTP server name into Xref lines
  Add Xrefs to over/xover lines

 lib/PublicInbox/NNTP.pm  | 17 +++++++++--------
 lib/PublicInbox/NNTPD.pm |  6 ++++++
 2 files changed, 15 insertions(+), 8 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 1/2] Put the NNTP server name into Xref lines
  2018-09-04 15:28 [PATCH 0/2] Make NNTP Xrefs work better Jonathan Corbet
@ 2018-09-04 15:28 ` Jonathan Corbet
  2018-09-05 21:34   ` Eric Wong
  2018-09-04 15:28 ` [PATCH 2/2] Add Xrefs to over/xover lines Jonathan Corbet
  1 sibling, 1 reply; 6+ messages in thread
From: Jonathan Corbet @ 2018-09-04 15:28 UTC (permalink / raw)
  To: meta; +Cc: Jonathan Corbet

RFC 5536 sec 3.2.14 says that the server-name in an Xref line is "which
news server generated the header field"; indeed, that is necessary for
newsreaders like gnus to handle references properly.  So pick up the server
name from the config if available, from the host name otherwise, and use it
rather than the domain name of the list server.
---
 lib/PublicInbox/NNTP.pm  | 4 ++--
 lib/PublicInbox/NNTPD.pm | 6 ++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index cdbd8e9..cbd4ecf 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -94,7 +94,7 @@ sub new ($$$) {
 	my $self = fields::new($class);
 	$self->SUPER::new($sock);
 	$self->{nntpd} = $nntpd;
-	res($self, '201 server ready - post via email');
+	res($self, '201 ' . $nntpd->{servername} . ' ready - post via email');
 	$self->{rbuf} = '';
 	$self->watch_read(1);
 	update_idle_time($self);
@@ -410,7 +410,7 @@ sub header_append ($$$) {
 
 sub xref ($$$$) {
 	my ($self, $ng, $n, $mid) = @_;
-	my $ret = "$ng->{domain} $ng->{newsgroup}:$n";
+	my $ret = $self->{nntpd}->{servername} . " $ng->{newsgroup}:$n";
 
 	# num_for is pretty cheap and sometimes we'll lookup the existence
 	# of an article without getting even the OVER info.  In other words,
diff --git a/lib/PublicInbox/NNTPD.pm b/lib/PublicInbox/NNTPD.pm
index 117c9c0..666c53b 100644
--- a/lib/PublicInbox/NNTPD.pm
+++ b/lib/PublicInbox/NNTPD.pm
@@ -6,15 +6,21 @@
 package PublicInbox::NNTPD;
 use strict;
 use warnings;
+use Sys::Hostname;
 require PublicInbox::Config;
 
 sub new {
 	my ($class) = @_;
+	my $pi_config = PublicInbox::Config->new;
+	my $name = $pi_config->{'publicinbox.nntpserver'};
+	if ($name eq '') { $name = hostname; }
+
 	bless {
 		groups => {},
 		err => \*STDERR,
 		out => \*STDOUT,
 		grouplist => [],
+		servername => $name,
 	}, $class;
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2] Add Xrefs to over/xover lines
  2018-09-04 15:28 [PATCH 0/2] Make NNTP Xrefs work better Jonathan Corbet
  2018-09-04 15:28 ` [PATCH 1/2] Put the NNTP server name into Xref lines Jonathan Corbet
@ 2018-09-04 15:28 ` Jonathan Corbet
  1 sibling, 0 replies; 6+ messages in thread
From: Jonathan Corbet @ 2018-09-04 15:28 UTC (permalink / raw)
  To: meta; +Cc: Jonathan Corbet

Putting the Xref field into xover lines allows newsreaders to mark
cross-posted messages read when catching up a group.  That, in turn,
massively improves the life of crazy people who try to follow dozens of
kernel lists, where emails are often heavily cross-posted.
---
 lib/PublicInbox/NNTP.pm | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index cbd4ecf..022bb80 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -28,7 +28,7 @@ use constant {
 
 sub now () { clock_gettime(CLOCK_MONOTONIC) };
 
-my @OVERVIEW = qw(Subject From Date Message-ID References);
+my @OVERVIEW = qw(Subject From Date Message-ID References Xref);
 my $OVERVIEW_FMT = join(":\r\n", @OVERVIEW, qw(Bytes Lines)) . ":\r\n";
 my $LIST_HEADERS = join("\r\n", @OVERVIEW,
 			qw(:bytes :lines Xref To Cc)) . "\r\n";
@@ -812,8 +812,8 @@ sub cmd_xrover ($;$) {
 	});
 }
 
-sub over_line ($$) {
-	my ($num, $smsg) = @_;
+sub over_line ($$$$) {
+	my ($self, $ng, $num, $smsg) = @_;
 	# n.b. field access and procedural calls can be
 	# 10%-15% faster than OO method calls:
 	my $s = join("\t", $num,
@@ -823,7 +823,8 @@ sub over_line ($$) {
 		"<$smsg->{mid}>",
 		$smsg->{references},
 		$smsg->{bytes},
-		$smsg->{lines});
+		$smsg->{lines},
+		"Xref: " . xref($self, $ng, $num, $smsg->{mid}));
 	utf8::encode($s);
 	$s
 }
@@ -839,7 +840,7 @@ sub cmd_over ($;$) {
 		# Only set article number column if it's the current group
 		my $self_ng = $self->{ng};
 		$n = 0 if (!$self_ng || $self_ng ne $ng);
-		more($self, over_line($n, $smsg));
+		more($self, over_line($self, $ng, $n, $smsg));
 		'.';
 	} else {
 		cmd_xover($self, $range);
@@ -861,7 +862,7 @@ sub cmd_xover ($;$) {
 
 		# OVERVIEW.FMT
 		more($self, join("\r\n", map {
-			over_line($_->{num}, $_);
+			over_line($self, $self->{ng}, $_->{num}, $_);
 			} @$msgs));
 		$cur = $msgs->[-1]->{num} + 1;
 	});
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] Put the NNTP server name into Xref lines
  2018-09-04 15:28 ` [PATCH 1/2] Put the NNTP server name into Xref lines Jonathan Corbet
@ 2018-09-05 21:34   ` Eric Wong
  2018-09-05 22:26     ` Jonathan Corbet
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Wong @ 2018-09-05 21:34 UTC (permalink / raw)
  To: Jonathan Corbet; +Cc: meta

Jonathan Corbet <corbet@lwn.net> wrote:
> RFC 5536 sec 3.2.14 says that the server-name in an Xref line is "which
> news server generated the header field"; indeed, that is necessary for
> newsreaders like gnus to handle references properly.  So pick up the server
> name from the config if available, from the host name otherwise, and use it
> rather than the domain name of the list server.

Thanks.  Oops, I didn't realize the server-name was supposed to be there :x.
But it looks like some test cases need to be adjusted for these.
(see below)

> diff --git a/lib/PublicInbox/NNTPD.pm b/lib/PublicInbox/NNTPD.pm
> index 117c9c0..666c53b 100644
> --- a/lib/PublicInbox/NNTPD.pm
> +++ b/lib/PublicInbox/NNTPD.pm
> @@ -6,15 +6,21 @@
>  package PublicInbox::NNTPD;
>  use strict;
>  use warnings;
> +use Sys::Hostname;
>  require PublicInbox::Config;
>  
>  sub new {
>  	my ($class) = @_;
> +	my $pi_config = PublicInbox::Config->new;
> +	my $name = $pi_config->{'publicinbox.nntpserver'};
> +	if ($name eq '') { $name = hostname; }
> +

I haven't tested, but I think this needs to account for
multi-value nntpserver in the config (picking the first value is
fine, I suppose)

For example, I use the following config on news.public-inbox.org
because it's also available as a Tor hidden service:

	[publicinbox]
		nntpserver = news.public-inbox.org
		nntpserver = ou63pmih66umazou.onion


And here are test failures from t/nntp.t and t/nntpd.t: ("grep -v ^ok")


Use of uninitialized value $name in string eq at $HOME/public-inbox/lib/PublicInbox/NNTPD.pm line 16.

#   Failed test 'got greeting'
#   at t/nntpd.t line 149.
#          got: '201 $HOSTNAME ready - post via email
# '
#     expected: '201 server ready - post via email
# '

#   Failed test 'got greeting'
#   at t/nntpd.t line 159.
#          got: '201 $HOSTNAME ready - post via email
# '
#     expected: '201 server ready - post via email
# '

#   Failed test 'XHDR xref by message-id works'
#   at t/nntpd.t line 167.
#     Structures begin differing at:
#          $got->{<nntp@example.com>} = '$HOSTNAME test-nntpd:1'
#     $expected->{<nntp@example.com>} = 'example.com test-nntpd:1'

#   Failed test 'xref by article number works'
#   at t/nntpd.t line 169.
#     Structures begin differing at:
#          $got->{1} = '$HOSTNAME test-nntpd:1'
#     $expected->{1} = 'example.com test-nntpd:1'

#   Failed test 'xref by article range works'
#   at t/nntpd.t line 171.
#     Structures begin differing at:
#          $got->{1} = '$HOSTNAME test-nntpd:1'
#     $expected->{1} = 'example.com test-nntpd:1'

#   Failed test 'got expected response for HDR'
#   at t/nntpd.t line 178.
#          got: '0 $HOSTNAME test-nntpd:1'
#     expected: '0 example.com test-nntpd:1'

#   Failed test 'xref by message-id works without group'
#   at t/nntpd.t line 184.
#     Structures begin differing at:
#          $got->{<nntp@example.com>} = '$HOSTNAME test-nntpd:1'
#     $expected->{<nntp@example.com>} = 'example.com test-nntpd:1'
# Looks like you failed 7 tests of 92.
t/nntpd.t .. 
not ok 15 - got greeting
not ok 19 - got greeting
not ok 46 - XHDR xref by message-id works
not ok 47 - xref by article number works
not ok 48 - xref by article range works
not ok 50 - got expected response for HDR
not ok 66 - xref by message-id works without group
1..92
Dubious, test returned 7 (wstat 1792, 0x700)
Failed 7/92 subtests 
Use of uninitialized value in concatenation (.) or string at $HOME/public-inbox/lib/PublicInbox/NNTP.pm line 413.

#   Failed test 'Xref: set'
#   at t/nntp.t line 125.
#     Structures begin differing at:
#          $got->[0] = ' test:1'
#     $expected->[0] = 'example.com test:1'
Use of uninitialized value in concatenation (.) or string at $HOME/public-inbox/lib/PublicInbox/NNTP.pm line 413.

#   Failed test 'Old Xref: clobbered'
#   at t/nntp.t line 135.
#     Structures begin differing at:
#          $got->[0] = ' test:2'
#     $expected->[0] = 'example.com test:2'
# Looks like you failed 2 tests of 27.
t/nntp.t ... 
not ok 24 - Xref: set
not ok 27 - Old Xref: clobbered
1..27
Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/27 subtests 

Test Summary Report
-------------------
t/nntpd.t (Wstat: 1792 Tests: 92 Failed: 7)
  Failed tests:  15, 19, 46-48, 50, 66
  Non-zero exit status: 7
t/nntp.t (Wstat: 512 Tests: 27 Failed: 2)
  Failed tests:  24, 27
  Non-zero exit status: 2
Files=2, Tests=119,  1 wallclock secs ( 0.05 usr  0.00 sys +  0.97 cusr  0.21 csys =  1.23 CPU)
Result: FAIL

Thanks again.

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] Put the NNTP server name into Xref lines
  2018-09-05 21:34   ` Eric Wong
@ 2018-09-05 22:26     ` Jonathan Corbet
  0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Corbet @ 2018-09-05 22:26 UTC (permalink / raw)
  To: Eric Wong; +Cc: meta

On Wed, 5 Sep 2018 21:34:10 +0000
Eric Wong <e@80x24.org> wrote:

> But it looks like some test cases need to be adjusted for these.
> (see below)

Hey, man, the feature works for me...what more do you want?!? :)

It didn't occur to me to run the tests, of course.  I'll fix that up and
repost, but it may be a little before I get back to it; this allegedly
real-life thing is being a little intense at the moment...

Thanks,

jon

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 2/2] Add Xrefs to over/xover lines
  2018-10-13 21:42 [PATCH v2 0/2] Make NNTP Xrefs work better Jonathan Corbet
@ 2018-10-13 21:42 ` Jonathan Corbet
  0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Corbet @ 2018-10-13 21:42 UTC (permalink / raw)
  To: meta; +Cc: Eric Wong, Jonathan Corbet

Putting the Xref field into xover lines allows newsreaders to mark
cross-posted messages read when catching up a group.  That, in turn,
massively improves the life of crazy people who try to follow dozens of
kernel lists, where emails are often heavily cross-posted.
---
 lib/PublicInbox/NNTP.pm | 13 +++++++------
 t/nntpd.t               | 11 ++++++++---
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index cbd4ecf..022bb80 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -28,7 +28,7 @@ use constant {
 
 sub now () { clock_gettime(CLOCK_MONOTONIC) };
 
-my @OVERVIEW = qw(Subject From Date Message-ID References);
+my @OVERVIEW = qw(Subject From Date Message-ID References Xref);
 my $OVERVIEW_FMT = join(":\r\n", @OVERVIEW, qw(Bytes Lines)) . ":\r\n";
 my $LIST_HEADERS = join("\r\n", @OVERVIEW,
 			qw(:bytes :lines Xref To Cc)) . "\r\n";
@@ -812,8 +812,8 @@ sub cmd_xrover ($;$) {
 	});
 }
 
-sub over_line ($$) {
-	my ($num, $smsg) = @_;
+sub over_line ($$$$) {
+	my ($self, $ng, $num, $smsg) = @_;
 	# n.b. field access and procedural calls can be
 	# 10%-15% faster than OO method calls:
 	my $s = join("\t", $num,
@@ -823,7 +823,8 @@ sub over_line ($$) {
 		"<$smsg->{mid}>",
 		$smsg->{references},
 		$smsg->{bytes},
-		$smsg->{lines});
+		$smsg->{lines},
+		"Xref: " . xref($self, $ng, $num, $smsg->{mid}));
 	utf8::encode($s);
 	$s
 }
@@ -839,7 +840,7 @@ sub cmd_over ($;$) {
 		# Only set article number column if it's the current group
 		my $self_ng = $self->{ng};
 		$n = 0 if (!$self_ng || $self_ng ne $ng);
-		more($self, over_line($n, $smsg));
+		more($self, over_line($self, $ng, $n, $smsg));
 		'.';
 	} else {
 		cmd_xover($self, $range);
@@ -861,7 +862,7 @@ sub cmd_xover ($;$) {
 
 		# OVERVIEW.FMT
 		more($self, join("\r\n", map {
-			over_line($_->{num}, $_);
+			over_line($self, $self->{ng}, $_->{num}, $_);
 			} @$msgs));
 		$cur = $msgs->[-1]->{num} + 1;
 	});
diff --git a/t/nntpd.t b/t/nntpd.t
index f859908..9c1d076 100644
--- a/t/nntpd.t
+++ b/t/nntpd.t
@@ -196,7 +196,9 @@ EOF
 			'<nntp@example.com>',
 			'<reftabsqueezed>',
 			$len,
-			'1' ] }, "XOVER range works");
+			'1',
+			'Xref: '. hostname . ' test-nntpd:1'] },
+		"XOVER range works");
 
 	is_deeply($n->xover('1'), {
 		'1' => ["Testing for El\xc3\xa9anor",
@@ -205,7 +207,9 @@ EOF
 			'<nntp@example.com>',
 			'<reftabsqueezed>',
 			$len,
-			'1' ] }, "XOVER by article works");
+			'1',
+			'Xref: '. hostname . ' test-nntpd:1'] },
+		"XOVER by article works");
 
 	is_deeply($n->head(1), $n->head('<nntp@example.com>'), 'HEAD OK');
 	is_deeply($n->body(1), $n->body('<nntp@example.com>'), 'BODY OK');
@@ -225,7 +229,8 @@ EOF
 		is($r[1], "0\tTesting for El\xc3\xa9anor\t" .
 			"El\xc3\xa9anor <me\@example.com>\t" .
 			"Thu, 01 Jan 1970 06:06:06 +0000\t" .
-			"$mid\t<reftabsqueezed>\t$len\t1",
+			"$mid\t<reftabsqueezed>\t$len\t1" .
+			"\tXref: " . hostname . " test-nntpd:0",
 			'OVER by Message-ID works');
 		is($r[2], '.', 'correctly terminated response');
 	}
-- 
2.17.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-13 21:42 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-04 15:28 [PATCH 0/2] Make NNTP Xrefs work better Jonathan Corbet
2018-09-04 15:28 ` [PATCH 1/2] Put the NNTP server name into Xref lines Jonathan Corbet
2018-09-05 21:34   ` Eric Wong
2018-09-05 22:26     ` Jonathan Corbet
2018-09-04 15:28 ` [PATCH 2/2] Add Xrefs to over/xover lines Jonathan Corbet
  -- strict thread matches above, loose matches on Subject: below --
2018-10-13 21:42 [PATCH v2 0/2] Make NNTP Xrefs work better Jonathan Corbet
2018-10-13 21:42 ` [PATCH 2/2] Add Xrefs to over/xover lines Jonathan Corbet

Code repositories for project(s) associated with this public inbox

	https://80x24.org/public-inbox.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).