git-cvsexportcommit fails for huge commits

git-cvsexportcommit fails for huge commits

[Markus Klinik]

Hi,

git-cvsexportcommit fails for huge commits, that is commits with lots of files.
To be exact, the problem arises if the generated 'cvs status' command exceeds
the maximum length for commands.

Here is the error:

Can't exec "cvs": Argument list too long at /home/mkl/bin/git-cvsexportcommit
line 334.  Argument list too long 0 at /home/mkl/bin/git-cvsexportcommit line
334.  cvs status [snip: long, long list of files]  1792 at
/home/mkl/bin/git-cvsexportcommit line 332.

The complete error message was 334523 characters long before I snipped the
files.

Used version is from public git repository, Tue Dec 11 20:59:01 CET 2007
(bf82a15095ed374496c2e98b6b672aa8c8c4d034).

Re: git-cvsexportcommit fails for huge commits

[Jeff King]

On Tue, Dec 11, 2007 at 09:04:18PM +0100, Markus Klinik wrote:

> git-cvsexportcommit fails for huge commits, that is commits with lots
> of files.  To be exact, the problem arises if the generated 'cvs
> status' command exceeds the maximum length for commands.
> 
> Here is the error:
> 
> Can't exec "cvs": Argument list too long at
> /home/mkl/bin/git-cvsexportcommit line 334.  Argument list too long 0
> at /home/mkl/bin/git-cvsexportcommit line 334.  cvs status [snip:
> long, long list of files]  1792 at /home/mkl/bin/git-cvsexportcommit
> line 332.

Yuck. Unfortunately, CVS doesn't offer a more scalable interface, so we
are stuck splitting the arguments across multiple invocations.

However, I think this should work. The output of "cvs status foo bar" is
the same as "cvs status foo; cvs status bar". We will make our commits
in two CVS invocations, but since CVS isn't atomic _anyway_, we
shouldn't mind losing the atomicity.

Does the patch below clear up your problem?

---
diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index 92e4162..20e432b 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,6 +335,22 @@ sub safe_pipe_capture {
     return wantarray ? @output : join('',@output);
 }
 
