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

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

[Johannes Schindelin]

In my use cases, "cvs status" sometimes reordered the passed filenames,
which often led to a misdetection of a dirty state (when it was in
reality a clean state).

I finally tracked it down to two filenames having the same basename.

So no longer trust the order of the results blindly, but actually check
the file name.

Since "cvs status" only returns the basename (and the complete path on the
server which is useless for our purposes), run "cvs status" several times
with lists consisting of files with unique (chomped) basenames.

This makes cvsexportcommit slightly slower, when the list of changed files
has non-unique basenames, but at least it is accurate now.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	So I finally broke down.  I really need a working cvsexportcommit, 
	everything else is just too painful.

	Feel free to criticise/educate me on my Perl style.

 git-cvsexportcommit.perl       |   33 +++++++++++++++++++++++++--------
 t/t9200-git-cvsexportcommit.sh |   23 +++++++++++++++++++++++
 2 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index 2a8ad1e..7fd05c7 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -197,15 +197,32 @@ 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++;
+    # "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++) {
+        my $name = $canstatusfiles[$i];
+	my $basename = $name;
+	$basename =~ s/.*\///;
+	$basename = "no file " . $basename if (grep {$_ eq $basename} @afiles);
+	chomp($basename);
+	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});
+          }
+      }
     }
 }
 
diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index 49d57a8..483d8fa 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -262,4 +262,27 @@ test_expect_success '-w option should work with relative GIT_DIR' '
       )
 '
 
+test_expect_success 'check files before directories' '
+
+	echo Notes > release-notes &&
+	git add release-notes &&
+	git commit -m "Add release notes" release-notes &&
+	id=$(git rev-parse HEAD) &&
+	git cvsexportcommit -w "$CVSWORK" -c $id &&
+
+	echo new > DS &&
+	echo new > E/DS &&
+	echo modified > release-notes &&
+	git add DS E/DS release-notes &&
+	git commit -m "Add two files with the same basename" &&
+	id=$(git rev-parse HEAD) &&
+	git cvsexportcommit -w "$CVSWORK" -c $id &&
+	check_entries "$CVSWORK/E" "DS/1.1/|newfile5.txt/1.1/" &&
+	check_entries "$CVSWORK" "DS/1.1/|release-notes/1.2/" &&
+	diff -u "$CVSWORK/DS" DS &&
+	diff -u "$CVSWORK/E/DS" E/DS &&
+	diff -u "$CVSWORK/release-notes" release-notes
+
+'
+
 test_done
-- 
1.5.4.2.217.g468be

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

