[PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file.

[PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file.

[Steffen Prohaska]

The old implementation executed 'cvs status' for each file touched by  
the patch
to be applied. The new code calls 'cvs status' only once and parses  
cvs's
output to collect status information of all files contained in the  
cvs working
copy.

Runtime is now independent of the number of modified files. A  
drawback is that
the new code retrieves status information for all files even if only  
a few are
touched. The old implementation may be noticeably faster for small  
patches to
large workingcopies. However, the old implementation doesn't scale if  
more
files are touched, especially in remotely located cvs repositories.

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
git-cvsexportcommit.perl |   45 ++++++++++++++++++++++++++++++++++ 
+----------
1 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index 6ed4719..f2c4bc4 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -160,36 +160,61 @@ foreach my $p (@afiles) {
      }
}
+# ... check dirs,
foreach my $d (@dirs) {
      if (-e $d) {
	$dirty = 1;
	warn "$d exists and is not a directory!\n";
      }
}
+# ... query and store status of files by parsing output of 'cvs  
status',
+my @cvsoutput;
+my %cvsstat;
+open CVSSTAT, "cvs status 2>&1 |" || die "failed to query cvs status";
+@cvsoutput=<CVSSTAT>;
+close CVSSTAT || die "failed to query cvs status";
+my ( $dir, $status, $file );
+foreach my $f (@cvsoutput) {
+# cvs reports directories on stderr before reporting file status on  
stdout
+# using basename of 'Repository revision:' should be a safe way to  
deal with whitespace in filenames.
+    chomp $f;
+    if ( $f =~ /^cvs status: Examining (.*)$/ ) {
+        $dir = $1;
+        if ( $dir ne "." ) {
+            $dir .= "/";
+        } else {
+            $dir = "";
+        }
+    } elsif ( $f =~ /Status: (.*)$/ ) {
+        $status = $1;
+    } elsif ( $f =~ /^   Repository revision:/ ) {
+        $f =~ s/,v$//;
+        $f =~ /([^\/]*)$/;
+        $file = $1;
+        $cvsstat{"$dir$file"} = $status;
+    }
+}
+
+# ... validate new files,
foreach my $f (@afiles) {
      # This should return only one value
      if ($f =~ m,(.*)/[^/]*$,) {
	my $p = $1;
	next if (grep { $_ eq $p } @dirs);
      }
-    my @status = grep(m/^File/,  safe_pipe_capture(@cvs, '-q',  
'status' ,$f));
-    if (@status > 1) { warn 'Strange! cvs status returned more than  
one line?'};
-    if (-d dirname $f and $status[0] !~ m/Status: Unknown$/
-	and $status[0] !~ m/^File: no file /) {
+    if (defined ($cvsstat{$f})) {
   	$dirty = 1;
	warn "File $f 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. 
\n";
-	warn "Status was: $status[0]\n";
+	warn "Status was: $cvsstat{$f}\n";
      }
}
-
+# ... validate known files.
foreach my $f (@files) {
      next if grep { $_ eq $f } @afiles;
      # TODO:we need to handle removed in cvs
-    my @status = grep(m/^File/,  safe_pipe_capture(@cvs, '-q',  
'status' ,$f));
-    if (@status > 1) { warn 'Strange! cvs status returned more than  
one line?'};
-    unless ($status[0] =~ m/Status: Up-to-date$/) {
+    unless (defined ($cvsstat{$f}) and $cvsstat{$f} eq "Up-to-date") {
	$dirty = 1;
-	warn "File $f not up to date in your CVS checkout!\n";
+	warn "File $f not up to date but has status '$cvsstat{$f}' in your  
CVS checkout!\n";
      }
}
if ($dirty) {
--
1.5.1.2

Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file.

[Steffen Prohaska]

On May 9, 2007, at 1:59 AM, Steffen Prohaska wrote:

> The old implementation executed 'cvs status' for each file touched  
> by the patch
> to be applied. The new code calls 'cvs status' only once and parses  
> cvs's
> output to collect status information of all files contained in the  
> cvs working
> copy.
>
> [...]

I didn't recognize that my modifications cause the testsuite to fail.  
I'll send a corrected patch in a minute.

I apologize,
- Steffen

[PATCH (corrected)] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file.

[Steffen Prohaska]

The old implementation executed 'cvs status' for each file touched by  
the patch
to be applied. The new code calls 'cvs status' only once and parses  
cvs's
output to collect status information of all files contained in the  
cvs working
copy.

Runtime is now independent of the number of modified files. A  
drawback is that
the new code retrieves status information for all files even if only  
a few are
touched. The old implementation may be noticeably faster for small  
patches to
large workingcopies. However, the old implementation doesn't scale if  
more
files are touched, especially in remotely located cvs repositories.

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
git-cvsexportcommit.perl |   48 +++++++++++++++++++++++++++++++++++ 
+---------
1 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index 6ed4719..4d91574 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -160,36 +160,64 @@ foreach my $p (@afiles) {
      }
}
+# ... check dirs,
foreach my $d (@dirs) {
      if (-e $d) {
         $dirty = 1;
         warn "$d exists and is not a directory!\n";
      }
}
+
+# ... query and store status of files by parsing output of 'cvs  
status',
+# Note, we must use -n to avoid any modifications to working copy.
+# Otherwise the testsuite fails because it expects unmodfied CVS/ 
Entries files.
+my @cvsoutput;
+my %cvsstat;
+open CVSSTAT, "cvs -n status 2>&1 |" || die "failed to query cvs  
status";
+@cvsoutput=<CVSSTAT>;
+close CVSSTAT || die "failed to query cvs status";
+my ( $dir, $status, $file );
+foreach my $f (@cvsoutput) {
+# cvs reports directories on stderr before reporting file status on  
stdout
+# using basename of 'Repository revision:' should be a safe way to  
deal with whitespace in filenames.
+    chomp $f;
+    if ( $f =~ /^cvs status: Examining (.*)$/ ) {
+        $dir = $1;
+        if ( $dir ne "." ) {
+            $dir .= "/";
+        } else {
+            $dir = "";
+        }
+    } elsif ( $f =~ /Status: (.*)$/ ) {
+        $status = $1;
+    } elsif ( $f =~ /^   Repository revision:/ ) {
+        $f =~ s/,v$//;
+        $f =~ /([^\/]*)$/;
+        $file = $1;
+        $cvsstat{"$dir$file"} = $status;
+    }
+}
+
+# ... validate new files,
foreach my $f (@afiles) {
      # This should return only one value
      if ($f =~ m,(.*)/[^/]*$,) {
         my $p = $1;
         next if (grep { $_ eq $p } @dirs);
      }
-    my @status = grep(m/^File/,  safe_pipe_capture(@cvs, '-q',  
'status' ,$f));
-    if (@status > 1) { warn 'Strange! cvs status returned more than  
one line?'};
-    if (-d dirname $f and $status[0] !~ m/Status: Unknown$/
-       and $status[0] !~ m/^File: no file /) {
+    if (defined ($cvsstat{$f})) {
         $dirty = 1;
         warn "File $f 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.\n";
-       warn "Status was: $status[0]\n";
+       warn "Status was: $cvsstat{$f}\n";
      }
}
-
+# ... validate known files.
foreach my $f (@files) {
      next if grep { $_ eq $f } @afiles;
      # TODO:we need to handle removed in cvs
-    my @status = grep(m/^File/,  safe_pipe_capture(@cvs, '-q',  
'status' ,$f));
-    if (@status > 1) { warn 'Strange! cvs status returned more than  
one line?'};
-    unless ($status[0] =~ m/Status: Up-to-date$/) {
+    unless (defined ($cvsstat{$f}) and $cvsstat{$f} eq "Up-to-date") {
         $dirty = 1;
-       warn "File $f not up to date in your CVS checkout!\n";
+       warn "File $f not up to date but has status '$cvsstat{$f}' in  
your CVS checkout!\n";
      }
}
if ($dirty) {
--
1.5.1.2

Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file.

[Johannes Schindelin]

Hi,

On Wed, 9 May 2007, Steffen Prohaska wrote:

> The old implementation executed 'cvs status' for each file touched by 
> the patch to be applied.

I did not follow development of that script closely, but could it be that 
this is a safety valve, to make it unlikely to commit something which was 
changed by somebody else in the meantime?

Ciao,
Dscho

Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file.

[Steffen Prohaska]

Hello,

On May 9, 2007, at 1:04 PM, Johannes Schindelin wrote:

> On Wed, 9 May 2007, Steffen Prohaska wrote:
>
>> The old implementation executed 'cvs status' for each file touched by
>> the patch to be applied.
>
> I did not follow development of that script closely, but could it  
> be that
> this is a safety valve, to make it unlikely to commit something  
> which was
> changed by somebody else in the meantime?

Right. My patch doesn't change the functionality of the safety check.  
It's just a magnitude faster if you commit a lot of files. I'm now  
able to apply a patch that changes 900 files to a cvs working copy  
using ssh over DSL. I wasn't before, at least not in reasonable time.

Another solution would be to make the safety checks optional as they  
are not needed for the core functionality. If you have a clean, up-to- 
date cvs working copy you're fine.

- Steffen

Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file.

[Johannes Schindelin]

Hi,

On Wed, 9 May 2007, Steffen Prohaska wrote:

> On May 9, 2007, at 1:04 PM, Johannes Schindelin wrote:
> 
> > On Wed, 9 May 2007, Steffen Prohaska wrote:
> > 
> > > The old implementation executed 'cvs status' for each file touched by
> > > the patch to be applied.
> > 
> > I did not follow development of that script closely, but could it be that
> > this is a safety valve, to make it unlikely to commit something which was
> > changed by somebody else in the meantime?
> 
> Right. My patch doesn't change the functionality of the safety check. It's
> just a magnitude faster if you commit a lot of files. I'm now able to apply a
> patch that changes 900 files to a cvs working copy using ssh over DSL. I
> wasn't before, at least not in reasonable time.

What I was trying to get at: if you commit 900 files, and after the 450th 
file somebody _else_ commits a file, which just so happens to be one of 
your 450 remaining files, that safety check no longer holds.

CVS is slow.

Ciao,
Dscho

Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file.

[Steffen Prohaska]

On May 9, 2007, at 2:25 PM, Johannes Schindelin wrote:

> What I was trying to get at: if you commit 900 files, and after the  
> 450th
> file somebody _else_ commits a file, which just so happens to be  
> one of
> your 450 remaining files, that safety check no longer holds.
>
> CVS is slow.

I know. I'd probably apply my patch late at night or on the weekend,  
when the chance is lower to conflict with other commits.

You can't remove the race conditions that you have when interacting  
with cvs. We can only decrease the chance of being hit by making the  
time window smaller. This is a side benefit of my patch. The main  
purpose is to lower the overall time needed to apply patch that  
modifies lots of files.

- Steffen

Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file.

[Robin Rosenberg]

onsdag 09 maj 2007 skrev Steffen Prohaska:
> The old implementation executed 'cvs status' for each file touched by  
> the patch
> to be applied. The new code calls 'cvs status' only once and parses  
> cvs's
> output to collect status information of all files contained in the  
> cvs working
> copy.
> 
> Runtime is now independent of the number of modified files. A  
> drawback is that
> the new code retrieves status information for all files even if only  
> a few are
> touched. The old implementation may be noticeably faster for small  
> patches to

Ouch, lets see now. My working cvs checkout contains ~25k files and
my typical commit touches 5-20 files. 

A quick (well....) test says cvs status on my checkout takes about
five minutes to execute. Compare this with my typical exportcommit
time of about ten seconds. 

If you really need this, make a switch to select it.

Still we're missing a check for the case that new files/directories have been
added on the server, but are missing from the checkout, or why not run
an update first. If you are commit this number of large files you'll need that
check, or it's hurt a lot when things fail.

> large workingcopies. However, the old implementation doesn't scale if  
> more
> files are touched, especially in remotely located cvs repositories.

How come your commit are so large you'd prefer this behaviour?

-- robin

Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file.

[Steffen Prohaska]

On May 9, 2007, at 10:30 PM, Robin Rosenberg wrote:

> onsdag 09 maj 2007 skrev Steffen Prohaska:
>> The old implementation executed 'cvs status' for each file touched by
>> the patch
>> to be applied. The new code calls 'cvs status' only once and parses
>> cvs's
>> output to collect status information of all files contained in the
>> cvs working
>> copy.
>>
>> Runtime is now independent of the number of modified files. A
>> drawback is that
>> the new code retrieves status information for all files even if only
>> a few are
>> touched. The old implementation may be noticeably faster for small
>> patches to
>
> Ouch, lets see now. My working cvs checkout contains ~25k files and
> my typical commit touches 5-20 files.
>
> A quick (well....) test says cvs status on my checkout takes about
> five minutes to execute. Compare this with my typical exportcommit
> time of about ten seconds.

I tested with ~7k files using ssh to connect. Maybe something's wrong
with the server I'm connecting to but it takes some time for each  
connect.
This alone kills the performance for a couple of files.

'cvs -z6 status' on the other hand took only 10 seconds.

> If you really need this, make a switch to select it.

I'll post a patch soon that takes the best of both: Call cvs status
once with the list of touched files and parse the output. Only one
connection is needed and only the minimal amount of status data is
transferred.

- Steffen

[PATCH] Optimized cvsexportcommit: calling 'cvs status' once instead of once per touched file.

[Steffen Prohaska]

Runtime is now independent of the number of modified files.

The old implementation executed 'cvs status' for each file touched by the patch
to be applied. The new code calls 'cvs status' only once with all touched files
and parses cvs's output to collect all available status information.

Signed-off-by: Steffen Prohaska <prohaska@zib.de>
---
 git-cvsexportcommit.perl |   45 ++++++++++++++++++++++++++++++---------------
 1 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index 6ed4719..21d49f6 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -160,36 +160,51 @@ foreach my $p (@afiles) {
     }
 }
 
+# ... check dirs,
 foreach my $d (@dirs) {
     if (-e $d) {
 	$dirty = 1;
 	warn "$d exists and is not a directory!\n";
     }
 }
-foreach my $f (@afiles) {
-    # This should return only one value
-    if ($f =~ m,(.*)/[^/]*$,) {
-	my $p = $1;
-	next if (grep { $_ eq $p } @dirs);
+
+# ... query status of all files that we have a directory for and parse output of 'cvs status' to %cvsstat.
+my @canstatusfiles;
+foreach my $f (@files) {
+    my $path = dirname $f;
+    next if (grep { $_ eq $path } @dirs);
+    push @canstatusfiles, $f;
+}
+  
+my %cvsstat;
+if (@canstatusfiles) {
+    my @cvsoutput;
+    @cvsoutput= safe_pipe_capture(@cvs, 'status', @canstatusfiles);
+    my $matchcount = 0;
+    foreach my $l (@cvsoutput) {
+        chomp $l;
+        if ( $l =~ /^File:/ and  $l =~ /Status: (.*)$/ ) { 
+            $cvsstat{$canstatusfiles[$matchcount]} = $1;
+            $matchcount++;
+        }
     }
-    my @status = grep(m/^File/,  safe_pipe_capture(@cvs, '-q', 'status' ,$f));
-    if (@status > 1) { warn 'Strange! cvs status returned more than one line?'};
-    if (-d dirname $f and $status[0] !~ m/Status: Unknown$/
-	and $status[0] !~ m/^File: no file /) {
+}
+
+# ... validate new files,
+foreach my $f (@afiles) {
+    if (defined ($cvsstat{$f}) and $cvsstat{$f} ne "Unknown") {
  	$dirty = 1;
 	warn "File $f 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.\n";
-	warn "Status was: $status[0]\n";
+	warn "Status was: $cvsstat{$f}\n";
     }
 }
-
+# ... validate known files. 
 foreach my $f (@files) {
     next if grep { $_ eq $f } @afiles;
     # TODO:we need to handle removed in cvs
-    my @status = grep(m/^File/,  safe_pipe_capture(@cvs, '-q', 'status' ,$f));
-    if (@status > 1) { warn 'Strange! cvs status returned more than one line?'};
-    unless ($status[0] =~ m/Status: Up-to-date$/) {
+    unless (defined ($cvsstat{$f}) and $cvsstat{$f} eq "Up-to-date") {
 	$dirty = 1;
-	warn "File $f not up to date in your CVS checkout!\n";
+	warn "File $f not up to date but has status '$cvsstat{$f}' in your CVS checkout!\n";
     }
 }
 if ($dirty) {
-- 
1.5.1.2

Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file.

[Martin Langhoff]

On 5/10/07, Steffen Prohaska <prohaska@zib.de> wrote:
> I'll post a patch soon that takes the best of both: Call cvs status
> once with the list of touched files and parse the output. Only one
> connection is needed and only the minimal amount of status data is
> transferred.

Yes, please :-) I wrote cvsexportcommit originally, and use it on a
huge cvs checkout that is hosted on SF.net (slow!). My commits are
small (3~5 files). I agree with using only one connection so commits
with many files are sanity-checked faster, but status needs to ask
explicitly about the files it's about to touch.

cheers,


m

Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file.

[Junio C Hamano]

"Martin Langhoff" <martin.langhoff@gmail.com> writes:

> On 5/10/07, Steffen Prohaska <prohaska@zib.de> wrote:
>> I'll post a patch soon that takes the best of both: Call cvs status
>> once with the list of touched files and parse the output. Only one
>> connection is needed and only the minimal amount of status data is
>> transferred.
>
> Yes, please :-) I wrote cvsexportcommit originally, and use it on a
> huge cvs checkout that is hosted on SF.net (slow!). My commits are
> small (3~5 files). I agree with using only one connection so commits
> with many files are sanity-checked faster, but status needs to ask
> explicitly about the files it's about to touch.
>
> cheers,

Sounds like you are perfect guinea pig.  A test in the real-life
use followed by an Ack is very appreciated.

RFH for [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file.

[Junio C Hamano]

I'd like to have feedbacks on this patch, as I think what it
tries to do is sensible and worth to have it in v1.5.2 if it
works for people.

Ack?  Nack?  YesButNeedsmorework?

Re: RFH for [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file.

[Robin Rosenberg]

söndag 13 maj 2007 skrev Junio C Hamano:
> I'd like to have feedbacks on this patch, as I think what it
> tries to do is sensible and worth to have it in v1.5.2 if it
> works for people.
> 
> Ack?  Nack?  YesButNeedsmorework?

Looks good, but I haven't tried it for production work yet.

-- robin

Re: RFH for [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file.

[Martin Langhoff]

On 5/14/07, Robin Rosenberg <robin.rosenberg.lists@dewire.com> wrote:
> söndag 13 maj 2007 skrev Junio C Hamano:
> > I'd like to have feedbacks on this patch, as I think what it
> > tries to do is sensible and worth to have it in v1.5.2 if it
> > works for people.
> >
> > Ack?  Nack?  YesButNeedsmorework?
>
> Looks good, but I haven't tried it for production work yet.

Same here - got it in my dev system, but haven't had a chance to use
it in real-life.

m