Jeff King [off-list ref] writes:
quoted
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?
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
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
fredag 14 december 2007 skrev Jeff King:
On Thu, Dec 13, 2007 at 11:45:54PM -0500, Jeff King wrote:
quoted
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
On Fri, Dec 14, 2007 at 02:47:03PM +0100, Robin Rosenberg wrote:
quoted
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