[PATCH 0/4] File::Spec-related things

[PATCH 0/4] File::Spec-related things

[Eric Wong]

1/4 fixes a real problem in rare cases, while the rest are
relatively minor improvements.

Eric Wong (4):
  cindex: rework path canonicalization check
  www_coderepo: drop unused File::Spec import
  hval: use File::Spec::Functions::abs2rel
  lei_mirror: favor File::Spec::Functions

 lib/PublicInbox/CodeSearchIdx.pm | 20 ++++++++++----------
 lib/PublicInbox/Hval.pm          |  4 ++--
 lib/PublicInbox/LeiMirror.pm     | 19 +++++++++----------
 lib/PublicInbox/WwwCoderepo.pm   |  1 -
 4 files changed, 21 insertions(+), 23 deletions(-)

[PATCH 1/4] cindex: rework path canonicalization check

[Eric Wong]

While reading the code, I noticed inadvertant `$_' use when the
loop iterator is  `$d'.  Using `$_' here would result in
uninitialized variable access.  I've yet to hit this case in
real-world access.

Furthermore, we can use a single pass to canonicalize existing
directories instead of relying on a grep block, first.

Finally, favor File::Spec::Functions since `->' method dispatch
is slower than normal subroutine calls by a small amount even
when both the package and method names are static and known
early in advance..
---
 lib/PublicInbox/CodeSearchIdx.pm | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/lib/PublicInbox/CodeSearchIdx.pm b/lib/PublicInbox/CodeSearchIdx.pm
index ff3db8ba..13533a00 100644
--- a/lib/PublicInbox/CodeSearchIdx.pm
+++ b/lib/PublicInbox/CodeSearchIdx.pm
@@ -56,7 +56,7 @@ use PublicInbox::PktOp;
 use PublicInbox::IPC qw(nproc_shards);
 use POSIX qw(WNOHANG SEEK_SET strftime);
 use File::Path ();
-use File::Spec ();
+use File::Spec::Functions qw(canonpath);
 use List::Util qw(max);
 use PublicInbox::SHA qw(sha256_hex sha_all);
 use PublicInbox::Search qw(xap_terms);
@@ -1341,15 +1341,15 @@ sub cidx_run { # main entry point
 		delete $REINDEX->{lock_path};
 		$REINDEX->dbh;
 	}
-	my @nc = grep { File::Spec->canonpath($_) ne $_ } @{$self->{git_dirs}};
-	if (@nc) {
-		warn "E: BUG? paths in $self->{cidx_dir} not canonicalized:\n";
-		for my $d (@{$self->{git_dirs}}) {
-			my $c = File::Spec->canonpath($_);
-			warn "E: $d => $c\n";
-			$d = $c;
-		}
-		warn "E: canonicalized and attempting to continue\n";
+	my $cwarn;
+	for my $d (@{$self->{git_dirs}}) {
+		my $c = canonpath $d;
+		next if $c eq $d;
+		$cwarn = warn <<EOM if !$cwarn;
+E: BUG? paths in $self->{cidx_dir} not canonicalized:
+EOM
+		warn "E: $d => $c\n";
+		$d = $c;
 	}
 	if (defined(my $excl = $self->{-opt}->{exclude})) {
 		my $re = '(?:'.join('\\z|', map {

[PATCH 2/4] www_coderepo: drop unused File::Spec import

[Eric Wong]

I initially thought File::Spec->abs2rel would be used in this
file, but it turns out `prurl' is a better place to call abs2rel.
---
 lib/PublicInbox/WwwCoderepo.pm | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/PublicInbox/WwwCoderepo.pm b/lib/PublicInbox/WwwCoderepo.pm
index 413dfee5..8be8ada0 100644
--- a/lib/PublicInbox/WwwCoderepo.pm
+++ b/lib/PublicInbox/WwwCoderepo.pm
@@ -22,7 +22,6 @@ use PublicInbox::RepoTree;
 use PublicInbox::RepoList;
 use PublicInbox::OnDestroy;
 use URI::Escape qw(uri_escape_utf8);
-use File::Spec;
 use autodie qw(fcntl open);
 use PublicInbox::Git qw(git_exe);
 

[PATCH 3/4] hval: use File::Spec::Functions::abs2rel

[Eric Wong]

Function calls are faster than method dispatch and abs2rel() may
be called hundreds/thousands of times when listing associated
inboxes||coderepos.
---
 lib/PublicInbox/Hval.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/PublicInbox/Hval.pm b/lib/PublicInbox/Hval.pm
index 963dbb71..d44b6562 100644
--- a/lib/PublicInbox/Hval.pm
+++ b/lib/PublicInbox/Hval.pm
@@ -13,7 +13,7 @@ our @EXPORT_OK = qw/ascii_html obfuscate_addrs to_filename src_escape
 		to_attr prurl mid_href fmt_ts ts2str utf8_maybe/;
 use POSIX qw(strftime);
 my $enc_ascii = find_encoding('us-ascii');