[Junio C Hamano]

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

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> 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'}
> ...
> +    while (@canstatusfiles) {
> +	    my %basename = ();
> +	    my @status = ();
> +	    my @leftover = ();
> +	    for (my $i = 0; $i < @canstatusfiles; $i++) {
> +		    my $name = $canstatusfiles[$i];
> +		    my $basename = basename($name);

Side note to the side note.

A related naming guideline I failed to follow (because I was
mostly copying your code) suggests that the hash here should be
named %fullname, instead of %basename.  Then logically:

	$fullname{'hello.c'} = 'a/b/hello.c';

that is, you consult %fullname hash using the basename as the
key to extract the corresponding fullname.  The naming guideline
is "Name the dictionary after its values, not after its keys."

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

[Martin Langhoff]

Junio C Hamano wrote:
> A related naming guideline I failed to follow (because I was
> mostly copying your code) suggests that the hash here should be
> named %fullname, instead of %basename.  Then logically:

Double ACK on your logic and arguments - I was thinking "fullname" as I
read your first email. Not sure how stable the output is across CVS
versions/ports WRT leading slashes, might be a good idea to try to
canonicalise the paths.

I am travelling at the moment, but I'll try and review the patch with
the actual code. Some of the ugliness you're complaining about might be
mine (plurals, and perhaps even the $#array) but I refuse to recognise
the grep as mine.

Annotate might still put me to shame - perhaps I was drunk?

cheers,


m
-- 
-----------------------------------------------------------------------
Martin @ Catalyst .Net .NZ  Ltd, PO Box 11-053, Manners St,  Wellington
WEB: http://catalyst.net.nz/           PHYS: Level 2, 150-154 Willis St
NZ: +64(4)916-7224    MOB: +64(21)364-017    UK: 0845 868 5733 ext 7224
      Make things as simple as possible, but no simpler - Einstein
-----------------------------------------------------------------------

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

[Johannes Schindelin]

Hi,

On Tue, 19 Feb 2008, Martin Langhoff wrote:

> Junio C Hamano wrote:
> > A related naming guideline I failed to follow (because I was mostly 
> > copying your code) suggests that the hash here should be named 
> > %fullname, instead of %basename.  Then logically:
> 
> Double ACK on your logic and arguments - I was thinking "fullname" as I 
> read your first email.

Okay, will change.

> Not sure how stable the output is across CVS versions/ports WRT leading 
> slashes, might be a good idea to try to canonicalise the paths.

Note that for this reason, only the "File:" output -- which does not show 
slashes, but only the basenames -- is used to match the files.  We need 
the full path in the git repository, though, to apply the patches.

> I am travelling at the moment, but I'll try and review the patch with 
> the actual code.

Thanks.  I am confident that I will have posted another version by the 
time you come around to review it.

Ciao,
Dscho

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

[Martin Langhoff]

Johannes Schindelin wrote:
> Note that for this reason, only the "File:" output -- which does not show 
> slashes, but only the basenames -- is used to match the files.  We need 
> the full path in the git repository, though, to apply the patches.

Yes - that's ugly. We have a couple of options

 - Run cvs status once per directory we touch. Use -l tomake it
non-recursive. It will be a tad slower/chattier.

 - Parse the 'Repository revision:' line to find out what path on the
server matches our repo 'root'.

> Thanks.  I am confident that I will have posted another version by the 
> time you come around to review it.

Great! Careful, you might end up enjoying Perl ;-)



m
-- 

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

[Johannes Schindelin]

Hi,

On Tue, 19 Feb 2008, Martin Langhoff wrote:

> Johannes Schindelin wrote:
>
> > Note that for this reason, only the "File:" output -- which does not 
> > show slashes, but only the basenames -- is used to match the files.  
> > We need the full path in the git repository, though, to apply the 
> > patches.
> 
> Yes - that's ugly. We have a couple of options
> 
>  - Run cvs status once per directory we touch. Use -l tomake it
> non-recursive. It will be a tad slower/chattier.

I think that my approach is a bit faster: make lists with unique 
basenames.  In the most common case, there will be only one list, I 
suspect.

>  - Parse the 'Repository revision:' line to find out what path on the
> server matches our repo 'root'.

I am not so sure.  It _should_ be reconstructible by CVS/Repository + 
dirname + basename, but I guess that it makes us susceptible to other CVS 
breakages.  Whereas I think that we will be fine, relying on the basename 
in the File: line.

> > Thanks.  I am confident that I will have posted another version by the 
> > time you come around to review it.
> 
> Great! Careful, you might end up enjoying Perl ;-)

Heh.  I always said that I like Perl, because it's easier to tell who's 
an expert, than for, say, Python.  Unfortunately, that means that it is 
easy to see that I am not an expert myself :-(

Ciao,
Dscho

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

[Johannes Schindelin]

Hi,

On Sun, 17 Feb 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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

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

[Johannes Schindelin]

In my use cases, "cvs status" sometimes reordered the passed filenames,
which often led to a misdetection of a dirty state (when it was in
reality a clean state).

I finally tracked it down to two filenames having the same basename.

So no longer trust the order of the results blindly, but actually check
the file name.

Since "cvs status" only returns the basename (and the complete path on the
server which is useless for our purposes), run "cvs status" several times
with lists consisting of files with unique (chomped) basenames.

Be a bit clever about new files: these are reported as "no file <blabla>",
so in order to discern it from existing files, prepend "no file " to the
basename.

In other words, one call to "cvs status" will not ask for two files
"blabla" (which does not yet exist) and "no file blabla" (which exists).

This patch makes cvsexportcommit slightly slower, when the list of changed
files has non-unique basenames, but at least it is accurate now.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	This should address your concerns, Junio!

 git-cvsexportcommit.perl       |   40 ++++++++++++++++++++++++++++++++--------
 t/t9200-git-cvsexportcommit.sh |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 8 deletions(-)

diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index 2a8ad1e..c00368b 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -197,15 +197,39 @@ 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++;
+    # "cvs status" reorders the parameters, notably when there are multiple
+    # arguments with the same basename.  So be precise here.
+
+    my %added = map { $_ => 1 } @afiles;
+    my %todo = map { $_ => 1 } @canstatusfiles;
+
+    while (%todo) {
+      my @canstatusfiles2 = ();
+      my %fullname = ();
+      foreach my $name (keys %todo) {
+	my $basename = $name;
+	$basename =~ s/.*\///;
+	$basename = "no file " . $basename if (exists($added{$basename}));
+	chomp($basename);
+
+	if (!exists($fullname{$basename})) {
+	  $fullname{$basename} = $name;
+	  push (@canstatusfiles2, $name);
+	  delete($todo{$name});
         }
+      }
+      my @cvsoutput;
+      @cvsoutput = xargs_safe_pipe_capture([@cvs, 'status'], @canstatusfiles2);
+      foreach my $l (@cvsoutput) {
+        chomp $l;
+        if ($l =~ /^File:\s+(.*\S)\s+Status: (.*)$/) {
+	  if (!exists($fullname{$1})) {
+	    print STDERR "Huh? Status reported for unexpected file '$1'\n";
+	  } else {
+	    $cvsstat{$fullname{$1}} = $2;
+	  }
+	}
+      }
     }
 }
 
diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index 49d57a8..a55a203 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -262,4 +262,39 @@ test_expect_success '-w option should work with relative GIT_DIR' '
       )
 '
 
