Johannes Schindelin <Johannes.Schindelin@xxxxxx> 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 ;-)
> + # "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?
> + if (!defined($basenames{$basename})) {
> + $basenames{$basename} = $name;
> + push (@canstatusfiles2, $name);
> + splice (@canstatusfiles, $i, 1);
> + $i--;
> }
> + }
> + 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;
}
}
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
