# [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file. 15 messages from 2007-05-08 to 2007-05-14. Participants: Steffen Prohaska, Johannes Schindelin, Robin Rosenberg, Martin Langhoff, Junio C Hamano. Thread: https://gitlist.dev/t/8047 ## Steffen Prohaska, 2007-05-08 23:59 Subject: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file. Message-ID: <0056A63A-D511-4FDD-82A6-A13B06E237E9@zib.de> URL: https://gitlist.dev/e/0056A63A-D511-4FDD-82A6-A13B06E237E9%40zib.de ``` 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 --- 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=; +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 ``` ## Steffen Prohaska, 2007-05-09 07:42 Subject: Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file. Message-ID: URL: https://gitlist.dev/e/EC3AE084-0AB8-406A-A3C7-916CCF35BEAE%40zib.de In-Reply-To: <0056A63A-D511-4FDD-82A6-A13B06E237E9@zib.de> ``` 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 ``` ## Steffen Prohaska, 2007-05-09 07:45 Subject: [PATCH (corrected)] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file. Message-ID: <82A134F6-C11A-440C-8424-DDDDBF7DCD7A@zib.de> URL: https://gitlist.dev/e/82A134F6-C11A-440C-8424-DDDDBF7DCD7A%40zib.de In-Reply-To: ``` 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 --- 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=; +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 ``` ## Johannes Schindelin, 2007-05-09 11:04 Subject: Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0705091303280.4167%40racer.site In-Reply-To: <0056A63A-D511-4FDD-82A6-A13B06E237E9@zib.de> ``` 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 ``` ## Steffen Prohaska, 2007-05-09 11:43 Subject: Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file. Message-ID: <4BA4CF7D-40CB-4A55-ADDC-56DE179FD6C1@zib.de> URL: https://gitlist.dev/e/4BA4CF7D-40CB-4A55-ADDC-56DE179FD6C1%40zib.de In-Reply-To: ``` 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 ``` ## Johannes Schindelin, 2007-05-09 12:25 Subject: Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file. Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0705091424190.4167%40racer.site In-Reply-To: <4BA4CF7D-40CB-4A55-ADDC-56DE179FD6C1@zib.de> ``` 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 ``` ## Steffen Prohaska, 2007-05-09 13:00 Subject: Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file. Message-ID: <3467FF99-9945-4ECF-9282-308759E63ED9@zib.de> URL: https://gitlist.dev/e/3467FF99-9945-4ECF-9282-308759E63ED9%40zib.de In-Reply-To: ``` 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 ``` ## Robin Rosenberg, 2007-05-09 20:30 Subject: Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file. Message-ID: <200705092230.16027.robin.rosenberg.lists@dewire.com> URL: https://gitlist.dev/e/200705092230.16027.robin.rosenberg.lists%40dewire.com In-Reply-To: <0056A63A-D511-4FDD-82A6-A13B06E237E9@zib.de> ``` 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 ``` ## Steffen Prohaska, 2007-05-09 22:45 Subject: Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file. Message-ID: <380B28A3-5CD0-4371-A717-1D2629E6302D@zib.de> URL: https://gitlist.dev/e/380B28A3-5CD0-4371-A717-1D2629E6302D%40zib.de In-Reply-To: <200705092230.16027.robin.rosenberg.lists@dewire.com> ``` 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 ``` ## Steffen Prohaska, 2007-05-09 23:06 Subject: [PATCH] Optimized cvsexportcommit: calling 'cvs status' once instead of once per touched file. Message-ID: <11787519961666-git-send-email-prohaska@zib.de> URL: https://gitlist.dev/e/11787519961666-git-send-email-prohaska%40zib.de In-Reply-To: <380B28A3-5CD0-4371-A717-1D2629E6302D@zib.de> ``` 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 --- 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 ``` ## Martin Langhoff, 2007-05-10 06:53 Subject: Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file. Message-ID: <46a038f90705092353x107c738foa7db547ab29b1748@mail.gmail.com> URL: https://gitlist.dev/e/46a038f90705092353x107c738foa7db547ab29b1748%40mail.gmail.com In-Reply-To: <380B28A3-5CD0-4371-A717-1D2629E6302D@zib.de> ``` On 5/10/07, Steffen Prohaska 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 ``` ## Junio C Hamano, 2007-05-10 07:08 Subject: Re: [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file. Message-ID: <7vd519m9z7.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vd519m9z7.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <46a038f90705092353x107c738foa7db547ab29b1748@mail.gmail.com> ``` "Martin Langhoff" writes: > On 5/10/07, Steffen Prohaska 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. ``` ## Junio C Hamano, 2007-05-13 21:01 Subject: RFH for [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file. Message-ID: <7vps54qvya.fsf_-_@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vps54qvya.fsf_-_%40assigned-by-dhcp.cox.net In-Reply-To: <7vd519m9z7.fsf@assigned-by-dhcp.cox.net> ``` 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? ``` ## Robin Rosenberg, 2007-05-13 21:51 Subject: Re: RFH for [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file. Message-ID: <200705132351.24867.robin.rosenberg.lists@dewire.com> URL: https://gitlist.dev/e/200705132351.24867.robin.rosenberg.lists%40dewire.com In-Reply-To: <7vps54qvya.fsf_-_@assigned-by-dhcp.cox.net> ``` 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 ``` ## Martin Langhoff, 2007-05-14 06:40 Subject: Re: RFH for [PATCH] Optimized cvsexportcommit: calling 'cvs status' only once instead of once per changed file. Message-ID: <46a038f90705132340o4006d2f1gbad841144a4c8054@mail.gmail.com> URL: https://gitlist.dev/e/46a038f90705132340o4006d2f1gbad841144a4c8054%40mail.gmail.com In-Reply-To: <200705132351.24867.robin.rosenberg.lists@dewire.com> ``` On 5/14/07, Robin Rosenberg 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 ```