[PATCH 0/7] nntp: another round of updates

[PATCH 0/7] nntp: another round of updates

[Eric Wong]

I guess we'll implement RFC 3977 before we commit to a extending
fields needed for an overview database...

Eric Wong (7):
      t/nntpd.t: fix fcntl test to ensure no failures
      nntp: support YYYYMMDD dates for commands
      nntp: ensure body lines end with CRLF
      nntp: avoid setting Bytes and Lines headers
      nntp: implement OVER from RFC 3977
      nntp: XHDR fixes for Message-ID lookups
      nntp: XHDR lookups by Message-ID may cross groups

 lib/PublicInbox/NNTP.pm | 136 ++++++++++++++++++++++++++++++++----------------
 t/nntp.t                |  29 +++++++++++
 t/nntpd.t               |  30 ++++++++++-
 3 files changed, 147 insertions(+), 48 deletions(-)

[PATCH 1/7] t/nntpd.t: fix fcntl test to ensure no failures

[Eric Wong]

We need to run the syscall before testing for its failure :x
---
 t/nntpd.t | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/nntpd.t b/t/nntpd.t
index d30ed63..d886e3c 100644
--- a/t/nntpd.t
+++ b/t/nntpd.t
@@ -88,8 +88,8 @@ EOF
 	}
 	ok(defined $pid, 'forked nntpd process successfully');
 	$! = 0;
-	ok(! $!, 'no error from fcntl(F_SETFD)');
 	fcntl($sock, F_SETFD, $fl |= FD_CLOEXEC);
+	ok(! $!, 'no error from fcntl(F_SETFD)');
 	my $n = Net::NNTP->new($sock->sockhost . ':' . $sock->sockport);
 	my $list = $n->list;
 	is_deeply($list, { $group => [ qw(1 1 n) ] }, 'LIST works');
-- 
EW

[PATCH 2/7] nntp: support YYYYMMDD dates for commands

[Eric Wong]

