[PATCH] Make cvsexportcommit work with filenames containing spaces.

[PATCH] Make cvsexportcommit work with filenames containing spaces.

[Robin Rosenberg]

From: Robin Rosenberg <robin.rosenberg@dewire.com>

Binary files are except to this so far.
---

 git-cvsexportcommit.perl       |   24 +++++++++++++++--
 t/t9200-git-cvsexportcommit.sh |   55 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 3 deletions(-)

diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index 99b3dc3..d78100c 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -121,7 +121,14 @@ #print @files;
 $? && die "Error in git-diff-tree";
 foreach my $f (@files) {
     chomp $f;
-    my @fields = split(m!\s+!, $f);
+    $f =~ m/^(\S+) (\S+) (\S+) (\S+) (\S+)	(.*)/;
+    my @fields = ();
+    $fields[++$#fields] = $1;
+    $fields[++$#fields] = $2;
+    $fields[++$#fields] = $3;
+    $fields[++$#fields] = $4;
+    $fields[++$#fields] = $5;
+    $fields[++$#fields] = $6;
     if ($fields[4] eq 'A') {
         my $path = $fields[5];
 	push @afiles, $path;
@@ -217,7 +224,7 @@ foreach my $f (@bfiles) {
     }
 
     # replace with the new file
-     `git-cat-file blob $blob > $f`;
+     `git-cat-file blob $blob > "$f"`;
 
     # TODO: something smart with file modes
 
@@ -231,7 +238,18 @@ ## apply non-binary changes
 my $fuzz = $opt_p ? 0 : 2;
 
 print "Patching non-binary files\n";
-print `(git-diff-tree -p $parent -p $commit | patch -p1 -F $fuzz ) 2>&1`;
+
+my $saveslash = $/;
+undef $/;
+
+open DIFF, "git-diff-tree -p $parent -p $commit|" || die "Cannot diff";
+open PATCH, "|patch -p1 -F $fuzz" || die "Cannot patch";
+my $delta = <DIFF>;
+close DIFF || die "Could not diff";
+$delta =~ s/\n(index [^\n]*)\n(--- [^\n]*)\n(\+\+\+ [^\n]*)\n(@@[^\n]*@@)\n/$1\n$2\t\n$3\t\n$4\n/sg;
+print PATCH $delta;
+close PATCH || die "Could not patch";
+$/ = $saveslash;
 
 my $dirtypatch = 0;
 if (($? >> 8) == 2) {
diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
new file mode 100755
index 0000000..1041bb6
--- /dev/null
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -0,0 +1,55 @@
+#!/bin/bash
+
+test_description='CVS export comit. 
+
+These tests are ad-hoc ones made to test
+some changes, not a complete test.'
+
+. ./test-lib.sh
+
+export CVSROOT=$(dirname $(pwd))/cvsroot
+export CVSWORK=$(dirname $(pwd))/cvswork
+rm -rf $CVSROOT $CVSWORK
+mkdir $CVSROOT
+cvs init
+cvs -Q co -d $CVSWORK .
+export GIT_DIR=$(pwd)/.git
+
+echo >empty &&
+git add empty &&
+git commit -a -m "Initial"
+
+test_expect_success \
+    'New file' \
+    'echo hello >newfile.txt &&
+     git add newfile.txt &&
+     git commit -a -m "Hello" &&
+     id=$(git rev-list --max-count=1 HEAD) &&
+     (cd $CVSWORK &&
+     git cvsexportcommit -c $id &&
+     test $(cat CVS/Entries|wc -l) = 2
+     )'
+
+test_expect_success \
+     'New file with spaces in file name' \
+     'echo ok then >"with spaces.txt" &&
+      git add "with spaces.txt" && \
+      git commit -a -m "With spaces" &&
+      id=$(git rev-list --max-count=1 HEAD) &&
+      (cd $CVSWORK &&
+      git-cvsexportcommit.perl -c $id &&
+      test $(cat CVS/Entries|wc -l) = 3
+      )'
+
+test_expect_success \
+     'Update file with spaces in file name' \
+     'echo Ok then >>"with spaces.txt" &&
+      git add "with spaces.txt" &&
+      git commit -a -m "Update with spaces" &&
+      id=$(git rev-list --max-count=1 HEAD) &&
+      (cd $CVSWORK &&
+      git-cvsexportcommit.perl -c $id &&
+      test $(cat CVS/Entries|wc -l) = 3
+      )'
+     
+test_done

Re: [PATCH] Make cvsexportcommit work with filenames containing spaces.

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg@dewire.com> writes:

> Binary files are except to this so far.

Funny.  This works on my home machine (CVS 1.12.13) while it
does not pass test #2 "with spaces" on another machine that has
CVS 1.11.22.

> +. ./test-lib.sh
> +
> +export CVSROOT=$(dirname $(pwd))/cvsroot
> +export CVSWORK=$(dirname $(pwd))/cvswork

You are creating t/{cvsroot,cvswork} directories.  Do not
contaminate outside the test/trash directory your tests is
started in.

> +rm -rf $CVSROOT $CVSWORK

People's $(pwd) can contain shell $IFS characters, especially on
Cygwin.   Be careful and quote them when in doubt.

Re: [PATCH] Make cvsexportcommit work with filenames containing spaces.

[Robin Rosenberg]

lördag 23 september 2006 06:17 skrev Junio C Hamano:
> Robin Rosenberg <robin.rosenberg@dewire.com> writes:
> > Binary files are except to this so far.
>
> Funny.  This works on my home machine (CVS 1.12.13) while it
> does not pass test #2 "with spaces" on another machine that has
> CVS 1.11.22.
That must be something else. I've tried with various cvs versions here 
(including plain 1.11.22) on Mandriva and SLES and it works on all with the 
lastest git on master (and 1.4.2.1). 

Could you give me some more info, like the output from -v on the failing 
machine and your perl and patch version numbers and locale? I'm just guessing 
what could matter here.  

Why patch? Well this patch works around (i.e. not perfect) a mismatch between 
what patch eats and git submits. They are not totally, compatible, and I'm 
not sure who to blame yet.  git emits diff's without timestamps, and what 
matters to patch, without a TAB before the file timestamp. When patch sees a 
header like "+++ filename with spaces.txt" it patches "filename". When it 
sees "+++ filename with spaces.txt<TAB>" if patches "filename with 
spaces.txt". The real fix would ofcourse be in git diff or patch sometime in 
the future.

> > +. ./test-lib.sh
> > +
> > +export CVSROOT=$(dirname $(pwd))/cvsroot
> > +export CVSWORK=$(dirname $(pwd))/cvswork
acknowledged

> You are creating t/{cvsroot,cvswork} directories.  Do not
> contaminate outside the test/trash directory your tests is
> started in.
>
> > +rm -rf $CVSROOT $CVSWORK
>
> People's $(pwd) can contain shell $IFS characters, especially on
> Cygwin.   Be careful and quote them when in doubt.

Considering this is a patch that should improve on whitespace handlung... ok. 
I'll resubmit later.

-- robin

Re: [PATCH] Make cvsexportcommit work with filenames containing spaces.

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg@dewire.com> writes:

> Why patch? Well this patch works around (i.e. not perfect) a mismatch between 
> what patch eats and git submits. They are not totally, compatible, and I'm 
> not sure who to blame yet.  git emits diff's without timestamps, and what 
> matters to patch, without a TAB before the file timestamp. When patch sees a 
> header like "+++ filename with spaces.txt" it patches "filename". When it 
> sees "+++ filename with spaces.txt<TAB>" if patches "filename with 
> spaces.txt". The real fix would ofcourse be in git diff or patch sometime in 
> the future.

Ah, it might be because our diff output do not have trailing TAB
(and timestamp) and if CVS uses GNU patch that would be confused
and not detect the file being patched.

Interestingly, I did a two-patch series to address that issue
separately.  But that is not even in "next" yet.

More interestingly, the version of "GNU patch" that cannot grok
our patch (without trailing TAB) for a file that has SP in its
name is on my home machine where your patched cvsexportcommit
works.  I do not know if GNU patch on the other machine does.

Will collect necessary info and follow up.

Re: [PATCH] Make cvsexportcommit work with filenames containing spaces.

[Robin Rosenberg]

[-- Attachment #1: Type: text/plain, Size: 359 bytes --]


Jounio. Thanks for the feedback. It is your fedora patch 2.5.4 that doesn't work. 
It doesn't seem to handle spaces in filenames in any way. Not much I can do about that.

Btw, 2.5.4 is nine years old, but strangely that's the one that's around everywhere on
source mirrors, but most (=the few ones I checked) Linux distros have 2.5.9 (except RH).

-- robin

[-- Attachment #2: cvsexportcommit --]
[-- Type: text/x-diff, Size: 3929 bytes --]

Make cvsexportcommit work with filenames containing spaces.

From: Robin Rosenberg <robin.rosenberg@dewire.com>

Binary files are except to this so far.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---

 git-cvsexportcommit.perl       |   27 +++++++++++++++++---
 t/t9200-git-cvsexportcommit.sh |   55 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index 99b3dc3..ecded35 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -5,6 +5,7 @@ # - cannot add or remove binary files
 # - does not propagate permissions
 # - tells "ready for commit" even when things could not be completed
 #   (eg addition of a binary file)
+# - Fedora/RHEL uses patch 2.5.4 which doesn't handles spaces in file names
 
 use strict;
 use Getopt::Std;
@@ -121,7 +122,14 @@ #print @files;
 $? && die "Error in git-diff-tree";
 foreach my $f (@files) {
     chomp $f;
-    my @fields = split(m!\s+!, $f);
+    $f =~ m/^(\S+) (\S+) (\S+) (\S+) (\S+)	(.*)/;
+    my @fields = ();
+    $fields[++$#fields] = $1;
+    $fields[++$#fields] = $2;
+    $fields[++$#fields] = $3;
+    $fields[++$#fields] = $4;
+    $fields[++$#fields] = $5;
+    $fields[++$#fields] = $6;
     if ($fields[4] eq 'A') {
         my $path = $fields[5];
 	push @afiles, $path;
@@ -217,7 +225,7 @@ foreach my $f (@bfiles) {
     }
 
     # replace with the new file
-     `git-cat-file blob $blob > $f`;
+     `git-cat-file blob $blob > "$f"`;
 
     # TODO: something smart with file modes
 
@@ -231,7 +239,20 @@ ## apply non-binary changes
 my $fuzz = $opt_p ? 0 : 2;
 
 print "Patching non-binary files\n";
-print `(git-diff-tree -p $parent -p $commit | patch -p1 -F $fuzz ) 2>&1`;
+
+my $saveslash = $/;
+undef $/;
+
+open DIFF, "git-diff-tree -p $parent -p $commit|" || die "Cannot diff";
+open PATCH, "|patch -p1 -F $fuzz" || die "Cannot patch";
+my $delta = <DIFF>;
+close DIFF || die "Could not diff";
+unless (defined $ENV{'GIT_CVSEXPORTCOMMIT_NO_SPACES'}) {
+  $delta =~ s/\n(index [^\n]*)\n(--- [^\n]*)\n(\+\+\+ [^\n]*)\n(@@[^\n]*@@)\n/$1\n$2\t\n$3\t\n$4\n/sg
+}
+print PATCH $delta;
+close PATCH || die "Could not patch";
+$/ = $saveslash;
 
 my $dirtypatch = 0;
 if (($? >> 8) == 2) {
diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
new file mode 100755
index 0000000..548e329
--- /dev/null
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -0,0 +1,55 @@
+#!/bin/bash
+
+test_description='CVS export comit. 
+
+These tests are ad-hoc ones made to test
+some changes, not a complete test.'
+
+. ./test-lib.sh
+
+export CVSROOT=$(pwd)/cvsroot
+export CVSWORK=$(pwd)/cvswork
+rm -rf "$CVSROOT" "$CVSWORK"
+mkdir "$CVSROOT" &&
+cvs init &&
+cvs -Q co -d "$CVSWORK" . &&
+export GIT_DIR=$(pwd)/.git &&
+echo >empty &&
+git add empty &&
+git commit -a -m "Initial" 2>/dev/null ||
+exit 1
+
+test_expect_success \
+    'New file' \
+    'echo hello >newfile.txt &&
+     git add newfile.txt &&
+     git commit -a -m "Hello" &&
+     id=$(git rev-list --max-count=1 HEAD) &&
+     (cd "$CVSWORK" &&
+     git cvsexportcommit -c $id &&
+     test $(cat CVS/Entries|wc -l) = 2
+     )'
+
+test_expect_success \
+     'New file with spaces in file name' \
+     'echo ok then >"with spaces.txt" &&
+      git add "with spaces.txt" && \
+      git commit -a -m "With spaces" &&
+      id=$(git rev-list --max-count=1 HEAD) &&
+      (cd "$CVSWORK" &&
+      git-cvsexportcommit -c $id &&
+      test $(cat CVS/Entries|wc -l) = 3
+      )'
+
+test_expect_success \
+     'Update file with spaces in file name' \
+     'echo Ok then >>"with spaces.txt" &&
+      git add "with spaces.txt" &&
+      git commit -a -m "Update with spaces" &&
+      id=$(git rev-list --max-count=1 HEAD) &&
+      (cd "$CVSWORK" &&
+      git-cvsexportcommit -c $id &&
+      test $(cat CVS/Entries|wc -l) = 3
+      )'
+     
+test_done

[-- Attachment #3: cvsexportcommitrm --]
[-- Type: text/x-diff, Size: 1352 bytes --]

Make cvsexportcommit remove files.

From: Robin Rosenberg <robin.rosenberg@dewire.com>

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---

 git-cvsexportcommit.perl       |    2 +-
 t/t9200-git-cvsexportcommit.sh |   12 ++++++++++++
 2 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index ecded35..4fb55a6 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -143,7 +143,7 @@ foreach my $f (@files) {
     if ($fields[4] eq 'M') {
 	push @mfiles, $fields[5];
     }
-    if ($fields[4] eq 'R') {
+    if ($fields[4] eq 'D') {
 	push @dfiles, $fields[5];
     }
 }
diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index 548e329..6f28da6 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -52,4 +52,16 @@ test_expect_success \
       test $(cat CVS/Entries|wc -l) = 3
       )'
      
+test_expect_success \
+     'Remove file with spaces in file name' \
+     'echo Ok then >"with spaces.txt" &&
+      rm -v "with spaces.txt" && \
+      git rm "with spaces.txt" && \
+      git commit -a -m "Remove file" &&
+      id=$(git rev-list --max-count=1 HEAD) &&
+      (cd "$CVSWORK" &&
+      git-cvsexportcommit -v -c $id &&
+      test $(cat CVS/Entries|wc -l) = 2
+      )'
+     
 test_done

Re: [PATCH] Make cvsexportcommit work with filenames containing spaces.

[Junio C Hamano]

Please do not do more than one patch per e-mail; I'll have to
save the attachment in separate files and manually make commits,
which is more work.

The build procedure for the release to build rpms runs testsuite
as part of it, so I need to think a bit how to proceed with this
patch.  Leaving the test failing on FC5 means I won't be able to
cut binary releases.  Checking early in the test script to see
if "patch" can grok a diff for a file with whitespaces, and
skipping the whitespace test if we have a bad "patch", seems to
the best workaround.

BTW, the addition of TAB to the patch tail is done by "git diff"  
automatically for files with whitespace in them in the proposed
updates version in "pu" (and it is done conditionally only for
files whose names have whitespace), so I think your fix will
become redundant when it graduates to "master".

Re: [PATCH] Make cvsexportcommit work with filenames containing spaces.

[Robin Rosenberg]

fredag 29 september 2006 08:37 skrev Junio C Hamano:
> Please do not do more than one patch per e-mail; I'll have to
> save the attachment in separate files and manually make commits,
> which is more work.
>
> The build procedure for the release to build rpms runs testsuite
> as part of it, so I need to think a bit how to proceed with this
> patch.  Leaving the test failing on FC5 means I won't be able to
> cut binary releases.  Checking early in the test script to see
> if "patch" can grok a diff for a file with whitespaces, and
> skipping the whitespace test if we have a bad "patch", seems to
> the best workaround.
>
> BTW, the addition of TAB to the patch tail is done by "git diff"
> automatically for files with whitespace in them in the proposed
> updates version in "pu" (and it is done conditionally only for
> files whose names have whitespace), so I think your fix will
> become redundant when it graduates to "master".

But, then I can stop here, because that sounds like a much better solution. My
hack is kind-of-kludgy anyway. I'll look in the pu branch. Still want the test 
cases, although not complete?

-- robin