+test_expect_success 'check files before directories' '
+
+	echo Notes > release-notes &&
+	git add release-notes &&
+	git commit -m "Add release notes" release-notes &&
+	id=$(git rev-parse HEAD) &&
+	git cvsexportcommit -w "$CVSWORK" -c $id &&
+
+	echo new > DS &&
+	echo new > E/DS &&
+	echo modified > release-notes &&
+	git add DS E/DS release-notes &&
+	git commit -m "Add two files with the same basename" &&
+	id=$(git rev-parse HEAD) &&
+	git cvsexportcommit -w "$CVSWORK" -c $id &&
+	check_entries "$CVSWORK/E" "DS/1.1/|newfile5.txt/1.1/" &&
+	check_entries "$CVSWORK" "DS/1.1/|release-notes/1.2/" &&
+	diff -u "$CVSWORK/DS" DS &&
+	diff -u "$CVSWORK/E/DS" E/DS &&
+	diff -u "$CVSWORK/release-notes" release-notes
+
+'
+
+test_expect_success 'commit a file with leading spaces in the name' '
+
+	echo space > " space" &&
+	git add " space" &&
+	git commit -m "Add a file with a leading space" &&
+	id=$(git rev-parse HEAD) &&
+	git cvsexportcommit -w "$CVSWORK" -c $id &&
+	check_entries "$CVSWORK" " space/1.1/|DS/1.1/|release-notes/1.2/" &&
+	diff -u "$CVSWORK/ space" " space"
+
+'
+	
 test_done
-- 
1.5.4.2.217.g468be

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> 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.

The reason why "basename" is not being used needs documented in
the code then.  The next person will likely to make the same
mistake as I did.

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

[Johannes Schindelin]

Hi,

On Mon, 18 Feb 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> 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.
> 
> The reason why "basename" is not being used needs documented in the code 
> then.  The next person will likely to make the same mistake as I did.

