Re: [PATCH] cvsexportcommit: be graceful when "cvs status" reorders the arguments
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 18, 2008, 03:03 UTC
- Message-ID
- <7vbq6fvudp.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <alpine.LSU.1.00.0802180127100.30505@racer.site>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> I finally tracked it down to two filenames having the same basename.
Wonderful.
> Feel free to criticise/educate me on my Perl style.
Here it goes ;-)
Show 6 quoted lines
> + # "cvs status" reorders the parameters, notably when there are multiple
> + # arguments with the same basename. So be precise here.
> + while (@canstatusfiles) {
> + my @canstatusfiles2 = ();
> + my %basenames = ();
> + for (my $i = 0; $i <= $#canstatusfiles; $i++) {The "$index <= $#array" termination condition feels so Perl4-ish.
for (my $i = 0; $i < @canstatusfiles; $i++) {> + my $name = $canstatusfiles[$i];
> + my $basename = $name; > + $basename =~ s/.*\///;
The script uses File::Basename upfront so perhaps just simply...
my $basename = basename($name);
> + $basename = "no file " . $basename if (grep {$_ eq $basename} @afiles);Huh? Perl or no Perl that is too ugly a hack... What special treatment do "added files" need? We would want to make sure that the files are not reported from "cvs status"?
> + chomp($basename);
Huh? Perhaps you wanted to chomp at the very beginning of the loop?
Show 7 quoted lines
> + if (!defined($basenames{$basename})) {
> + $basenames{$basename} = $name;
> + push (@canstatusfiles2, $name);
> + splice (@canstatusfiles, $i, 1);
> + $i--;
> }
> + }Show 8 quoted lines
> + my @cvsoutput;
> + @cvsoutput = xargs_safe_pipe_capture([@cvs, 'status'], @canstatusfiles2);
> + foreach my $l (@cvsoutput) {
> + chomp $l;
> + if ( $l =~ /^File:\s+(.*\S)\s+Status: (.*)$/ ) {
> + $cvsstat{$basenames{$1}} = $2 if defined($basenames{$1});
> + }
> + }I think "exists $hash{$index}" would be easier to read and more logical here and also if () condition above.
Without understanding what is really going on with the "added files" case, here is how I would write your patch.
Side note. I personally do not like naming hashes and arrays plural, and call a hash of paths and list of files %path and @file respectively. That convention makes it easier to read things like these:
$file[4] ;# fourth file, not $files[4]
$path{'hello.c'} ;# path for 'hello.c', not $paths{'hello.c'}---
git-cvsexportcommit.perl | 43 ++++++++++++++++++++++++++++++++++--------- 1 files changed, 34 insertions(+), 9 deletions(-)
diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl index 2a8ad1e..06e7fda 100755 --- a/git-cvsexportcommit.perl +++ b/git-cvsexportcommit.perl @@ -197,15 +197,40 @@ if (@canstatusfiles) { my @updated = xargs_safe_pipe_capture([@cvs, 'update'], @canstatusfiles); print @updated; } - my @cvsoutput; - @cvsoutput = xargs_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 %added = map { $_ => 1 } @afiles; + + while (@canstatusfiles) { + my %basename = (); + my @status = (); + my @leftover = (); + for (my $i = 0; $i < @canstatusfiles; $i++) { + my $name = $canstatusfiles[$i]; + my $basename = basename($name); + if (exists $basename{$basename}) { + push @leftover, $name; + next; + } + if (exists $added{$name}) { + # Hmph... + next; + } + $basename{$basename} = $name; + push @status, $name; + } + my @cvsoutput = xargs_safe_pipe_capture([@cvs, 'status'], @status); + for my $l (@cvsoutput) { + chomp $l; + if ($l =~ /^File:\s+(.*\S)\s+Status: (.*)$/) { + my ($n, $s) = ($1, $2); + if (!exists $basename{$n}) { + print STDERR "Huh ($n)?\n"; + } else { + $cvsstat{$basename{$n}} = $s; + } + } + } + @canstatusfiles = @leftover; } }