[PATCH 0/5] a few more cgit compatibility updates

[PATCH 0/5] a few more cgit compatibility updates

[Eric Wong]

This all needs tests, but Works For Me right now...

Eric Wong (5):
  config: support cgit scan-path and scan-hidden-path
  cgit: serve static css, logo, favicon directly
  config: support more cgit directives for project lists
  doc/config: update cgit.cgi scan location
  viewdiff: document constants

 Documentation/public-inbox-config.pod |   2 +-
 lib/PublicInbox/Cgit.pm               |  41 ++++++++++++-
 lib/PublicInbox/Config.pm             | 107 +++++++++++++++++++++++++++++++---
 lib/PublicInbox/GitHTTPBackend.pm     |  62 +++++++++++---------
 lib/PublicInbox/ViewDiff.pm           |   4 ++
 5 files changed, 174 insertions(+), 42 deletions(-)

-- 
EW

[PATCH 1/5] config: support cgit scan-path and scan-hidden-path

[Eric Wong]

project_list support still needs to be done

And tests need to be written... :<
---
 lib/PublicInbox/Config.pm | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm
index 9c1c3e2..9f1e57a 100644
--- a/lib/PublicInbox/Config.pm
+++ b/lib/PublicInbox/Config.pm
@@ -208,6 +208,30 @@ sub cgit_repo_merge ($$) {
 	$self->{"coderepo.$nick.cgiturl"} ||= $nick;
 }
 
+sub is_git_dir ($) {
+	my ($git_dir) = @_;
+	-d "$git_dir/objects" && -f "$git_dir/HEAD";
+}
+
+sub scan_path_coderepo {
+	my ($self, $base, $path) = @_;
+	opendir my $dh, $path or return;
+	while (defined(my $dn = readdir $dh)) {
+		next if $dn eq '.' || $dn eq '..';
+		if (index($dn, '.') == 0 && !$self->{-cgit_scan_hidden_path}) {
+			next;
+		}
+		my $nick = $base eq '' ? $dn : "$base/$dn";
+		my $git_dir = "$path/$dn";
+		if (is_git_dir($git_dir)) {
+			my $repo = { url => $nick, path => $git_dir };
+			cgit_repo_merge($self, $repo);
+		} elsif (-d $git_dir) {
+			scan_path_coderepo($self, $nick, $git_dir);
+		}
+	}
+}
+
 sub parse_cgitrc {
 	my ($self, $cgitrc, $nesting) = @_;
 
@@ -235,6 +259,10 @@ sub parse_cgitrc {
 			}
 		} elsif (m!\Ainclude=(.+)\z!) {
 			parse_cgitrc($self, $1, $nesting + 1);
+		} elsif (m!\Ascan-hidden-path=(\d+)\z!) {
+			$self->{-cgit_scan_hidden_path} = $1;
+		} elsif (m!\Ascan-path=(.+)\z!) {
+			scan_path_coderepo($self, '', $1);
 		}
 	}
 	cgit_repo_merge($self, $repo) if $repo;
-- 
EW

[PATCH 2/5] cgit: serve static css, logo, favicon directly

[Eric Wong]

We can reduce the configuration needed to run cgit by reusing
the static file handling logic of the dumb git HTTP protocol.

I hate logos and icons, so don't expect public-inbox.org or
80x24.org to ever have those to waste users' bandwidth with :P
But I expect other users to find this useful.
---
 lib/PublicInbox/Cgit.pm           | 41 ++++++++++++++++++++++++--
 lib/PublicInbox/Config.pm         | 10 +++++++
 lib/PublicInbox/GitHTTPBackend.pm | 62 +++++++++++++++++++++------------------
 3 files changed, 81 insertions(+), 32 deletions(-)

diff --git a/lib/PublicInbox/Cgit.pm b/lib/PublicInbox/Cgit.pm
index 9ba9e14..8922ec5 100644
--- a/lib/PublicInbox/Cgit.pm
+++ b/lib/PublicInbox/Cgit.pm
@@ -13,17 +13,45 @@ use PublicInbox::GitHTTPBackend;
 *input_prepare = *PublicInbox::GitHTTPBackend::input_prepare;
 *parse_cgi_headers = *PublicInbox::GitHTTPBackend::parse_cgi_headers;
 *serve = *PublicInbox::GitHTTPBackend::serve;
+*static_result = *PublicInbox::GitHTTPBackend::static_result;
 use warnings;
 use PublicInbox::Qspawn;
