git-cvsexportcommit fails for huge commits

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).

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, '-|') {

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

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

On Wed, 12 Dec 2007, Jeff King wrote:

quoted

 
+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

On Dec 12, 2007 9:31 PM, Jeff King [off-list ref] 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

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

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

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

quoted

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