threads / patch / 12166

patchcvsexportcommit: be graceful when "cvs status" reorders the arguments

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

## tl;dr

15 messages between Feb 18, 2008 and Feb 18, 2008. Diffs are folded; open one to read it.

replies: 14people: 3as markdown or json

Johannes Schindelin· Feb 18, 2008, 01:31 UTC · lore

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(-)
Show changes to 2 files +48 −8

git-cvsexportcommit.perl, t/t9200-git-cvsexportcommit.sh

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
Junio C Hamano· Feb 18, 2008, 03:03 UTC · re: Johannes Schindelin · lore

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

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 ;-)
Show 6 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++) {
> +        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?
Show 7 quoted lines
> +	if (!defined($basenames{$basename})) {
> +	  $basenames{$basename} = $name;
> +	  push (@canstatusfiles2, $name);
> +	  splice (@canstatusfiles, $i, 1);
> +	  $i--;
>          }
> +      }
Show 8 quoted lines
> +      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(-)
Show changes to git-cvsexportcommit.perl +34 −9
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;
     }
 }
Junio C Hamano· Feb 18, 2008, 03:20 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 15 quoted lines
> 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."

Martin Langhoff· Feb 18, 2008, 15:25 UTC · re: Junio C Hamano · lore

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

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
-----------------------------------------------------------------------
Johannes Schindelin· Feb 18, 2008, 16:27 UTC · re: Martin Langhoff · lore

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

Hi,
On Tue, 19 Feb 2008, Martin Langhoff wrote:
Show 7 quoted lines
> 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

Martin Langhoff· Feb 18, 2008, 16:33 UTC · re: Johannes Schindelin · lore

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

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
Johannes Schindelin· Feb 18, 2008, 17:43 UTC · re: Martin Langhoff · lore

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

Hi,
On Tue, 19 Feb 2008, Martin Langhoff wrote:
Show 11 quoted lines
> 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

Johannes Schindelin· Feb 18, 2008, 17:55 UTC · re: Martin Langhoff · lore

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

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(-)
Show changes to 2 files +67 −8

git-cvsexportcommit.perl, t/t9200-git-cvsexportcommit.sh

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
Johannes Schindelin· Feb 18, 2008, 17:54 UTC · re: Junio C Hamano · lore

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

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

Junio C Hamano· Feb 18, 2008, 18:36 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 5 quoted lines
>> 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.

Johannes Schindelin· Feb 18, 2008, 18:50 UTC · re: Junio C Hamano · lore

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

Hi,
On Mon, 18 Feb 2008, Junio C Hamano wrote:
Show 11 quoted lines
> 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 --
Show changes to git-cvsexportcommit.perl +1 −0
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);
Martin Langhoff· Feb 18, 2008, 18:55 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 6 quoted lines
> 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
-----------------------------------------------------------------------
Johannes Schindelin· Feb 18, 2008, 19:42 UTC · re: Martin Langhoff · lore

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

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

Martin Langhoff· Feb 18, 2008, 20:06 UTC · re: Johannes Schindelin · lore

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

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
-----------------------------------------------------------------------
Johannes Schindelin· Feb 18, 2008, 20:29 UTC · re: Martin Langhoff · lore

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

Hi,
On Tue, 19 Feb 2008, Martin Langhoff wrote:
Show 9 quoted lines
> 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(-)
Show changes to git-cvsexportcommit.perl +1 −2
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);

← back to recent threads