+use Plack::MIME;
+
+sub locate_cgit ($) {
+	my ($pi_config) = @_;
+	my $cgit_bin = $pi_config->{'publicinbox.cgitbin'};
+	my $cgit_data = $pi_config->{'publicinbox.cgitdata'};
+
+	# /var/www/htdocs/cgit is the default install path from cgit.git
+	# /usr/{lib,share}/cgit is where Debian puts cgit
+	# TODO: check other distros for common paths
+	unless (defined $cgit_bin) {
+		foreach (qw(/var/www/htdocs/cgit /usr/lib/cgit)) {
+			my $x = "$_/cgit.cgi";
+			next unless -x $x;
+			$cgit_bin = $x;
+			last;
+		}
+	}
+	unless (defined $cgit_data) {
+		foreach my $d (qw(/var/www/htdocs/cgit /usr/share/cgit)) {
+			my $f = "$d/cgit.css";
+			next unless -f $f;
+			$cgit_data = $d;
+			last;
+		}
+	}
+	($cgit_bin, $cgit_data);
+}
 
 sub new {
 	my ($class, $pi_config) = @_;
-	my $cgit_bin = $pi_config->{'publicinbox.cgitbin'} ||
-		# Debian default location:
-		'/usr/lib/cgit/cgit.cgi';
+	my ($cgit_bin, $cgit_data) = locate_cgit($pi_config);
 
 	my $self = bless {
 		cmd => [ $cgit_bin ],
+		cgit_data => $cgit_data,
 		pi_config => $pi_config,
 	}, $class;
 
@@ -39,6 +67,9 @@ sub new {
 	while (my ($nick, $repo) = each %$code_repos) {
 		$self->{"\0$nick"} = $repo;
 	}
+	my $cgit_static = $pi_config->{-cgit_static};
+	my $static = join('|', map { quotemeta $_ } keys %$cgit_static);
+	$self->{static} = qr/\A($static)\z/;
 	$self;
 }
 
@@ -66,6 +97,10 @@ sub call {
 		if (my $git = $self->{"\0$nick"}) {
 			return serve($env, $git, $path);
 		}
+	} elsif ($path_info =~ m!$self->{static}!) {
+		my $f = $1;
+		my $type = Plack::MIME->mime_type($f);
+		return static_result($env, [], "$self->{cgit_data}$f", $type);
 	}
 
 	my $cgi_env = { PATH_INFO => $path_info };
diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm
index 9f1e57a..2c1c511 100644
--- a/lib/PublicInbox/Config.pm
+++ b/lib/PublicInbox/Config.pm
@@ -234,6 +234,12 @@ sub scan_path_coderepo {
 
 sub parse_cgitrc {
 	my ($self, $cgitrc, $nesting) = @_;
+	if ($nesting == 0) {
+		# defaults:
+		my %s = map { $_ => 1 } qw(/cgit.css /cgit.png
+						/favicon.ico /robots.txt);
+		$self->{-cgit_static} = \%s;
+	}
 
 	# same limit as cgit/configfile.c::parse_configfile
 	return if $nesting > 8;
@@ -263,6 +269,10 @@ sub parse_cgitrc {
 			$self->{-cgit_scan_hidden_path} = $1;
 		} elsif (m!\Ascan-path=(.+)\z!) {
 			scan_path_coderepo($self, '', $1);
+
+		} elsif (m!\A(?:css|favicon|logo|repo\.logo)=(/.+)\z!) {
+			# absolute paths for static files via PublicInbox::Cgit
+			$self->{-cgit_static}->{$1} = 1;
 		}
 	}
 	cgit_repo_merge($self, $repo) if $repo;
diff --git a/lib/PublicInbox/GitHTTPBackend.pm b/lib/PublicInbox/GitHTTPBackend.pm
index cd8cdf8..57944a0 100644
--- a/lib/PublicInbox/GitHTTPBackend.pm
+++ b/lib/PublicInbox/GitHTTPBackend.pm
@@ -80,46 +80,26 @@ sub cache_one_year {
 		'Cache-Control', 'public, max-age=31536000';
 }
 
-sub serve_dumb {
-	my ($env, $git, $path) = @_;
-
-	my @h;
-	my $type;
-	if ($path =~ m!\Aobjects/[a-f0-9]{2}/[a-f0-9]{38}\z!) {
-		$type = 'application/x-git-loose-object';
-		cache_one_year(\@h);
-	} elsif ($path =~ m!\Aobjects/pack/pack-[a-f0-9]{40}\.pack\z!) {
-		$type = 'application/x-git-packed-objects';
-		cache_one_year(\@h);
-	} elsif ($path =~ m!\Aobjects/pack/pack-[a-f0-9]{40}\.idx\z!) {
-		$type = 'application/x-git-packed-objects-toc';
-		cache_one_year(\@h);
-	} elsif ($path =~ /\A(?:$TEXT)\z/o) {
-		$type = 'text/plain';
-		push @h, @no_cache;
-	} else {
-		return r(404);
-	}
-
-	my $f = $git->{git_dir} . '/' . $path;
+sub static_result ($$$$) {
+	my ($env, $h, $f, $type) = @_;
 	return r(404) unless -f $f && -r _; # just in case it's a FIFO :P
-	my $size = -s _;
 
 	# TODO: If-Modified-Since and Last-Modified?
 	open my $in, '<', $f or return r(404);
+	my $size = -s $in;
 	my $len = $size;
 	my $code = 200;
-	push @h, 'Content-Type', $type;
+	push @$h, 'Content-Type', $type;
 	if (($env->{HTTP_RANGE} || '') =~ /\bbytes=(\d*)-(\d*)\z/) {
-		($code, $len) = prepare_range($env, $in, \@h, $1, $2, $size);
+		($code, $len) = prepare_range($env, $in, $h, $1, $2, $size);
 		if ($code == 416) {
-			push @h, 'Content-Range', "bytes */$size";
-			return [ 416, \@h, [] ];
+			push @$h, 'Content-Range', "bytes */$size";
+			return [ 416, $h, [] ];
 		}
 	}
-	push @h, 'Content-Length', $len;
+	push @$h, 'Content-Length', $len;
 	my $n = 65536;
-	[ $code, \@h, Plack::Util::inline_object(close => sub { close $in },
+	[ $code, $h, Plack::Util::inline_object(close => sub { close $in },
 		getline => sub {
 			return if $len == 0;
 			$n = $len if $len < $n;
@@ -138,6 +118,30 @@ sub serve_dumb {
 		})]
 }
 
+sub serve_dumb {
+	my ($env, $git, $path) = @_;
+
+	my $h = [];
+	my $type;
+	if ($path =~ m!\Aobjects/[a-f0-9]{2}/[a-f0-9]{38}\z!) {
+		$type = 'application/x-git-loose-object';
+		cache_one_year($h);
+	} elsif ($path =~ m!\Aobjects/pack/pack-[a-f0-9]{40}\.pack\z!) {
+		$type = 'application/x-git-packed-objects';
+		cache_one_year($h);
+	} elsif ($path =~ m!\Aobjects/pack/pack-[a-f0-9]{40}\.idx\z!) {
+		$type = 'application/x-git-packed-objects-toc';
+		cache_one_year($h);
+	} elsif ($path =~ /\A(?:$TEXT)\z/o) {
+		$type = 'text/plain';
+		push @$h, @no_cache;
+	} else {
+		return r(404);
+	}
+
+	static_result($env, $h, "$git->{git_dir}/$path", $type);
+}
+
 sub prepare_range {
 	my ($env, $in, $h, $beg, $end, $size) = @_;
 	my $code = 200;
-- 
EW

[PATCH 3/5] config: support more cgit directives for project lists

[Eric Wong]

Hopefully this gets us closer to matching cgit upstream behavior
(which also lacks tests).  We'll still need to support macro
expansion at some point for compatibility...
---
 lib/PublicInbox/Config.pm | 95 ++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 73 insertions(+), 22 deletions(-)

diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm
index 2c1c511..2e6f493 100644
--- a/lib/PublicInbox/Config.pm
+++ b/lib/PublicInbox/Config.pm
@@ -200,12 +200,33 @@ sub valid_inbox_name ($) {
 	1;
 }
 
-sub cgit_repo_merge ($$) {
-	my ($self, $repo) = @_;
-	# $repo = { url => 'foo.git', path => '/path/to/foo.git' }
-	my $nick = $repo->{url};
-	$self->{"coderepo.$nick.dir"} ||= $repo->{path};
-	$self->{"coderepo.$nick.cgiturl"} ||= $nick;
+# XXX needs testing for cgit compatibility
+# cf. cgit/scan-tree.c::add_repo
+sub cgit_repo_merge ($$$) {
+	my ($self, $base, $repo) = @_;
+	my $path = $repo->{dir};
+	if (defined(my $se = $self->{-cgit_strict_export})) {
+		return unless -e "$path/$se";
+	}
+	return if -e "$path/noweb";
+	# $repo = { url => 'foo.git', dir => '/path/to/foo.git' }
+	my $rel = $repo->{url};
+	unless (defined $rel) {
+		my $off = index($path, $base, 0);
+		if ($off != 0) {
+			$rel = $path;
+		} else {
+			$rel = substr($path, length($base) + 1);
+		}
+
+		$rel =~ s!/\.git\z!! or
+			$rel =~ s!/+\z!!;
+
+		$self->{-cgit_remove_suffix} and
+			$rel =~ s!/?\.git\z!!;
+	}
+	$self->{"coderepo.$rel.dir"} ||= $path;
+	$self->{"coderepo.$rel.cgiturl"} ||= $rel;
 }
 
 sub is_git_dir ($) {
@@ -213,22 +234,43 @@ sub is_git_dir ($) {
 	-d "$git_dir/objects" && -f "$git_dir/HEAD";
 }
 
+# XXX needs testing for cgit compatibility
 sub scan_path_coderepo {
 	my ($self, $base, $path) = @_;
-	opendir my $dh, $path or return;
+	opendir(my $dh, $path) or do {
+		warn "error opening directory: $path\n";
+		return
+	};
+	my $git_dir = $path;
+	if (is_git_dir($git_dir) || is_git_dir($git_dir .= '/.git')) {
+		my $repo = { dir => $git_dir };
+		cgit_repo_merge($self, $base, $repo);
+		return;
+	}
 	while (defined(my $dn = readdir $dh)) {
 		next if $dn eq '.' || $dn eq '..';
 		if (index($dn, '.') == 0 && !$self->{-cgit_scan_hidden_path}) {
 			next;
 		}
-		my $nick = $base eq '' ? $dn : "$base/$dn";
-		my $git_dir = "$path/$dn";
-		if (is_git_dir($git_dir)) {
-			my $repo = { url => $nick, path => $git_dir };
-			cgit_repo_merge($self, $repo);
-		} elsif (-d $git_dir) {
-			scan_path_coderepo($self, $nick, $git_dir);
-		}
+		my $dir = "$path/$dn";
+		scan_path_coderepo($self, $base, $dir) if -d $dir;
+	}
+}
+
+sub scan_tree_coderepo ($$) {
+	my ($self, $path) = @_;
+	scan_path_coderepo($self, $path, $path);
+}
+
+sub scan_projects_coderepo ($$$) {
+	my ($self, $list, $path) = @_;
+	open my $fh, '<', $list or do {
+		warn "failed to open cgit projectlist=$list: $!\n";
+		return;
+	};
+	foreach (<$fh>) {
+		chomp;
+		scan_path_coderepo($self, $path, "$path/$_");
 	}
 }
 
@@ -255,7 +297,7 @@ sub parse_cgitrc {
 		chomp;
 		if (m!\Arepo\.url=(.+?)/*\z!) {
 			my $nick = $1;
-			cgit_repo_merge($self, $repo) if $repo;
+			cgit_repo_merge($self, $repo->{path}, $repo) if $repo;
 			$repo = { url => $nick };
 		} elsif (m!\Arepo\.path=(.+)\z!) {
 			if (defined $repo) {
@@ -265,17 +307,26 @@ sub parse_cgitrc {
 			}
 		} elsif (m!\Ainclude=(.+)\z!) {
 			parse_cgitrc($self, $1, $nesting + 1);
-		} elsif (m!\Ascan-hidden-path=(\d+)\z!) {
-			$self->{-cgit_scan_hidden_path} = $1;
+		} elsif (m!\A(scan-hidden-path|remove-suffix)=(\d+)\z!) {
+			my ($k, $v) = ($1, $2);
+			$k =~ tr/-/_/;
+			$self->{"-cgit_$k"} = $v;
+		} elsif (m!\A(project-list|strict-export)=(.+)\z!) {
+			my ($k, $v) = ($1, $2);
+			$k =~ tr/-/_/;
+			$self->{"-cgit_$k"} = $v;
 		} elsif (m!\Ascan-path=(.+)\z!) {
-			scan_path_coderepo($self, '', $1);
-
+			if (defined(my $list = $self->{-cgit_project_list})) {
+				scan_projects_coderepo($self, $list, $1);
+			} else {
+				scan_tree_coderepo($self, $1);
+			}
 		} elsif (m!\A(?:css|favicon|logo|repo\.logo)=(/.+)\z!) {
 			# absolute paths for static files via PublicInbox::Cgit
 			$self->{-cgit_static}->{$1} = 1;
 		}
 	}
-	cgit_repo_merge($self, $repo) if $repo;
+	cgit_repo_merge($self, $repo->{path}, $repo) if $repo;
 }
 
 # parse a code repo
@@ -290,7 +341,7 @@ sub _fill_code_repo {
 	}
 	my $dir = $self->{"$pfx.dir"}; # aka "GIT_DIR"
 	unless (defined $dir) {
-		warn "$pfx.dir unset";
+		warn "$pfx.dir unset\n";
 		return;
 	}
 
-- 
EW

[PATCH 6/5] config: fix regression in repo.path => coderepo.dir mapping

[Eric Wong]

We parse cgitrc for "repo.path", while we use "coderepo.dir" to
mean the same thing for non-cgit users.  So I ended up confusing
myself, here.

But then again, git uses "--git-dir" and "GIT_DIR", so I suspect
"dir" is the better choice than "path", here
---
 lib/PublicInbox/Config.pm | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/PublicInbox/Config.pm b/lib/PublicInbox/Config.pm
index 2e6f493..cdc939a 100644
--- a/lib/PublicInbox/Config.pm
+++ b/lib/PublicInbox/Config.pm
@@ -297,11 +297,11 @@ sub parse_cgitrc {
 		chomp;
 		if (m!\Arepo\.url=(.+?)/*\z!) {
 			my $nick = $1;
-			cgit_repo_merge($self, $repo->{path}, $repo) if $repo;
+			cgit_repo_merge($self, $repo->{dir}, $repo) if $repo;
 			$repo = { url => $nick };
 		} elsif (m!\Arepo\.path=(.+)\z!) {
 			if (defined $repo) {
-				$repo->{path} = $1;
+				$repo->{dir} = $1;
 			} else {
 				warn "$_ without repo.url\n";
 			}
@@ -326,7 +326,7 @@ sub parse_cgitrc {
 			$self->{-cgit_static}->{$1} = 1;
 		}
 	}
-	cgit_repo_merge($self, $repo->{path}, $repo) if $repo;
+	cgit_repo_merge($self, $repo->{dir}, $repo) if $repo;
 }
 
 # parse a code repo
-- 
EW

[PATCH 4/5] doc/config: update cgit.cgi scan location

[Eric Wong]

We account for the upstream default location as well as
the Debian-installed one.
---
 Documentation/public-inbox-config.pod | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/public-inbox-config.pod b/Documentation/public-inbox-config.pod
index d6ecd08..e7dcd8a 100644
--- a/Documentation/public-inbox-config.pod
+++ b/Documentation/public-inbox-config.pod
@@ -215,7 +215,7 @@ A path to the C<cgit.cgi> executable.  The L<PublicInbox::WWW>
 interface can spawn cgit as a fallback if the publicinbox.cgitrc
 directive is configured.
 
-Default: /usr/lib/cgit/cgit.cgi
+Default: /var/www/htdocs/cgit/cgit.gti or /usr/lib/cgit/cgit.cgi
 
 =back
 
-- 
EW

[PATCH 5/5] viewdiff: document constants

[Eric Wong]

We'll be building off of this for showing diffs in
the coderepo views.
---
 lib/PublicInbox/ViewDiff.pm | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/PublicInbox/ViewDiff.pm b/lib/PublicInbox/ViewDiff.pm
index a773e38..0cce952 100644
--- a/lib/PublicInbox/ViewDiff.pm
+++ b/lib/PublicInbox/ViewDiff.pm
@@ -15,12 +15,16 @@ use URI::Escape qw(uri_escape_utf8);
 use PublicInbox::Hval qw(ascii_html to_attr from_attr);
 use PublicInbox::Git qw(git_unquote);
 
+# keep track of state so we can avoid redundant HTML tags for
+# identically-classed lines
 sub DSTATE_INIT () { 0 }
 sub DSTATE_STAT () { 1 }
 sub DSTATE_HEAD () { 2 } # /^diff --git /, /^index /, /^--- /, /^\+\+\+ /
 sub DSTATE_CTX () { 3 } # /^ /
 sub DSTATE_ADD () { 4 } # /^\+/
 sub DSTATE_DEL () { 5 } # /^\-/
+
+# maps the DSTATE_* to CSS class names compatible with what cgit uses:
 my @state2class = (
 	'', # init
 	'', # stat
-- 
EW