Right.  Could you be so good and squash this in, then, please?

Thanks,
Dscho

-- snipsnap --
diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index c00368b..6eff42c 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -208,6 +208,7 @@ if (@canstatusfiles) {
       my %fullname = ();
       foreach my $name (keys %todo) {
 	my $basename = $name;
+	# cannot use basename(), since $name need not exist in the CVS tree
 	$basename =~ s/.*\///;
 	$basename = "no file " . $basename if (exists($added{$basename}));
 	chomp($basename);

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

[Martin Langhoff]

Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
>>> 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.

?! basename() never touches the disk. I just read it to confirm -
$VERSION is 2.74 and I'm somewhat disappointed to find it's not as
portable as I'd expect (perhaps it gets hardcoded during install?).

And my /usr/bin/basename doesn't care if the file exists either

  $ type basename
  basename is /usr/bin/basename
  $ basename /foo/bar/baz
  baz
  $ stat /foo/bar/baz
  stat: cannot stat `/foo/bar/baz': No such file or directory

So I am fairly confident that we can safely use File::Basename's
basename() on arbitrary strings that look like a path. We use basename()
quite a bit in our perl scripts in git.

cheers,


m
-- 
-----------------------------------------------------------------------
Martin @ Catalyst .Net .NZ  Ltd, PO Box 11-053, Manners St,  Wellington
WEB: http://catalyst.net.nz/           PHYS: Level 2, 150-154 Willis St
NZ: +64(4)916-7224    MOB: +64(21)364-017    UK: 0845 868 5733 ext 7224
      Make things as simple as possible, but no simpler - Einstein
-----------------------------------------------------------------------

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

[Johannes Schindelin]

Hi,

On Tue, 19 Feb 2008, Martin Langhoff wrote:

> ?! basename() never touches the disk. I just read it to confirm - 
> $VERSION is 2.74 and I'm somewhat disappointed to find it's not as 
> portable as I'd expect (perhaps it gets hardcoded during install?).

Well, please try for yourself.  If it works for you, then I probably had 
another error in my patch.

Ciao,
Dscho

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

[Martin Langhoff]

Johannes Schindelin wrote:
> Well, please try for yourself.  If it works for you, then I probably had 
> another error in my patch.

$ perl -MFile::Basename -e 'print basename("/foo/bar/baz");'
baz

Johannes, what are you smoking? No PUI here! ;-)



m
-- 
-----------------------------------------------------------------------
Martin @ Catalyst .Net .NZ  Ltd, PO Box 11-053, Manners St,  Wellington
WEB: http://catalyst.net.nz/           PHYS: Level 2, 150-154 Willis St
NZ: +64(4)916-7224    MOB: +64(21)364-017    UK: 0845 868 5733 ext 7224
      Make things as simple as possible, but no simpler - Einstein
-----------------------------------------------------------------------

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

[Johannes Schindelin]

Hi,

On Tue, 19 Feb 2008, Martin Langhoff wrote:

> Johannes Schindelin wrote:
>
> > Well, please try for yourself.  If it works for you, then I probably 
> > had another error in my patch.
> 
> $ perl -MFile::Basename -e 'print basename("/foo/bar/baz");'
> baz
> 
> Johannes, what are you smoking? No PUI here! ;-)

Unfortunately, I am not smoking, because my throat is inflamed...

Checked again, and sure enough, it works.  So, this is a replacement patch 
to be squashed in... So: I'm sorry...

Ciao,
Dscho

-- snipsnap --
 git-cvsexportcommit.perl |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index c00368b..b8114f7 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -207,8 +207,7 @@ if (@canstatusfiles) {
       my @canstatusfiles2 = ();
       my %fullname = ();
       foreach my $name (keys %todo) {
-	my $basename = $name;
-	$basename =~ s/.*\///;
+	my $basename = basename($name);
 	$basename = "no file " . $basename if (exists($added{$basename}));
 	chomp($basename);