user/dev discussion of public-inbox itself
 help / color / mirror / code / Atom feed
From: Eric Wong <e@80x24.org>
To: meta@public-inbox.org
Subject: [PATCH] git: remove cat_file sub callback interface
Date: Thu, 13 Jun 2019 08:10:02 +0000	[thread overview]
Message-ID: <20190613081002.19309-1-e@80x24.org> (raw)

We weren't using it, and in retrospect, it makes no sense to use
this API cat_file for giant responses which can't read quickly
with minimal context-switching (or sanely fit into memory for
Email::Simple/Email::MIME).

For giant blobs which we don't want slurped in memory, we'll
spawn a short-lived git-cat-file process like we do in ViewVCS.

Otherwise, monopolizing a git-cat-file process for a giant
blob is harmful to other PSGI/NNTP users.

A better interface is coming which will be more suitable for
for batch processing of "small" objects such as commits and
email blobs.
---
 lib/PublicInbox/Git.pm | 43 ++++++++++---------------------
 t/git.t                | 58 ------------------------------------------
 2 files changed, 13 insertions(+), 88 deletions(-)

diff --git a/lib/PublicInbox/Git.pm b/lib/PublicInbox/Git.pm
index 68445b3..6a87661 100644
--- a/lib/PublicInbox/Git.pm
+++ b/lib/PublicInbox/Git.pm
@@ -145,41 +145,24 @@ again:
 		fail($self, "Unexpected result from git cat-file: $head");
 
 	my $size = $1;
-	my $ref_type = $ref ? ref($ref) : '';
-
 	my $rv;
 	my $left = $size;
-	$$ref = $size if ($ref_type eq 'SCALAR');
-	my $cb_err;
-
-	if ($ref_type eq 'CODE') {
-		$rv = eval { $ref->($in, \$left) };
-		$cb_err = $@;
-		# drain the rest
-		my $max = 8192;
-		while ($left > 0) {
-			my $r = read($in, my $x, $left > $max ? $max : $left);
-			defined($r) or fail($self, "read failed: $!");
-			$r == 0 and fail($self, 'exited unexpectedly');
-			$left -= $r;
-		}
-	} else {
-		my $offset = 0;
-		my $buf = '';
-		while ($left > 0) {
-			my $r = read($in, $buf, $left, $offset);
-			defined($r) or fail($self, "read failed: $!");
-			$r == 0 and fail($self, 'exited unexpectedly');
-			$left -= $r;
-			$offset += $r;
-		}
-		$rv = \$buf;
+	$$ref = $size if $ref;
+
+	my $offset = 0;
+	my $buf = '';
+	while ($left > 0) {
+		my $r = read($in, $buf, $left, $offset);
+		defined($r) or fail($self, "read failed: $!");
+		$r == 0 and fail($self, 'exited unexpectedly');
+		$left -= $r;
+		$offset += $r;
 	}
+	$rv = \$buf;
 
-	my $r = read($in, my $buf, 1);
+	my $r = read($in, my $lf, 1);
 	defined($r) or fail($self, "read failed: $!");
-	fail($self, 'newline missing after blob') if ($r != 1 || $buf ne "\n");
-	die $cb_err if $cb_err;
+	fail($self, 'newline missing after blob') if ($r != 1 || $lf ne "\n");
 
 	$rv;
 }
diff --git a/t/git.t b/t/git.t
index 913f6e5..9bc8900 100644
--- a/t/git.t
+++ b/t/git.t
@@ -33,33 +33,7 @@ use_ok 'PublicInbox::Git';
 	my $raw = $gcf->cat_file($f);
 	is($x[2], length($$raw), 'length matches');
 
-	{
-		my $size;
-		my $rv = $gcf->cat_file($f, sub {
-			my ($in, $left) = @_;
-			$size = $$left;
-			'nothing'
-		});
-		is($rv, 'nothing', 'returned from callback without reading');
-		is($size, $x[2], 'set size for callback correctly');
-	}
-
-	eval { $gcf->cat_file($f, sub { die 'OMG' }) };
-	like($@, qr/\bOMG\b/, 'died in callback propagated');
 	is(${$gcf->cat_file($f)}, $$raw, 'not broken after failures');
-
-	{
-		my ($buf, $r);
-		my $rv = $gcf->cat_file($f, sub {
-			my ($in, $left) = @_;
-			$r = read($in, $buf, 2);
-			$$left -= $r;
-			'blah'
-		});
-		is($r, 2, 'only read 2 bytes');
-		is($buf, '--', 'partial read succeeded');
-		is($rv, 'blah', 'return value propagated');
-	}
 	is(${$gcf->cat_file($f)}, $$raw, 'not broken after partial read');
 }
 
@@ -79,44 +53,12 @@ if (1) {
 
 	my $gcf = PublicInbox::Git->new($dir);
 	my $rsize;
-	is($gcf->cat_file($buf, sub {
-		$rsize = ${$_[1]};
-		'x';
-	}), 'x', 'checked input');
-	is($rsize, $size, 'got correct size on big file');
-
 	my $x = $gcf->cat_file($buf, \$rsize);
 	is($rsize, $size, 'got correct size ref on big file');
 	is(length($$x), $size, 'read correct number of bytes');
 
-	my $rline;
-	$gcf->cat_file($buf, sub {
-		my ($in, $left) = @_;
-		$rline = <$in>;
-		$$left -= length($rline);
-	});
-	{
-		open my $fh, '<', $big_data or die "open failed: $!\n";
-		is($rline, <$fh>, 'first line matches');
-	};
-
-	my $all;
-	$gcf->cat_file($buf, sub {
-		my ($in, $left) = @_;
-		my $x = read($in, $all, $$left);
-		$$left -= $x;
-	});
-	{
-		open my $fh, '<', $big_data or die "open failed: $!\n";
-		local $/;
-		is($all, <$fh>, 'entire read matches');
-	};
-
 	my $ref = $gcf->qx(qw(cat-file blob), $buf);
-	is($all, $ref, 'qx read giant single string');
-
 	my @ref = $gcf->qx(qw(cat-file blob), $buf);
-	is($all, join('', @ref), 'qx returned array when wanted');
 	my $nl = scalar @ref;
 	ok($nl > 1, "qx returned array length of $nl");
 
-- 
EW


                 reply	other threads:[~2019-06-13  8:10 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://public-inbox.org/README

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190613081002.19309-1-e@80x24.org \
    --to=e@80x24.org \
    --cc=meta@public-inbox.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).