{"thread":{"id":"11243","subject":"git-cvsexportcommit fails for huge commits","startedAt":"2007-12-11T20:04:18Z","lastAt":"2007-12-15T10:09:50Z","messageCount":15,"participants":["Markus Klinik","Jeff King","Junio C Hamano","Linus Torvalds","Martin Langhoff","Robin Rosenberg"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"62789","messageId":"20071211200418.GA13815@mkl-desktop","threadId":"11243","inReplyTo":null,"subject":"git-cvsexportcommit fails for huge commits","fromName":"Markus Klinik","fromEmail":"markus.klinik@gmx.de","sentAt":"2007-12-11T20:04:18Z","receivedAt":"2007-12-11T20:04:18Z","isPatch":false,"sender":{"key":"markus.klinik@gmx.de","avatar":null},"body":"Hi,\n\ngit-cvsexportcommit fails for huge commits, that is commits with lots of files.\nTo be exact, the problem arises if the generated 'cvs status' command exceeds\nthe maximum length for commands.\n\nHere is the error:\n\nCan't exec \"cvs\": Argument list too long at /home/mkl/bin/git-cvsexportcommit\nline 334.  Argument list too long 0 at /home/mkl/bin/git-cvsexportcommit line\n334.  cvs status [snip: long, long list of files]  1792 at\n/home/mkl/bin/git-cvsexportcommit line 332.\n\nThe complete error message was 334523 characters long before I snipped the\nfiles.\n\nUsed version is from public git repository, Tue Dec 11 20:59:01 CET 2007\n(bf82a15095ed374496c2e98b6b672aa8c8c4d034).\n"},{"id":"62873","messageId":"20071212083154.GB7676@coredump.intra.peff.net","threadId":"11243","inReplyTo":"20071211200418.GA13815@mkl-desktop","subject":"Re: git-cvsexportcommit fails for huge commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-12T08:31:54Z","receivedAt":"2007-12-12T08:31:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 11, 2007 at 09:04:18PM +0100, Markus Klinik wrote:\n\n> git-cvsexportcommit fails for huge commits, that is commits with lots\n> of files.  To be exact, the problem arises if the generated 'cvs\n> status' command exceeds the maximum length for commands.\n> \n> Here is the error:\n> \n> Can't exec \"cvs\": Argument list too long at\n> /home/mkl/bin/git-cvsexportcommit line 334.  Argument list too long 0\n> at /home/mkl/bin/git-cvsexportcommit line 334.  cvs status [snip:\n> long, long list of files]  1792 at /home/mkl/bin/git-cvsexportcommit\n> line 332.\n\nYuck. Unfortunately, CVS doesn't offer a more scalable interface, so we\nare stuck splitting the arguments across multiple invocations.\n\nHowever, I think this should work. The output of \"cvs status foo bar\" is\nthe same as \"cvs status foo; cvs status bar\". We will make our commits\nin two CVS invocations, but since CVS isn't atomic _anyway_, we\nshouldn't mind losing the atomicity.\n\nDoes the patch below clear up your problem?\n\n---\ndiff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl\nindex 92e4162..20e432b 100755\n--- a/git-cvsexportcommit.perl\n+++ b/git-cvsexportcommit.perl\n@@ -195,11 +195,11 @@ foreach my $f (@files) {\n my %cvsstat;\n if (@canstatusfiles) {\n     if ($opt_u) {\n-      my @updated = safe_pipe_capture(@cvs, 'update', @canstatusfiles);\n+      my @updated = xargs_safe_pipe_capture([@cvs, 'update'], @canstatusfiles);\n       print @updated;\n     }\n     my @cvsoutput;\n-    @cvsoutput= safe_pipe_capture(@cvs, 'status', @canstatusfiles);\n+    @cvsoutput = xargs_safe_pipe_capture([@cvs, 'status'], @canstatusfiles);\n     my $matchcount = 0;\n     foreach my $l (@cvsoutput) {\n         chomp $l;\n@@ -295,7 +295,7 @@ if ($dirtypatch) {\n \n if ($opt_c) {\n     print \"Autocommit\\n  $cmd\\n\";\n-    print safe_pipe_capture(@cvs, 'commit', '-F', '.msg', @files);\n+    print xargs_safe_pipe_capture([@cvs, 'commit', '-F', '.msg'], @files);\n     if ($?) {\n \tdie \"Exiting: The commit did not succeed\";\n     }\n@@ -335,6 +335,22 @@ sub safe_pipe_capture {\n     return wantarray ? @output : join('',@output);\n }\n \n+sub xargs_safe_pipe_capture {\n+\tmy $MAX_ARG_LENGTH = 1024;\n+\tmy $cmd = shift;\n+\tmy @output;\n+\twhile(@_) {\n+\t\tmy @args;\n+\t\tmy $length = 0;\n+\t\twhile(@_ && $length < $MAX_ARG_LENGTH) {\n+\t\t\tpush @args, shift;\n+\t\t\t$length += length($args[$#args]);\n+\t\t}\n+\t\tpush @output, safe_pipe_capture(@$cmd, @args);\n+\t}\n+\treturn @output;\n+}\n+\n sub safe_pipe_capture_blob {\n     my $output;\n     if (my $pid = open my $child, '-|') {\n"},{"id":"62881","messageId":"7vir348e0l.fsf@gitster.siamese.dyndns.org","threadId":"11243","inReplyTo":"20071212083154.GB7676@coredump.intra.peff.net","subject":"Re: git-cvsexportcommit fails for huge commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-12T09:21:14Z","receivedAt":"2007-12-12T09:21:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> @@ -335,6 +335,22 @@ sub safe_pipe_capture {\n>      return wantarray ? @output : join('',@output);\n>  }\n>  \n> +sub xargs_safe_pipe_capture {\n> +\tmy $MAX_ARG_LENGTH = 1024;\n> +\tmy $cmd = shift;\n> +\tmy @output;\n> +\twhile(@_) {\n> +\t\tmy @args;\n> +\t\tmy $length = 0;\n> +\t\twhile(@_ && $length < $MAX_ARG_LENGTH) {\n> +\t\t\tpush @args, shift;\n> +\t\t\t$length += length($args[$#args]);\n> +\t\t}\n> +\t\tpush @output, safe_pipe_capture(@$cmd, @args);\n> +\t}\n> +\treturn @output;\n> +}\n> +\n\nMakes me wonder why you are not spawning xargs by doing it by hand.  If\nthe path at the beginning happens to be longer than 1024 then you will\nrun path-less \"cvs status\"?\n"},{"id":"62883","messageId":"20071212092512.GB20799@coredump.intra.peff.net","threadId":"11243","inReplyTo":"7vir348e0l.fsf@gitster.siamese.dyndns.org","subject":"Re: git-cvsexportcommit fails for huge commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-12T09:25:12Z","receivedAt":"2007-12-12T09:25:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 12, 2007 at 01:21:14AM -0800, Junio C Hamano wrote:\n\n> > +sub xargs_safe_pipe_capture {\n> > +\tmy $MAX_ARG_LENGTH = 1024;\n> > +\tmy $cmd = shift;\n> > +\tmy @output;\n> > +\twhile(@_) {\n> > +\t\tmy @args;\n> > +\t\tmy $length = 0;\n> > +\t\twhile(@_ && $length < $MAX_ARG_LENGTH) {\n> > +\t\t\tpush @args, shift;\n> > +\t\t\t$length += length($args[$#args]);\n> > +\t\t}\n> > +\t\tpush @output, safe_pipe_capture(@$cmd, @args);\n> > +\t}\n> > +\treturn @output;\n> > +}\n> > +\n> \n> Makes me wonder why you are not spawning xargs by doing it by hand.  If\n\nBecause we are reading the output, and it is possible to get a pipe\ndeadlock. This could be avoided with a tempfile.\n\n> the path at the beginning happens to be longer than 1024 then you will\n> run path-less \"cvs status\"?\n\nNo, read the loop again. The length starts at 0, so we always go through\nthe loop body once.\n\n-Peff\n"},{"id":"62921","messageId":"alpine.LFD.0.9999.0712120753120.25032@woody.linux-foundation.org","threadId":"11243","inReplyTo":"20071212083154.GB7676@coredump.intra.peff.net","subject":"Re: git-cvsexportcommit fails for huge commits","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-12-12T16:02:35Z","receivedAt":"2007-12-12T16:02:35Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 12 Dec 2007, Jeff King wrote:\n>  \n> +sub xargs_safe_pipe_capture {\n> +\tmy $MAX_ARG_LENGTH = 1024;\n\nWell, that's a bit extreme. Make it 16kB or something. Anything should be \nable to handle that.\n\nBtw, on Linux, the argument length is realy only limited by the stack size \nlimits these days (you have to have a fairly recent kernel, though). It's \nlimited to stack limit / 4, to be exact, iirc. So if you see these kinds \nof problems, and are running a recent kernel, do something like\n\n\tulimit -s 65536\n\nto give yourself a big stack and you can continue without these kinds of \nchanges..\n\n\t\tLinus\n"},{"id":"62924","messageId":"20071212161701.GA18930@sigill.intra.peff.net","threadId":"11243","inReplyTo":"alpine.LFD.0.9999.0712120753120.25032@woody.linux-foundation.org","subject":"Re: git-cvsexportcommit fails for huge commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-12T16:17:01Z","receivedAt":"2007-12-12T16:17:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 12, 2007 at 08:02:35AM -0800, Linus Torvalds wrote:\n\n> On Wed, 12 Dec 2007, Jeff King wrote:\n> >  \n> > +sub xargs_safe_pipe_capture {\n> > +\tmy $MAX_ARG_LENGTH = 1024;\n> \n> Well, that's a bit extreme. Make it 16kB or something. Anything should be \n> able to handle that.\n\nObviously it should be tweaked, and I just chose an absurdly low value.\nHowever, \"xargs --show-limit\" claims that the POSIX minimum is 2048,\nless the size of the environment. I have no idea what the original\nproblem reporter's platform is, or what sort of environment size is sane\nthere.\n\n-Peff\n"},{"id":"62957","messageId":"46a038f90712121158n674a9044t75ef99473314457c@mail.gmail.com","threadId":"11243","inReplyTo":"20071212083154.GB7676@coredump.intra.peff.net","subject":"Re: git-cvsexportcommit fails for huge commits","fromName":"Martin Langhoff","fromEmail":"martin.langhoff@gmail.com","sentAt":"2007-12-12T19:58:33Z","receivedAt":"2007-12-12T19:58:33Z","isPatch":false,"sender":{"key":"martin.langhoff@gmail.com","avatar":"https://gravatar.com/avatar/1e3f311b6c4c15836501901ca58f8c0b0667246488084ba524d8bc9867e22fd9?d=mp&s=160"},"body":"On Dec 12, 2007 9:31 PM, Jeff King <peff@peff.net> wrote:\n> We will make our commits\n> in two CVS invocations, but since CVS isn't atomic _anyway_, we\n> shouldn't mind losing the atomicity.\n\nQuick note -- I used to understand that cvs was not atomic, but\nreading the on-the-wire protocol has taught me otherwise. Modern\nversions of cvs are actually quite atomic-ish -- the protocol expects\nthe client to give all the relevant data to the server, and then say\n\"yep, that was all\", and only _then_ the server does its commit.\n\nSo if the client dies or cancels along the way, nothing ever happens\non the server side.\n\nStill, I suspect that the _server_ is not atomic, so if the server\nprocess dies or finds a problem along the way, you could end up with a\nhalf-commit in the repository, and maybe some hosed ,v files.\n\nIOWs, the protocol *is* atomic, and this patch does make things\nslightly more brittle. Perhaps require an option to be set before we\ndo this?\n\n\nm\n"},{"id":"62978","messageId":"20071213041737.GA23624@coredump.intra.peff.net","threadId":"11243","inReplyTo":"46a038f90712121158n674a9044t75ef99473314457c@mail.gmail.com","subject":"Re: git-cvsexportcommit fails for huge commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-13T04:17:37Z","receivedAt":"2007-12-13T04:17:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 13, 2007 at 08:58:33AM +1300, Martin Langhoff wrote:\n\n> IOWs, the protocol *is* atomic, and this patch does make things\n> slightly more brittle. Perhaps require an option to be set before we\n> do this?\n\nI started writing a patch to let the user specify the limit, but it was\njust too ugly. What user knows the right limit? We are probably better\noff just setting the limit at something high and reasonable (like 64K --\nit would be nice to get feedback from Markus on what platform he is\nusing and what is a reasonable value), or just using a tempfile with\nxargs, which should figure out the correct value.\n\n-Peff\n"},{"id":"62991","messageId":"20071213083931.GA6441@mkl-desktop","threadId":"11243","inReplyTo":"20071213041737.GA23624@coredump.intra.peff.net","subject":"Re: git-cvsexportcommit fails for huge commits","fromName":"Markus Klinik","fromEmail":"markus.klinik@gmx.de","sentAt":"2007-12-13T08:39:31Z","receivedAt":"2007-12-13T08:39:31Z","isPatch":false,"sender":{"key":"markus.klinik@gmx.de","avatar":null},"body":"On Wed, Dec 12, 2007 at 11:17:37PM -0500, Jeff King wrote:\n> it would be nice to get feedback from Markus on what platform he is\n> using and what is a reasonable value), or just using a tempfile with\n> xargs, which should figure out the correct value.\n\nMy platform is an ubuntu 7.04.\n\n$ uname -a \nLinux mkl-desktop 2.6.20-16-386 #2 Sun Sep 23 19:47:10 UTC 2007 i686\nGNU/Linux\n$ ulimit -s\n8192\n\nI wasn't able to verify if the patch or increasing the stack limit works\nbecause I stumbled upon what seems to be this problem:\nhttp://marc.info/?l=git&m=119419453726664&w=2\n\nThere is already a patch for the cvs-status-fileorder bug but it doesn't\napply and obviously never made it to the public reopsitory.\nhttp://marc.info/?l=git&m=118718448305647&w=2\n\nAt the end of this message is a session log that reproduces this\nbehaviour. The session uses git-cvsexportcommit without the xargs-patch.\n\nIf my understanding is correct the 'cvs status' query is just to make\nsure everything is up to date, and can be overridden by the -f flag.\nI'll try that today and report if the xargs-patch and the stack limit\nthing worked.\n\n-Markus\n\nmkl@mkl-desktop:~/tmp$ # ---------------------- begin\nmkl@mkl-desktop:~$ cvs --version\n\nConcurrent Versions System (CVS) 1.12.13 (client/server)\n\nCopyright (C) 2005 Free Software Foundation, Inc.\n\nSenior active maintainers include Larry Jones, Derek R. Price,\nand Mark D. Baushke.  Please see the AUTHORS and README files from the\nCVS\ndistribution kit for a complete list of contributors and copyrights.\n\nCVS may be copied only under the terms of the GNU General Public\nLicense,\na copy of which can be found with the CVS distribution kit.\n\nSpecify the --help option for further information about CVS\nmkl@mkl-desktop:~$ git --version\ngit version 1.5.3.7.1157.gbf82a\nmkl@mkl-desktop:~/tmp$ mkdir ~/cvsroot\nmkl@mkl-desktop:~/tmp$ export CVSROOT=~/cvsroot\nmkl@mkl-desktop:~/tmp$ cvs init\nmkl@mkl-desktop:~/tmp$ mkdir cfoo\nmkl@mkl-desktop:~/tmp$ cd cfoo/\nmkl@mkl-desktop:~/tmp/cfoo$ cvs import -m'import' cfoo A AA\n\nNo conflicts created by this import\n\nmkl@mkl-desktop:~/tmp/cfoo$ cd ..\nmkl@mkl-desktop:~/tmp$ mkdir gfoo\nmkl@mkl-desktop:~/tmp$ cd gfoo/\nmkl@mkl-desktop:~/tmp/gfoo$ git-init\nInitialized empty Git repository in .git/\nmkl@mkl-desktop:~/tmp/gfoo$ mkdir bar\nmkl@mkl-desktop:~/tmp/gfoo$ touch a b c d bar/a bar/b bar/e\nmkl@mkl-desktop:~/tmp/gfoo$ git add .\nmkl@mkl-desktop:~/tmp/gfoo$ git commit -m'empty'\nCreated initial commit a11f9bc: empty\n 0 files changed, 0 insertions(+), 0 deletions(-)\n create mode 100644 a\n create mode 100644 b\n create mode 040000 bar\n create mode 100644 c\n create mode 100644 d\nmkl@mkl-desktop:~/tmp/gfoo$ echo 'hi' > 0\nmkl@mkl-desktop:~/tmp/gfoo$ echo 'hi' > a\nmkl@mkl-desktop:~/tmp/gfoo$ echo 'hi' > bar/e\nmkl@mkl-desktop:~/tmp/gfoo$ echo 'hi' > e\nmkl@mkl-desktop:~/tmp/gfoo$ git add .\nmkl@mkl-desktop:~/tmp/gfoo$ git commit -m 'hi'\nCreated commit e71a144: hi\n 3 files changed, 3 insertions(+), 0 deletions(-)\n create mode 100644 0\n create mode 100644 e\nmkl@mkl-desktop:~/tmp/gfoo$ cd ..\nmkl@mkl-desktop:~/tmp$ cvs co cfoo\ncvs checkout: Updating cfoo\nmkl@mkl-desktop:~/tmp$ cd cfoo\nmkl@mkl-desktop:~/tmp/cfoo$ export GIT_DIR=../gfoo/.git\nmkl@mkl-desktop:~/tmp/cfoo$ git-cvsexportcommit -c HEAD^ \nChecking if patch will apply\ncvs status: nothing known about `a'\ncvs status: nothing known about `b'\ncvs status: nothing known about `c'\ncvs status: nothing known about `d'\nApplying\nPatch applied successfully. Adding new files and directories to CVS\nDirectory /home/mkl/cvsroot/cfoo/bar added to the repository\ncvs add: scheduling file `a' for addition\ncvs add: use `cvs commit' to add this file permanently\ncvs add: scheduling file `b' for addition\ncvs add: use `cvs commit' to add this file permanently\ncvs add: scheduling file `bar/a' for addition\ncvs add: use `cvs commit' to add this file permanently\ncvs add: scheduling file `bar/b' for addition\ncvs add: use `cvs commit' to add this file permanently\ncvs add: scheduling file `bar/e' for addition\ncvs add: use `cvs commit' to add this file permanently\ncvs add: scheduling file `c' for addition\ncvs add: use `cvs commit' to add this file permanently\ncvs add: scheduling file `d' for addition\ncvs add: use `cvs commit' to add this file permanently\nCommit to CVS\nPatch title (first comment line): empty\nAutocommit\n  cvs commit -F .msg 'a' 'b' 'bar/a' 'bar/b' 'bar/e' 'c' 'd'\n/home/mkl/cvsroot/cfoo/a,v  <--  a\ninitial revision: 1.1\n/home/mkl/cvsroot/cfoo/b,v  <--  b\ninitial revision: 1.1\n/home/mkl/cvsroot/cfoo/c,v  <--  c\ninitial revision: 1.1\n/home/mkl/cvsroot/cfoo/d,v  <--  d\ninitial revision: 1.1\n/home/mkl/cvsroot/cfoo/bar/a,v  <--  bar/a\ninitial revision: 1.1\n/home/mkl/cvsroot/cfoo/bar/b,v  <--  bar/b\ninitial revision: 1.1\n/home/mkl/cvsroot/cfoo/bar/e,v  <--  bar/e\ninitial revision: 1.1\nCommitted successfully to CVS\nmkl@mkl-desktop:~/tmp/cfoo$ git-cvsexportcommit -c HEAD\nChecking if patch will apply\ncvs status: nothing known about `0'\ncvs status: nothing known about `e'\nFile 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.\nStatus was: Up-to-date\nFile bar/e not up to date but has status 'Unknown' in your CVS checkout!\nExiting: your CVS tree is not clean for this merge. at /home/mkl/bin/git-cvsexportcommit line 235.\nmkl@mkl-desktop:~/tmp/cfoo$ # ----------- end\n"},{"id":"62993","messageId":"20071213090148.GA4496@coredump.intra.peff.net","threadId":"11243","inReplyTo":"20071213083931.GA6441@mkl-desktop","subject":"Re: git-cvsexportcommit fails for huge commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-13T09:01:49Z","receivedAt":"2007-12-13T09:01:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 13, 2007 at 09:39:31AM +0100, Markus Klinik wrote:\n\n> > it would be nice to get feedback from Markus on what platform he is\n> > using and what is a reasonable value), or just using a tempfile with\n> > xargs, which should figure out the correct value.\n> \n> My platform is an ubuntu 7.04.\n\nIn that case, I think a very large value like 100K is fine. Then the code\nwon't activate unless it is really required.\n\nIt still feels a bit hack-ish and wrong, but I'm not sure what would be\nbetter.\n\n-Peff\n"},{"id":"63076","messageId":"7vzlwevu2k.fsf@gitster.siamese.dyndns.org","threadId":"11243","inReplyTo":"20071212092512.GB20799@coredump.intra.peff.net","subject":"Re: git-cvsexportcommit fails for huge commits","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-14T03:22:43Z","receivedAt":"2007-12-14T03:22:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> the path at the beginning happens to be longer than 1024 then you will\n>> run path-less \"cvs status\"?\n>\n> No, read the loop again. The length starts at 0, so we always go through\n> the loop body once.\n\nSorry, you are right.\n\nPerhaps pick a reasonably small but not insanely small value, like 16kB,\nforget about the atomicity issues for now, as an interim improvement\npatch?\n"},{"id":"63080","messageId":"20071214044554.GB10169@sigill.intra.peff.net","threadId":"11243","inReplyTo":"7vzlwevu2k.fsf@gitster.siamese.dyndns.org","subject":"Re: git-cvsexportcommit fails for huge commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-14T04:45:54Z","receivedAt":"2007-12-14T04:45:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 13, 2007 at 07:22:43PM -0800, Junio C Hamano wrote:\n\n> Sorry, you are right.\n> \n> Perhaps pick a reasonably small but not insanely small value, like 16kB,\n> forget about the atomicity issues for now, as an interim improvement\n> patch?\n\nI'm fine with that. We can probably go a bit higher than that. From my\nlimited testing[1]:\n\n  Linux 2.6.18: ~128K\n  Linux 2.6.23: huge? I tried ~350K and it worked fine\n  Solaris: huge? I tried ~350K and it worked fine\n  Freebsd 6.1: ~256K\n\nSo it seems that we could probably go with something more like 64K, and\nthen only truly pathological cases should trigger the behavior.\n\n-Peff\n\n[1] All numbers are approximate and determined experimentally with\nsomething like:\n  for i in `seq 1 $n`; do\n    touch $long_filename-$i\n  done\n  ls * | wc\n"},{"id":"63132","messageId":"20071214091546.GA20907@coredump.intra.peff.net","threadId":"11243","inReplyTo":"20071214044554.GB10169@sigill.intra.peff.net","subject":"Re: git-cvsexportcommit fails for huge commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-14T09:15:47Z","receivedAt":"2007-12-14T09:15:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 13, 2007 at 11:45:54PM -0500, Jeff King wrote:\n\n> So it seems that we could probably go with something more like 64K, and\n> then only truly pathological cases should trigger the behavior.\n\nSo here is a cleaned up patch. It bumps the maximum size to 64kB, adds\nscalar support (nobody uses it, but it makes sense for the interface to\nmatch that of safe_pipe_capture -- I am even tempted to just replace\nsafe_pipe_capture entirely and convert the few other callers), and\ncleans up the unused safe_pipe_capture_blob.\n\n-- >8 --\ncvsexportcommit: fix massive commits\n\nBecause we feed the changed filenames to CVS on the command\nline, it was possible for massive commits to overflow the\nsystem exec limits. Instead, we now do an xargs-like split\nof the arguments.\n\nThis means that we lose some of the atomicity of calling CVS\nin one shot. Since CVS commits are not atomic, but the CVS\nprotocol is, the possible effects of this are not clear;\nhowever, since CVS doesn't provide a different interface,\nthis is our only option for large commits (short of writing\na CVS client library).\n\nThe argument size limit is arbitrarily set to 64kB. This\nshould be high enough to trigger only in rare cases where it\nis necessary, so normal-sized commits are not affected by\nthe atomicity change.\n---\nI think the atomicity might matter if you are using cvsexportcommit to\ntalk to a CVS server backed by something besides CVS, like\ngit-cvsserver. Which of course would be useless, but who is to say\nthat other such systems don't exist with CVS as an SCM lingua franca?\n\n git-cvsexportcommit.perl |   37 +++++++++++++++++++++++--------------\n 1 files changed, 23 insertions(+), 14 deletions(-)\n\ndiff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl\nindex 92e4162..d2e50c3 100755\n--- a/git-cvsexportcommit.perl\n+++ b/git-cvsexportcommit.perl\n@@ -195,11 +195,11 @@ foreach my $f (@files) {\n my %cvsstat;\n if (@canstatusfiles) {\n     if ($opt_u) {\n-      my @updated = safe_pipe_capture(@cvs, 'update', @canstatusfiles);\n+      my @updated = xargs_safe_pipe_capture([@cvs, 'update'], @canstatusfiles);\n       print @updated;\n     }\n     my @cvsoutput;\n-    @cvsoutput= safe_pipe_capture(@cvs, 'status', @canstatusfiles);\n+    @cvsoutput = xargs_safe_pipe_capture([@cvs, 'status'], @canstatusfiles);\n     my $matchcount = 0;\n     foreach my $l (@cvsoutput) {\n         chomp $l;\n@@ -295,7 +295,7 @@ if ($dirtypatch) {\n \n if ($opt_c) {\n     print \"Autocommit\\n  $cmd\\n\";\n-    print safe_pipe_capture(@cvs, 'commit', '-F', '.msg', @files);\n+    print xargs_safe_pipe_capture([@cvs, 'commit', '-F', '.msg'], @files);\n     if ($?) {\n \tdie \"Exiting: The commit did not succeed\";\n     }\n@@ -335,15 +335,24 @@ sub safe_pipe_capture {\n     return wantarray ? @output : join('',@output);\n }\n \n-sub safe_pipe_capture_blob {\n-    my $output;\n-    if (my $pid = open my $child, '-|') {\n-        local $/;\n-\tundef $/;\n-\t$output = (<$child>);\n-\tclose $child or die join(' ',@_).\": $! $?\";\n-    } else {\n-\texec(@_) or die \"$! $?\"; # exec() can fail the executable can't be found\n-    }\n-    return $output;\n+sub xargs_safe_pipe_capture {\n+\tmy $MAX_ARG_LENGTH = 65536;\n+\tmy $cmd = shift;\n+\tmy @output;\n+\tmy $output;\n+\twhile(@_) {\n+\t\tmy @args;\n+\t\tmy $length = 0;\n+\t\twhile(@_ && $length < $MAX_ARG_LENGTH) {\n+\t\t\tpush @args, shift;\n+\t\t\t$length += length($args[$#args]);\n+\t\t}\n+\t\tif (wantarray) {\n+\t\t\tpush @output, safe_pipe_capture(@$cmd, @args);\n+\t\t}\n+\t\telse {\n+\t\t\t$output .= safe_pipe_capture(@$cmd, @args);\n+\t\t}\n+\t}\n+\treturn wantarray ? @output : $output;\n }\n-- \n1.5.4.rc0.1088.g8a30-dirty\n"},{"id":"63165","messageId":"200712141447.05039.robin.rosenberg.lists@dewire.com","threadId":"11243","inReplyTo":"20071214091546.GA20907@coredump.intra.peff.net","subject":"Re: git-cvsexportcommit fails for huge commits","fromName":"Robin Rosenberg","fromEmail":"robin.rosenberg.lists@dewire.com","sentAt":"2007-12-14T13:47:03Z","receivedAt":"2007-12-14T13:47:03Z","isPatch":false,"sender":{"key":"robin.rosenberg@dewire.com","avatar":"https://avatars.githubusercontent.com/u/46357?v=4"},"body":"fredag 14 december 2007 skrev Jeff King:\n> On Thu, Dec 13, 2007 at 11:45:54PM -0500, Jeff King wrote:\n> \n> > So it seems that we could probably go with something more like 64K, and\n> > then only truly pathological cases should trigger the behavior.\n> \n> So here is a cleaned up patch. It bumps the maximum size to 64kB, adds\n> scalar support (nobody uses it, but it makes sense for the interface to\n> match that of safe_pipe_capture -- I am even tempted to just replace\n> safe_pipe_capture entirely and convert the few other callers), and\n> cleans up the unused safe_pipe_capture_blob.\n\nWouldn't using the POSIX::ARG_MAX constant work?\n\n-- robin\n"},{"id":"63242","messageId":"20071215100950.GB1692@coredump.intra.peff.net","threadId":"11243","inReplyTo":"200712141447.05039.robin.rosenberg.lists@dewire.com","subject":"Re: git-cvsexportcommit fails for huge commits","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-12-15T10:09:50Z","receivedAt":"2007-12-15T10:09:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 14, 2007 at 02:47:03PM +0100, Robin Rosenberg wrote:\n\n> > So here is a cleaned up patch. It bumps the maximum size to 64kB, adds\n> > scalar support (nobody uses it, but it makes sense for the interface to\n> > match that of safe_pipe_capture -- I am even tempted to just replace\n> > safe_pipe_capture entirely and convert the few other callers), and\n> > cleans up the unused safe_pipe_capture_blob.\n> \n> Wouldn't using the POSIX::ARG_MAX constant work?\n\nIt does seem to produce sensible results. Does it work reasonably on\nWindows?\n\nNote that it would also need to be ARG_MAX - slop, where slop accounts\nfor the environment (which maybe we can get accurately by counting the\nkeys and values of %ENV, adding NUL terminators?).\n\n-Peff\n"}]}