-use File::Spec;
+use File::Spec::Functions qw(abs2rel);
 
 # safe-ish acceptable filename pattern for portability
 our $FN = '[a-zA-Z0-9][a-zA-Z0-9_\-\.]+[a-zA-Z0-9]'; # needs \z anchor
@@ -76,7 +76,7 @@ sub prurl ($$) {
 	} elsif ($dslash < 0 && substr($u, 0, 1) ne '/' &&
 			substr(my $path = $env->{PATH_INFO}, 0, 1) eq '/') {
 		# this won't touch the FS at all:
-		File::Spec->abs2rel("/$u", $path);
+		abs2rel "/$u", $path;
 	} else {
 		$u;
 	}

[PATCH 4/4] lei_mirror: favor File::Spec::Functions

[Eric Wong]

Functions calls are preferable over `->' method dispatch in
tight loops.  This can be the case when scanning alternates in
v1_done().  Since we're at it, replace all other `File::Spec->'
method dispatches with function calls since function calls can
be used to validate function prototypes at compile time.
---
 lib/PublicInbox/LeiMirror.pm | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/lib/PublicInbox/LeiMirror.pm b/lib/PublicInbox/LeiMirror.pm
index fb0295da..f87cdc51 100644
--- a/lib/PublicInbox/LeiMirror.pm
+++ b/lib/PublicInbox/LeiMirror.pm
@@ -11,7 +11,7 @@ use PublicInbox::Spawn qw(spawn run_wait run_die run_qx);
 use PublicInbox::IO qw(write_file);
 use File::Path ();
 use File::Temp ();
-use File::Spec ();
+use File::Spec::Functions qw(abs2rel rel2abs splitpath);
 use Fcntl qw(SEEK_SET O_CREAT O_EXCL O_WRONLY);
 use Carp qw(croak);
 use URI;
@@ -502,9 +502,9 @@ EOM
 EOM
 	}
 	if (!$self->{dry_run}) {
-		my $alt = File::Spec->rel2abs("$dir/objects");
+		my $alt = rel2abs "$dir/objects";
 		my $o = "$self->{cur_dst}/objects";
-		my $l = File::Spec->abs2rel($alt, File::Spec->rel2abs($o));
+		my $l = abs2rel $alt, rel2abs($o);
 		open my $fh, '+>>', my $f = "$o/info/alternates";
 		seek($fh, 0, SEEK_SET); # Perl did SEEK_END when it saw '>>'
 		my $seen = grep /\A\Q$l\E\n/, PublicInbox::IO::read_all $fh;
@@ -780,15 +780,14 @@ sub update_ent {
 		start_cmd($self, $cmd, { 2 => $self->{lei}->{2} }) if $cmd;
 	}
 	if (my $symlinks = $self->{-ent}->{symlinks}) {
-		my $top = File::Spec->rel2abs($self->{dst});
+		my $top = rel2abs $self->{dst};
 		push @{$self->{-new_symlinks}}, @$symlinks;
 		for my $p (@$symlinks) {
 			my $ln = "$top/$p";
 			$ln =~ tr!/!/!s;
-			my (undef, $dn, $bn) = File::Spec->splitpath($ln);
+			my (undef, $dn, $bn) = splitpath $ln;
 			File::Path::mkpath($dn);
-			my $tgt = "$top/$key";
-			$tgt = File::Spec->abs2rel($tgt, $dn);
+			my $tgt = abs2rel "$top/$key", $dn;
 			if (lstat($ln)) {
 				if (-l _) {
 					next if readlink($ln) eq $tgt;
@@ -828,12 +827,12 @@ sub v1_done { # called via OnDestroy
 	update_ent($self) if $self->{-ent};
 	my $o = "$dst/objects";
 	if (CORE::open(my $fh, '<', my $fn = "$o/info/alternates")) {;
-		my $base = File::Spec->rel2abs($o);
+		my $base = rel2abs $o;
 		my @l = <$fh>;
 		my $ft;
 		for (@l) {
 			next unless m!\A/!;
-			$_ = File::Spec->abs2rel($_, $base);
+			$_ = abs2rel $_, $base;
 			$ft //= File::Temp->new(TEMPLATE => '.XXXX',
 						DIR => "$o/info");
 		}
@@ -1123,7 +1122,7 @@ EOM
 	}
 	$self->{chg}->{nr_chg} += scalar(@remote) + scalar(@local);
 	return if !defined($f) || $self->{dry_run};
-	my (undef, $dn, $bn) = File::Spec->splitpath($f);
+	my (undef, $dn, $bn) = splitpath $f;
 	my $new = join("\n", @list, '');
 	atomic_write($dn, $bn, $new) if $new ne $old;
 }