RFC 3977 supports YYYYMMDD dates while retaining backwards
compatibility for old YYMMDD dates.
---
 lib/PublicInbox/NNTP.pm | 18 +++++++++++-------
 t/nntp.t                | 29 +++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index 91b10f2..bc8d6ed 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -169,22 +169,26 @@ sub cmd_listgroup ($;$) {
 sub parse_time ($$;$) {
 	my ($date, $time, $gmt) = @_;
 	use Time::Local qw();
-	my ($YY, $MM, $DD) = unpack('A2A2A2', $date);
 	my ($hh, $mm, $ss) = unpack('A2A2A2', $time);
 	if (defined $gmt) {
 		$gmt =~ /\A(?:UTC|GMT)\z/i or die "GM invalid: $gmt\n";
 		$gmt = 1;
 	}
 	my @now = $gmt ? gmtime : localtime;
-	if ($YY > strftime('%y', @now)) {
-		my $cur_year = $now[5] + 1900;
-		$YY += int($cur_year / 1000) * 1000 - 100;
+	my ($YYYY, $MM, $DD);
+	if (length($date) == 8) { # RFC 3977 allows YYYYMMDD
+		($YYYY, $MM, $DD) = unpack('A4A2A2', $date);
+	} else { # legacy clients send YYMMDD
+		($YYYY, $MM, $DD) = unpack('A2A2A2', $date);
+		if ($YYYY > strftime('%y', @now)) {
+			my $cur_year = $now[5] + 1900;
+			$YYYY += int($cur_year / 1000) * 1000 - 100;
+		}
 	}
-
 	if ($gmt) {
-		Time::Local::timegm($ss, $mm, $hh, $DD, $MM - 1, $YY);
+		Time::Local::timegm($ss, $mm, $hh, $DD, $MM - 1, $YYYY);
 	} else {
-		Time::Local::timelocal($ss, $mm, $hh, $DD, $MM - 1, $YY);
+		Time::Local::timelocal($ss, $mm, $hh, $DD, $MM - 1, $YYYY);
 	}
 }
 
diff --git a/t/nntp.t b/t/nntp.t
index 82918ff..c917888 100644
--- a/t/nntp.t
+++ b/t/nntp.t
@@ -61,4 +61,33 @@ use_ok 'PublicInbox::NNTP';
 	ngpat_like('a.s.r', 'a.t,a.s.*');
 }
 
+{
+	use POSIX qw(strftime);
+	sub time_roundtrip {
+		my ($date, $time, $gmt) = @_;
+		my $m = join(' ', @_);
+		my $ts = PublicInbox::NNTP::parse_time(@_);
+		my @t = gmtime($ts);
+		my ($d, $t);
+		if (length($date) == 8) {
+			($d, $t) = split(' ', strftime('%Y%m%d %H%M%S', @t));
+		} else {
+			($d, $t) = split(' ', strftime('%g%m%d %H%M%S', @t));
+		}
+		is_deeply([$d, $t], [$date, $time], "roundtripped: $m");
+		$ts;
+	}
+	my $x1 = time_roundtrip(qw(20141109 060606 GMT));
+	my $x2 = time_roundtrip(qw(141109 060606 GMT));
+	my $x3 = time_roundtrip(qw(930724 060606 GMT));
+
+	SKIP: {
+		skip('YYMMDD test needs updating', 2) if (time > 0x7fffffff);
+		# our world probably ends in 2038, but if not we'll try to
+		# remember to update the test then
+		is($x1, $x2, 'YYYYMMDD and YYMMDD parse identically');
+		is(strftime('%Y', gmtime($x3)), '1993', '930724 was in 1993');
+	}
+}
+
 done_testing();
-- 
EW

[PATCH 3/7] nntp: ensure body lines end with CRLF

[Eric Wong]

It's common for mail bodies to end with LF-only, so end them with
CRLF to avoid triggering errors in clients.
---
 lib/PublicInbox/NNTP.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index bc8d6ed..71b774d 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -393,7 +393,9 @@ sub simple_body_write ($$) {
 	my $body = $s->body;
 	$s->body_set('');
 	$body =~ s/^\./../smg;
+	$body =~ s/(?<!\r)\n/\r\n/sg;
 	do_more($self, $body);
+	do_more($self, "\r\n") unless $body =~ /\r\n\z/s;
 	'.'
 }
 
-- 
EW

[PATCH 4/7] nntp: avoid setting Bytes and Lines headers

[Eric Wong]

These are internal metadata should be calculated, so avoid
leaking it into the header.
---
 lib/PublicInbox/NNTP.pm | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index 71b774d..e5bb683 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -374,18 +374,16 @@ found:
 	my $bytes;
 	my $s = eval { Email::MIME->new($ng->gcf->cat_file($o, \$bytes)) };
 	return $err unless $s;
+	my $lines;
 	if ($set_headers) {
 		$s->header_set('Newsgroups', $ng->{name});
-		$s->header_set('Lines', $s->body =~ tr!\n!\n!);
 		$s->header_set('Xref', xref($ng, $n));
+		$lines = $s->body =~ tr!\n!\n!;
 
 		# must be last
-		if ($set_headers == 2) {
-			$s->header_set('Bytes', $bytes);
-			$s->body_set('');
-		}
+		$s->body_set('') if ($set_headers == 2);
 	}
-	[ $n, $mid, $s ];
+	[ $n, $mid, $s, $bytes, $lines ];
 }
 
 sub simple_body_write ($$) {
@@ -399,13 +397,6 @@ sub simple_body_write ($$) {
 	'.'
 }
 
-sub header_str ($) {
-	my ($s) = @_;
-	my $h = $s->header_obj;
-	$h->header_set('Bytes');
-	$h->as_string
-}
-
 sub set_art {
 	my ($self, $art) = @_;
 	$self->{article} = $art if defined $art && $art =~ /\A\d+\z/;
@@ -418,7 +409,7 @@ sub cmd_article ($;$) {
 	my ($n, $mid, $s) = @$r;
 	set_art($self, $art);
 	more($self, "220 $n <$mid> article retrieved - head and body follow");
-	do_more($self, header_str($s));
+	do_more($self, $s->header_obj->as_string);
 	do_more($self, "\r\n");
 	simple_body_write($self, $s);
 }
@@ -430,7 +421,7 @@ sub cmd_head ($;$) {
 	my ($n, $mid, $s) = @$r;
 	set_art($self, $art);
 	more($self, "221 $n <$mid> article retrieved - head follows");
-	do_more($self, header_str($s));
+	do_more($self, $s->header_obj->as_string);
 	'.'
 }
 
@@ -486,6 +477,9 @@ sub get_range ($$) {
 
 sub xhdr ($$) {
 	my ($r, $header) = @_;
+	$header = lc $header;
+	return $r->[3] if ($header eq 'bytes');
+	return $r->[4] if ($header eq 'lines');
 	$r = $r->[2]->header_obj->header($header);
 	defined $r or return;
 	$r =~ s/[\r\n\t]+/ /sg;
-- 
EW

[PATCH 5/7] nntp: implement OVER from RFC 3977

[Eric Wong]

This is just like the XOVER command, but allows a single Message-ID
to be given.
---
 lib/PublicInbox/NNTP.pm | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index e5bb683..1286b70 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -383,7 +383,7 @@ found:
 		# must be last
 		$s->body_set('') if ($set_headers == 2);
 	}
-	[ $n, $mid, $s, $bytes, $lines ];
+	[ $n, $mid, $s, $bytes, $lines, $ng ];
 }
 
 sub simple_body_write ($$) {
@@ -701,6 +701,32 @@ sub cmd_xrover ($;$) {
 	});
 }
 
+sub over_line {
+	my ($self, $r) = @_;
+
+	more($self, join("\t", $r->[0], map {
+				my $h = xhdr($r, $_);
+				defined $h ? $h : '';
+			} @OVERVIEW ));
+}
+
+sub cmd_over ($;$) {
+	my ($self, $range) = @_;
+	if ($range && $range =~ /\A<.+>\z/) {
+		my $r = $self->art_lookup($range, 2);
+		return '430 No article with that message-id' unless ref $r;
+		more($self, '224 Overview information follows (multi-line)');
+
+		# Only set article number column if it's the current group
+		my $ng = $self->{ng};
+		$r->[0] = 0 if (!$ng || $ng ne $r->[5]);
+		over_line($self, $r);
+		'.';
+	} else {
+		cmd_xover($self, $range);
+	}
+}
+
 sub cmd_xover ($;$) {
 	my ($self, $range) = @_;
 	$range = $self->{article} unless defined $range;
@@ -712,11 +738,7 @@ sub cmd_xover ($;$) {
 		my ($i) = @_;
 		my $r = $self->art_lookup($$i, 2);
 		return unless ref $r;
-		more($self, join("\t", $r->[0],
-				map {
-					my $h = xhdr($r, $_);
-					defined $h ? $h : '';
-				} @OVERVIEW ));
+		over_line($self, $r);
 	});
 }
 
