Hi,
On Sun, 17 Feb 2008, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
>
> > Feel free to criticise/educate me on my Perl style.
>
> Here it goes ;-)
Very much appreciated.
> > + # "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++) {
It looks nicer, granted. But because of my use of splice(), it does not
work. However, it seems that I introduced another breakage there...
So this is how I will do it: have a hash with all remaining fullnames, and
"delete" them when they are done.
> > + my $name = $canstatusfiles[$i];
>
> > + my $basename = $name;
> > + $basename =~ s/.*\///;
>
> The script uses File::Basename upfront so perhaps just simply...
I tried that. But as the file need not exist, "basename" went on strike.
So I'll keep the (ugly) version.
> 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"?
To the contrary, they _are_ reported, with an ugly "no file " prepended.
So in order to verify those, I have to make sure that there is no file
named "no file <blabla>", which would not be distinguishable from the
reported for the non-existing file "<blabla>".
But I'll just use your %added idea.
> > + chomp($basename);
>
> Huh? Perhaps you wanted to chomp at the very beginning of the loop?
No, I want to do that after the "no file " prepending. Because that is
the way "cvs status" reports them... with no good way for me to tell how
much leading/trailing white space there is.
But you're right, I should add a test to verify that a filename with
leading spaces is added correctly.
So I will do that.
> > + 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.
Right.
Thanks for your review,
Dscho
-
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
