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

From: Junio C Hamano <junkio@cox.net>
Date: Nov 15, 2006, 06:08 UTC
Message-ID: <7v8xidqm1e.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20061115005530.26560.18222.stgit@lathund.dewire.com>

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

Show 11 quoted lines

> From: Robin Rosenberg <robin.rosenberg@dewire.com>
>
> 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 <robin.rosenberg@dewire.com>

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

Some comments below.

Show 18 quoted lines

> @@ -116,12 +115,18 @@ if ($opt_a) {
>  close MSG;
>  
>  my (@afiles, @dfiles, @mfiles, @dirs);
> -my @files = safe_pipe_capture('git-diff-tree', '-r', $parent, $commit);
> -#print @files;
> +my $files = safe_pipe_capture_blob('git-diff-tree', '-z', '-r', $parent, $commit);
>  $? && die "Error in git-diff-tree";
> +while ($files =~ m/(.*?\000.*?\000)/g) {
> +    my $f=$1;
> +    $f =~ m/^(\S+) (\S+) (\S+) (\S+) (\S+)\000(.*)\000/;
> +    my @fields = ();
> +    $fields[++$#fields] = $1;
> +    $fields[++$#fields] = $2;
> +    $fields[++$#fields] = $3;
> +    $fields[++$#fields] = $4;
> +    $fields[++$#fields] = $5;
> +    $fields[++$#fields] = $6;

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

Show 13 quoted lines

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

Show 6 quoted lines

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

Good.

Show 5 quoted lines

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