Re: [PATCH] Make cvsexportcommit work with filenames with spaces and non-ascii characters.

Robin Rosenberg [off-list ref] writes:

From: Robin Rosenberg <redacted>

This patch uses git-apply to do the patching which simplifies the code a lot.

Removed the test for checking for matching binary files when deleting them
since git-apply happily deletes the file. This is matter of taste since we
allow some fuzz for text patches also.

Error handling was cleaned up, but not much testd (it did not work before).

Signed-off-by: Robin Rosenberg <redacted>

Thanks.  Allow me to put this in freezer for a few days while
releasing v1.4.4.

Some comments below.

my @fields = ($f =~ m/^.../);

 my (@binfiles, @abfiles, @dbfiles, @bfiles, @mbfiles);
 @binfiles = grep m/^Binary files/, safe_pipe_capture('git-diff-tree', '-p', $parent, $commit);
 map { chomp } @binfiles;
 @abfiles = grep s/^Binary files \/dev\/null and b\/(.*) differ$/$1/, @binfiles;
 @dbfiles = grep s/^Binary files a\/(.*) and \/dev\/null differ$/$1/, @binfiles;
 @mbfiles = grep s/^Binary files a\/(.*) and b\/(.*) differ$/$1/, @binfiles;
+push @abfiles, grep s/^Binary files \/dev\/null and "b\/(.*)" differ$/$1/, @binfiles;
+push @dbfiles, grep s/^Binary files "a\/(.*)" and \/dev\/null differ$/$1/, @binfiles;
+push @mbfiles, grep s/^Binary files "a\/(.*)" and \"b\/(.*)\" differ$/$1/, @binfiles;
+map { s/\\([\d]{3})/sprintf('%c',oct $1)/eg; } @abfiles;
+map { s/\\([\d]{3})/sprintf('%c',oct $1)/eg; } @dbfiles;
+map { s/\\([\d]{3})/sprintf('%c',oct $1)/eg; } @mbfiles;
+

Logically these map {} should be done only on the c-quoted
names, but it is Ok because the names that have backslash with
octal are quoted.  However, what's inside the map is not correct
c dequoting (see how Jakub does it in gitweb -- you need to
worry about \\, \" and such).

I think it is perhaps safer to parse "diff --git" and remember
the filenames of the "current patch" and then notice only
/^Binary files / part of the message.

But all of the above shows deficiency in the current set of
tools -- they are not helping Porcelain writers enough.  I think
we should enhance 'apply --numstat' to let it show binary diffs
differently:

	git diff-tree -p $parent $commit >.tmpfile
        git apply --numstat -z <.tmpfile

would currently say "0 0" for binary files (the primary benefit
of using "--numstat -z" here is that it would give Perl scripts
pathnames parsable without C dequoting).  We should somehow have
a way to show it differently from text files without any
added/deleted lines (e.g. only the mode change), and that would
make the life of Porcelain writers who needs to write something
like the above code much more pleasant.  Perhaps show "- -"
instead of "0 0", since there is no notion of lines in "binary
files differ" case?

-    `cvs add $d`;
-    if ($?) {
-	$dirty = 1;
+    print "Adding directory $d";
+    if (system('cvs','add',$d)) {
+	$dirtypatch = 1;

Good.

 ## apply non-binary changes
 my $fuzz = $opt_p ? 0 : 2;
 
-print "Patching non-binary files\n";
+print "Patching\n";

Leftover comment that does not apply anymore.

 if (scalar(@afiles)+scalar(@dfiles)+scalar(@mfiles) != scalar(@bfiles)) {
+    `git-diff-tree --binary -z -p $parent -p $commit >.cvsexportcommit.diff`;

Haven't you run this diff before to grep for "Binary files..."
Maybe doing a temporary file upfront once and using it would
make sense.

Also why multiple -p?  I do not think -z is wanted here, as -z
affects only output side of git apply and not input side (see
the above comment on --numstat -z).