-- 
EW

[PATCH 6/7] nntp: XHDR fixes for Message-ID lookups

[Eric Wong]

We'll require some modifications for HDR support, though.
---
 lib/PublicInbox/NNTP.pm | 13 +++++++------
 t/nntpd.t               | 17 +++++++++++++++++
 2 files changed, 24 insertions(+), 6 deletions(-)

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index 1286b70..c8ef01e 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -546,9 +546,10 @@ sub xhdr_message_id ($$) { # optimize XHDR Message-ID [range] for slrnpull.
 
 	my $mm = $self->{ng}->mm;
 	if (defined $range && $range =~ /\A<(.+)>\z/) { # Message-ID
-		my $n = $mm->num_for($range);
+		my $n = $mm->num_for($1);
+		return '430 No article with that message-id' unless $n;
 		more($self, '221 Header follows');
-		more($self, "<$range> <$range>") if defined $n;
+		more($self, "$range $range");
 		'.';
 	} else { # numeric range
 		$range = $self->{article} unless defined $range;
@@ -575,9 +576,9 @@ sub xhdr_xref ($$) { # optimize XHDR Xref [range] for rtin
 	my $ng = $self->{ng};
 	my $mm = $ng->mm;
 	if (defined $range && $range =~ /\A<(.+)>\z/) { # Message-ID
-		my $n = $mm->num_for($range);
+		my $n = $mm->num_for($1);
 		more($self, '221 Header follows');
-		more($self, "<$range> ".xref($ng, $n)) if defined $n;
+		more($self, "$range ".xref($ng, $n)) if defined $n;
 		'.';
 	} else { # numeric range
 		$range = $self->{article} unless defined $range;
@@ -655,12 +656,12 @@ sub cmd_xhdr ($$;$) {
 sub xhdr_slow ($$$) {
 	my ($self, $header, $range) = @_;
 
-	if (defined $range && $range =~ /\A<(.+)>\z/) { # Message-ID
+	if (defined $range && $range =~ /\A<.+>\z/) { # Message-ID
 		my $r = $self->art_lookup($range, 2);
 		return $r unless ref $r;
 		more($self, '221 Header follows');
 		if (defined($r = xhdr($r, $header))) {
-			more($self, "<$range> $r");
+			more($self, "$range $r");
 		}
 		'.';
 	} else { # numeric range
diff --git a/t/nntpd.t b/t/nntpd.t
index d886e3c..090e150 100644
--- a/t/nntpd.t
+++ b/t/nntpd.t
@@ -95,6 +95,23 @@ EOF
 	is_deeply($list, { $group => [ qw(1 1 n) ] }, 'LIST works');
 	is_deeply([$n->group($group)], [ qw(0 1 1), $group ], 'GROUP works');
 
+	my $mid = '<nntp@example.com>';
+	my %xhdr = (
+		'message-id' => $mid,
+		'subject' => 'hihi',
+		'date' => 'Thu, 01 Jan 1970 00:00:00 +0000',
+		'from' => 'Me <me@example.com>',
+		'to' => 'You <you@example.com>',
+		'cc' => $addr,
+		'xref' => "example.com $group:1"
+	);
+	while (my ($k, $v) = each %xhdr) {
+		is_deeply($n->xhdr("$k $mid"), { $mid => $v },
+			  "$k by message-id works");
+		is_deeply($n->xhdr("$k 1"), { 1 => $v },
+			  "$k by article number works");
+	}
+
 	# TODO: upgrades and such
 
 	ok(kill('TERM', $pid), 'killed nntpd');
-- 
EW

[PATCH 7/7] nntp: XHDR lookups by Message-ID may cross groups

[Eric Wong]

This is allowed by RFC 2980 and HDR (to-be-implemented) in
RFC 3977 supports it, too.
---
 lib/PublicInbox/NNTP.pm | 55 ++++++++++++++++++++++++++++++++++---------------
 t/nntpd.t               | 11 +++++++++-
 2 files changed, 48 insertions(+), 18 deletions(-)

diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm
index c8ef01e..19e0848 100644
--- a/lib/PublicInbox/NNTP.pm
+++ b/lib/PublicInbox/NNTP.pm
@@ -544,17 +544,18 @@ sub long_response ($$$$) {
 sub xhdr_message_id ($$) { # optimize XHDR Message-ID [range] for slrnpull.
 	my ($self, $range) = @_;
 
-	my $mm = $self->{ng}->mm;
 	if (defined $range && $range =~ /\A<(.+)>\z/) { # Message-ID
-		my $n = $mm->num_for($1);
+		my ($ng, $n) = mid_lookup($self, $1);
 		return '430 No article with that message-id' unless $n;
 		more($self, '221 Header follows');
+		my $self_ng = $self->{ng};
 		more($self, "$range $range");
 		'.';
 	} else { # numeric range
 		$range = $self->{article} unless defined $range;
 		my $r = get_range($self, $range);
 		return $r unless ref $r;
+		my $mm = $self->{ng}->mm;
 		my ($beg, $end) = @$r;
 		more($self, '221 Header follows');
 		$self->long_response($beg, $end, sub {
@@ -570,20 +571,36 @@ sub xref ($$) {
 	"$ng->{domain} $ng->{name}:$n"
 }
 
+sub mid_lookup ($$) {
+	my ($self, $mid) = @_;
+	my $self_ng = $self->{ng};
+	if ($self_ng) {
+		my $n = $self_ng->mm->num_for($mid);
+		return ($self_ng, $n) if defined $n;
+	}
+	foreach my $ng (values %{$self->{nntpd}->{groups}}) {
+		next if $ng eq $self_ng;
+		my $n = $ng->mm->num_for($mid);
+		return ($ng, $n) if defined $n;
+	}
+	(undef, undef);
+}
+
 sub xhdr_xref ($$) { # optimize XHDR Xref [range] for rtin
 	my ($self, $range) = @_;
 
-	my $ng = $self->{ng};
-	my $mm = $ng->mm;
 	if (defined $range && $range =~ /\A<(.+)>\z/) { # Message-ID
-		my $n = $mm->num_for($1);
+		my ($ng, $n) = mid_lookup($self, $1);
+		return '430 No article with that message-id' unless $n;
 		more($self, '221 Header follows');
-		more($self, "$range ".xref($ng, $n)) if defined $n;
+		more($self, "$range ".xref($ng, $n));
 		'.';
 	} else { # numeric range
 		$range = $self->{article} unless defined $range;
 		my $r = get_range($self, $range);
 		return $r unless ref $r;
+		my $ng = $self->{ng};
+		my $mm = $ng->mm;
 		my ($beg, $end) = @$r;
 		more($self, '221 Header follows');
 		$self->long_response($beg, $end, sub {
@@ -605,7 +622,6 @@ sub header_obj_for {
 
 sub xhdr_searchmsg ($$$) {
 	my ($self, $sub, $range) = @_;
-	my $srch = $self->{ng}->search;
 	my $emit = ($sub eq 'date') ? sub {
 		my ($pfx, $m) = @_;
 		my @t = gmtime($m->header('X-PI-TS'));
@@ -617,12 +633,20 @@ sub xhdr_searchmsg ($$$) {
 	};
 
 	if (defined $range && $range =~ /\A<(.+)>\z/) { # Message-ID
-		more($self, '221 Header follows');
-		my $m = header_obj_for($srch, $1);
-		$emit->($range, $m) if defined $m;
-		'.';
+		my ($ng, $n) = mid_lookup($self, $1);
+		return '430 No article with that message-id' unless $n;
+		if (my $srch = $ng->search) {
+			more($self, '221 Header follows');
+			my $m = header_obj_for($srch, $range);
+			$emit->($range, $m) if defined $m;
+			'.';
+		} else {
+			xhdr_slow($self, $sub, $range);
+		}
 	} else { # numeric range
 		$range = $self->{article} unless defined $range;
+		my $srch = $self->{ng}->search or
+				return xhdr_slow($self, $sub, $range);
 		my $mm = $self->{ng}->mm;
 		my $r = get_range($self, $range);
 		return $r unless ref $r;
@@ -639,14 +663,12 @@ sub xhdr_searchmsg ($$$) {
 
 sub cmd_xhdr ($$;$) {
 	my ($self, $header, $range) = @_;
-	my $ng = $self->{ng};
-	defined $ng or return '412 no news group currently selected';
 	my $sub = lc $header;
 	if ($sub eq 'message-id') {
 		xhdr_message_id($self, $range);
 	} elsif ($sub eq 'xref') {
 		xhdr_xref($self, $range);
-	} elsif ($sub =~ /\A(subject|references|date)\z/ && $ng->search) {
+	} elsif ($sub =~ /\A(subject|references|date)\z/) {
 		xhdr_searchmsg($self, $sub, $range);
 	} else {
 		xhdr_slow($self, $header, $range);
@@ -660,9 +682,8 @@ sub xhdr_slow ($$$) {
 		my $r = $self->art_lookup($range, 2);
 		return $r unless ref $r;
 		more($self, '221 Header follows');
-		if (defined($r = xhdr($r, $header))) {
-			more($self, "$range $r");
-		}
+		$r = xhdr($r, $header);
+		more($self, "$range $r") if defined $r;
 		'.';
 	} else { # numeric range
 		$range = $self->{article} unless defined $range;
diff --git a/t/nntpd.t b/t/nntpd.t
index 090e150..28d8491 100644
--- a/t/nntpd.t
+++ b/t/nntpd.t
@@ -90,7 +90,8 @@ EOF
 	$! = 0;
 	fcntl($sock, F_SETFD, $fl |= FD_CLOEXEC);
 	ok(! $!, 'no error from fcntl(F_SETFD)');
-	my $n = Net::NNTP->new($sock->sockhost . ':' . $sock->sockport);
+	my $host_port = $sock->sockhost . ':' . $sock->sockport;
+	my $n = Net::NNTP->new($host_port);
 	my $list = $n->list;
 	is_deeply($list, { $group => [ qw(1 1 n) ] }, 'LIST works');
 	is_deeply([$n->group($group)], [ qw(0 1 1), $group ], 'GROUP works');
@@ -112,6 +113,14 @@ EOF
 			  "$k by article number works");
 	}
 
+	{
+		my $nogroup = Net::NNTP->new($host_port);
+		while (my ($k, $v) = each %xhdr) {
+			is_deeply($nogroup->xhdr("$k $mid"), { $mid => $v },
+				  "$k by message-id works without group");
+		}
+	}
+
 	# TODO: upgrades and such
 
 	ok(kill('TERM', $pid), 'killed nntpd');
-- 
EW