Re: [PATCH] cvsexportcommit: be graceful when "cvs status" reorders the arguments

[Junio C Hamano <gitster@pobox.com>]

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 ;-)

> +    # "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;
     }
 }