Re: [PATCH] cvsexportcommit: be graceful when "cvs status" reorders the arguments
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Feb 18, 2008, 17:54 UTC
- Message-ID
- <alpine.LSU.1.00.0802181627340.30505@racer.site>
- In-Reply-To
- <7vbq6fvudp.fsf@gitster.siamese.dyndns.org>
Hi,
On Sun, 17 Feb 2008, Junio C Hamano wrote:
Show 5 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > Feel free to criticise/educate me on my Perl style. > > Here it goes ;-)
Very much appreciated.
Show 10 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++) {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.
Show 6 quoted lines
> > + 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.
Show 7 quoted lines
> 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.
Show 19 quoted lines
> > + 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