+sub xargs_safe_pipe_capture {
+	my $MAX_ARG_LENGTH = 1024;
+	my $cmd = shift;
+	my @output;
+	while(@_) {
+		my @args;
+		my $length = 0;
+		while(@_ && $length < $MAX_ARG_LENGTH) {
+			push @args, shift;
+			$length += length($args[$#args]);
+		}
+		push @output, safe_pipe_capture(@$cmd, @args);
+	}
+	return @output;
+}
+
 sub safe_pipe_capture_blob {
     my $output;
     if (my $pid = open my $child, '-|') {

Re: git-cvsexportcommit fails for huge commits

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> @@ -335,6 +335,22 @@ sub safe_pipe_capture {
>      return wantarray ? @output : join('',@output);
>  }
>  
> +sub xargs_safe_pipe_capture {
> +	my $MAX_ARG_LENGTH = 1024;
> +	my $cmd = shift;
> +	my @output;
> +	while(@_) {
> +		my @args;
> +		my $length = 0;
> +		while(@_ && $length < $MAX_ARG_LENGTH) {
> +			push @args, shift;
> +			$length += length($args[$#args]);
> +		}
> +		push @output, safe_pipe_capture(@$cmd, @args);
> +	}
> +	return @output;
> +}
> +

Makes me wonder why you are not spawning xargs by doing it by hand.  If
the path at the beginning happens to be longer than 1024 then you will
run path-less "cvs status"?

Re: git-cvsexportcommit fails for huge commits

[Jeff King]

On Wed, Dec 12, 2007 at 01:21:14AM -0800, Junio C Hamano wrote:

> > +sub xargs_safe_pipe_capture {
> > +	my $MAX_ARG_LENGTH = 1024;
> > +	my $cmd = shift;
> > +	my @output;
> > +	while(@_) {
> > +		my @args;
> > +		my $length = 0;
> > +		while(@_ && $length < $MAX_ARG_LENGTH) {
> > +			push @args, shift;
> > +			$length += length($args[$#args]);
> > +		}
> > +		push @output, safe_pipe_capture(@$cmd, @args);
> > +	}
> > +	return @output;
> > +}
> > +
> 
> Makes me wonder why you are not spawning xargs by doing it by hand.  If

Because we are reading the output, and it is possible to get a pipe
deadlock. This could be avoided with a tempfile.

> the path at the beginning happens to be longer than 1024 then you will
> run path-less "cvs status"?

No, read the loop again. The length starts at 0, so we always go through
the loop body once.

-Peff

Re: git-cvsexportcommit fails for huge commits

[Linus Torvalds]

On Wed, 12 Dec 2007, Jeff King wrote:
>  
> +sub xargs_safe_pipe_capture {
> +	my $MAX_ARG_LENGTH = 1024;

Well, that's a bit extreme. Make it 16kB or something. Anything should be 
able to handle that.

Btw, on Linux, the argument length is realy only limited by the stack size 
limits these days (you have to have a fairly recent kernel, though). It's 
limited to stack limit / 4, to be exact, iirc. So if you see these kinds 
of problems, and are running a recent kernel, do something like

	ulimit -s 65536

to give yourself a big stack and you can continue without these kinds of 
changes..

		Linus

Re: git-cvsexportcommit fails for huge commits

[Jeff King]

On Wed, Dec 12, 2007 at 08:02:35AM -0800, Linus Torvalds wrote:

> On Wed, 12 Dec 2007, Jeff King wrote:
> >  
> > +sub xargs_safe_pipe_capture {
> > +	my $MAX_ARG_LENGTH = 1024;
> 
> Well, that's a bit extreme. Make it 16kB or something. Anything should be 
> able to handle that.

Obviously it should be tweaked, and I just chose an absurdly low value.
However, "xargs --show-limit" claims that the POSIX minimum is 2048,
less the size of the environment. I have no idea what the original
problem reporter's platform is, or what sort of environment size is sane
there.

-Peff

Re: git-cvsexportcommit fails for huge commits

[Martin Langhoff]

On Dec 12, 2007 9:31 PM, Jeff King <peff@peff.net> wrote:
> We will make our commits
> in two CVS invocations, but since CVS isn't atomic _anyway_, we
> shouldn't mind losing the atomicity.

Quick note -- I used to understand that cvs was not atomic, but
reading the on-the-wire protocol has taught me otherwise. Modern
versions of cvs are actually quite atomic-ish -- the protocol expects
the client to give all the relevant data to the server, and then say
"yep, that was all", and only _then_ the server does its commit.

So if the client dies or cancels along the way, nothing ever happens
on the server side.

Still, I suspect that the _server_ is not atomic, so if the server
process dies or finds a problem along the way, you could end up with a
half-commit in the repository, and maybe some hosed ,v files.

IOWs, the protocol *is* atomic, and this patch does make things
slightly more brittle. Perhaps require an option to be set before we
do this?


m

Re: git-cvsexportcommit fails for huge commits

[Jeff King]

On Thu, Dec 13, 2007 at 08:58:33AM +1300, Martin Langhoff wrote:

> IOWs, the protocol *is* atomic, and this patch does make things
> slightly more brittle. Perhaps require an option to be set before we
> do this?

I started writing a patch to let the user specify the limit, but it was
just too ugly. What user knows the right limit? We are probably better
off just setting the limit at something high and reasonable (like 64K --
it would be nice to get feedback from Markus on what platform he is
using and what is a reasonable value), or just using a tempfile with
xargs, which should figure out the correct value.

-Peff

Re: git-cvsexportcommit fails for huge commits

[Markus Klinik]

On Wed, Dec 12, 2007 at 11:17:37PM -0500, Jeff King wrote:
> it would be nice to get feedback from Markus on what platform he is
> using and what is a reasonable value), or just using a tempfile with
> xargs, which should figure out the correct value.

My platform is an ubuntu 7.04.

$ uname -a 
Linux mkl-desktop 2.6.20-16-386 #2 Sun Sep 23 19:47:10 UTC 2007 i686
GNU/Linux
$ ulimit -s
8192

I wasn't able to verify if the patch or increasing the stack limit works
because I stumbled upon what seems to be this problem:
http://marc.info/?l=git&m=119419453726664&w=2

There is already a patch for the cvs-status-fileorder bug but it doesn't
apply and obviously never made it to the public reopsitory.
http://marc.info/?l=git&m=118718448305647&w=2

At the end of this message is a session log that reproduces this
behaviour. The session uses git-cvsexportcommit without the xargs-patch.

If my understanding is correct the 'cvs status' query is just to make
sure everything is up to date, and can be overridden by the -f flag.
I'll try that today and report if the xargs-patch and the stack limit
thing worked.

-Markus

mkl@mkl-desktop:~/tmp$ # ---------------------- begin
mkl@mkl-desktop:~$ cvs --version

Concurrent Versions System (CVS) 1.12.13 (client/server)

Copyright (C) 2005 Free Software Foundation, Inc.

Senior active maintainers include Larry Jones, Derek R. Price,
and Mark D. Baushke.  Please see the AUTHORS and README files from the
CVS
distribution kit for a complete list of contributors and copyrights.

CVS may be copied only under the terms of the GNU General Public
License,
a copy of which can be found with the CVS distribution kit.

Specify the --help option for further information about CVS
mkl@mkl-desktop:~$ git --version
git version 1.5.3.7.1157.gbf82a
mkl@mkl-desktop:~/tmp$ mkdir ~/cvsroot
mkl@mkl-desktop:~/tmp$ export CVSROOT=~/cvsroot
mkl@mkl-desktop:~/tmp$ cvs init
mkl@mkl-desktop:~/tmp$ mkdir cfoo
mkl@mkl-desktop:~/tmp$ cd cfoo/
mkl@mkl-desktop:~/tmp/cfoo$ cvs import -m'import' cfoo A AA

No conflicts created by this import

mkl@mkl-desktop:~/tmp/cfoo$ cd ..
mkl@mkl-desktop:~/tmp$ mkdir gfoo
mkl@mkl-desktop:~/tmp$ cd gfoo/
mkl@mkl-desktop:~/tmp/gfoo$ git-init
Initialized empty Git repository in .git/
mkl@mkl-desktop:~/tmp/gfoo$ mkdir bar
mkl@mkl-desktop:~/tmp/gfoo$ touch a b c d bar/a bar/b bar/e
mkl@mkl-desktop:~/tmp/gfoo$ git add .
mkl@mkl-desktop:~/tmp/gfoo$ git commit -m'empty'
Created initial commit a11f9bc: empty
 0 files changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 a
 create mode 100644 b
 create mode 040000 bar
 create mode 100644 c
 create mode 100644 d
mkl@mkl-desktop:~/tmp/gfoo$ echo 'hi' > 0
mkl@mkl-desktop:~/tmp/gfoo$ echo 'hi' > a
mkl@mkl-desktop:~/tmp/gfoo$ echo 'hi' > bar/e
mkl@mkl-desktop:~/tmp/gfoo$ echo 'hi' > e
mkl@mkl-desktop:~/tmp/gfoo$ git add .
mkl@mkl-desktop:~/tmp/gfoo$ git commit -m 'hi'
Created commit e71a144: hi
 3 files changed, 3 insertions(+), 0 deletions(-)
 create mode 100644 0
 create mode 100644 e
mkl@mkl-desktop:~/tmp/gfoo$ cd ..
mkl@mkl-desktop:~/tmp$ cvs co cfoo
cvs checkout: Updating cfoo
mkl@mkl-desktop:~/tmp$ cd cfoo
mkl@mkl-desktop:~/tmp/cfoo$ export GIT_DIR=../gfoo/.git
mkl@mkl-desktop:~/tmp/cfoo$ git-cvsexportcommit -c HEAD^ 
Checking if patch will apply
cvs status: nothing known about `a'
cvs status: nothing known about `b'
cvs status: nothing known about `c'
cvs status: nothing known about `d'
Applying
Patch applied successfully. Adding new files and directories to CVS
Directory /home/mkl/cvsroot/cfoo/bar added to the repository
cvs add: scheduling file `a' for addition
cvs add: use `cvs commit' to add this file permanently
cvs add: scheduling file `b' for addition
cvs add: use `cvs commit' to add this file permanently
cvs add: scheduling file `bar/a' for addition
cvs add: use `cvs commit' to add this file permanently
cvs add: scheduling file `bar/b' for addition
cvs add: use `cvs commit' to add this file permanently
cvs add: scheduling file `bar/e' for addition
cvs add: use `cvs commit' to add this file permanently
cvs add: scheduling file `c' for addition
cvs add: use `cvs commit' to add this file permanently
cvs add: scheduling file `d' for addition
cvs add: use `cvs commit' to add this file permanently
Commit to CVS
Patch title (first comment line): empty
Autocommit
  cvs commit -F .msg 'a' 'b' 'bar/a' 'bar/b' 'bar/e' 'c' 'd'
/home/mkl/cvsroot/cfoo/a,v  <--  a
initial revision: 1.1
/home/mkl/cvsroot/cfoo/b,v  <--  b
initial revision: 1.1
/home/mkl/cvsroot/cfoo/c,v  <--  c
initial revision: 1.1
/home/mkl/cvsroot/cfoo/d,v  <--  d
initial revision: 1.1
/home/mkl/cvsroot/cfoo/bar/a,v  <--  bar/a
initial revision: 1.1
/home/mkl/cvsroot/cfoo/bar/b,v  <--  bar/b
initial revision: 1.1
/home/mkl/cvsroot/cfoo/bar/e,v  <--  bar/e
initial revision: 1.1
Committed successfully to CVS
mkl@mkl-desktop:~/tmp/cfoo$ git-cvsexportcommit -c HEAD
Checking if patch will apply
cvs status: nothing known about `0'
cvs status: nothing known about `e'
File e is already known in your CVS checkout -- perhaps it has been added by another user. Or this may indicate that it exists on a different branch. If this is the case, use -f to force the merge.
Status was: Up-to-date
File bar/e not up to date but has status 'Unknown' in your CVS checkout!
Exiting: your CVS tree is not clean for this merge. at /home/mkl/bin/git-cvsexportcommit line 235.
mkl@mkl-desktop:~/tmp/cfoo$ # ----------- end

Re: git-cvsexportcommit fails for huge commits

[Jeff King]

On Thu, Dec 13, 2007 at 09:39:31AM +0100, Markus Klinik wrote:

> > it would be nice to get feedback from Markus on what platform he is
> > using and what is a reasonable value), or just using a tempfile with
> > xargs, which should figure out the correct value.
> 
> My platform is an ubuntu 7.04.

In that case, I think a very large value like 100K is fine. Then the code
won't activate unless it is really required.

It still feels a bit hack-ish and wrong, but I'm not sure what would be
better.

-Peff

Re: git-cvsexportcommit fails for huge commits

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> the path at the beginning happens to be longer than 1024 then you will
>> run path-less "cvs status"?
>
> No, read the loop again. The length starts at 0, so we always go through
> the loop body once.

Sorry, you are right.

Perhaps pick a reasonably small but not insanely small value, like 16kB,
forget about the atomicity issues for now, as an interim improvement
patch?

Re: git-cvsexportcommit fails for huge commits

[Jeff King]

On Thu, Dec 13, 2007 at 07:22:43PM -0800, Junio C Hamano wrote:

> Sorry, you are right.
> 
> Perhaps pick a reasonably small but not insanely small value, like 16kB,
> forget about the atomicity issues for now, as an interim improvement
> patch?

I'm fine with that. We can probably go a bit higher than that. From my
limited testing[1]:

  Linux 2.6.18: ~128K
  Linux 2.6.23: huge? I tried ~350K and it worked fine
  Solaris: huge? I tried ~350K and it worked fine
  Freebsd 6.1: ~256K

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

-Peff

[1] All numbers are approximate and determined experimentally with
something like:
  for i in `seq 1 $n`; do
    touch $long_filename-$i
  done
  ls * | wc

Re: git-cvsexportcommit fails for huge commits

[Jeff King]

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

Re: git-cvsexportcommit fails for huge commits

[Robin Rosenberg]

fredag 14 december 2007 skrev Jeff King:
> 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.

Wouldn't using the POSIX::ARG_MAX constant work?

-- robin

Re: git-cvsexportcommit fails for huge commits

[Jeff King]

On Fri, Dec 14, 2007 at 02:47:03PM +0100, Robin Rosenberg wrote:

> > 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.
> 
> Wouldn't using the POSIX::ARG_MAX constant work?

It does seem to produce sensible results. Does it work reasonably on
Windows?

Note that it would also need to be ARG_MAX - slop, where slop accounts
for the environment (which maybe we can get accurately by counting the
keys and values of %ENV, adding NUL terminators?).

-Peff