Re: git-cvsexportcommit fails for huge commits

[Jeff King <peff@peff.net>]

On Thu, Dec 13, 2007 at 11:45:54PM -0500, Jeff King wrote:

> So it seems that we could probably go with something more like 64K, and
> then only truly pathological cases should trigger the behavior.

So here is a cleaned up patch. It bumps the maximum size to 64kB, adds
scalar support (nobody uses it, but it makes sense for the interface to
match that of safe_pipe_capture -- I am even tempted to just replace
safe_pipe_capture entirely and convert the few other callers), and
cleans up the unused safe_pipe_capture_blob.

-- >8 --
cvsexportcommit: fix massive commits

Because we feed the changed filenames to CVS on the command
line, it was possible for massive commits to overflow the
system exec limits. Instead, we now do an xargs-like split
of the arguments.

This means that we lose some of the atomicity of calling CVS
in one shot. Since CVS commits are not atomic, but the CVS
protocol is, the possible effects of this are not clear;
however, since CVS doesn't provide a different interface,
this is our only option for large commits (short of writing
a CVS client library).

The argument size limit is arbitrarily set to 64kB. This
should be high enough to trigger only in rare cases where it
is necessary, so normal-sized commits are not affected by
the atomicity change.
---
I think the atomicity might matter if you are using cvsexportcommit to
talk to a CVS server backed by something besides CVS, like
git-cvsserver. Which of course would be useless, but who is to say
that other such systems don't exist with CVS as an SCM lingua franca?

 git-cvsexportcommit.perl |   37 +++++++++++++++++++++++--------------
 1 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index 92e4162..d2e50c3 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -195,11 +195,11 @@ foreach my $f (@files) {
 my %cvsstat;
 if (@canstatusfiles) {
     if ($opt_u) {
-      my @updated = safe_pipe_capture(@cvs, 'update', @canstatusfiles);
+      my @updated = xargs_safe_pipe_capture([@cvs, 'update'], @canstatusfiles);
       print @updated;
     }
     my @cvsoutput;
-    @cvsoutput= safe_pipe_capture(@cvs, 'status', @canstatusfiles);
+    @cvsoutput = xargs_safe_pipe_capture([@cvs, 'status'], @canstatusfiles);
     my $matchcount = 0;
     foreach my $l (@cvsoutput) {
         chomp $l;
@@ -295,7 +295,7 @@ if ($dirtypatch) {
 
 if ($opt_c) {
     print "Autocommit\n  $cmd\n";
-    print safe_pipe_capture(@cvs, 'commit', '-F', '.msg', @files);
+    print xargs_safe_pipe_capture([@cvs, 'commit', '-F', '.msg'], @files);
     if ($?) {
 	die "Exiting: The commit did not succeed";
     }
@@ -335,15 +335,24 @@ sub safe_pipe_capture {
     return wantarray ? @output : join('',@output);
 }
 
-sub safe_pipe_capture_blob {
-    my $output;
-    if (my $pid = open my $child, '-|') {
-        local $/;
-	undef $/;
-	$output = (<$child>);
-	close $child or die join(' ',@_).": $! $?";
-    } else {
-	exec(@_) or die "$! $?"; # exec() can fail the executable can't be found
-    }
-    return $output;
+sub xargs_safe_pipe_capture {
+	my $MAX_ARG_LENGTH = 65536;
+	my $cmd = shift;
+	my @output;
+	my $output;
+	while(@_) {
+		my @args;
+		my $length = 0;
+		while(@_ && $length < $MAX_ARG_LENGTH) {
+			push @args, shift;
+			$length += length($args[$#args]);
+		}
+		if (wantarray) {
+			push @output, safe_pipe_capture(@$cmd, @args);
+		}
+		else {
+			$output .= safe_pipe_capture(@$cmd, @args);
+		}
+	}
+	return wantarray ? @output : $output;
 }
-- 
1.5.4.rc0.1088.g8a30-dirty