# [PATCH] Make cvsexportcommit work with filenames with spaces and non-ascii characters. 3 messages from 2006-11-15 to 2006-11-15. Participants: Robin Rosenberg, Junio C Hamano. Thread: https://gitlist.dev/t/43647 ## Robin Rosenberg, 2006-11-15 00:55 Subject: [PATCH] Make cvsexportcommit work with filenames with spaces and non-ascii characters. Message-ID: <20061115005530.26560.18222.stgit@lathund.dewire.com> URL: https://gitlist.dev/e/20061115005530.26560.18222.stgit%40lathund.dewire.com ``` From: Robin Rosenberg 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 --- git-cvsexportcommit.perl | 141 ++++++++++++++++------------------------ t/t9200-git-cvsexportcommit.sh | 79 +++++++++++++++++++--- 2 files changed, 124 insertions(+), 96 deletions(-) diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl index 7bac16e..feff681 100755 --- a/git-cvsexportcommit.perl +++ b/git-cvsexportcommit.perl @@ -2,9 +2,8 @@ #!/usr/bin/perl -w # Known limitations: # - does not propagate permissions -# - tells "ready for commit" even when things could not be completed -# (not sure this is true anymore, more testing is needed) -# - does not handle whitespace in pathnames at all. +# - error handling has not been extensively tested +# use strict; use Getopt::Std; @@ -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"; -foreach my $f (@files) { - chomp $f; - my @fields = split(m!\s+!, $f); +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; if ($fields[4] eq 'A') { my $path = $fields[5]; push @afiles, $path; @@ -139,12 +144,20 @@ foreach my $f (@files) { push @dfiles, $fields[5]; } } + 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; + push @bfiles, @abfiles; push @bfiles, @dbfiles; push @bfiles, @mbfiles; @@ -152,7 +165,6 @@ push @mfiles, @mbfiles; $opt_v && print "The commit affects:\n "; $opt_v && print join ("\n ", @afiles,@mfiles,@dfiles) . "\n\n"; -undef @files; # don't need it anymore # check that the files are clean and up to date according to cvs my $dirty; @@ -195,76 +207,36 @@ if ($dirty) { } } -### -### NOTE: if you are planning to die() past this point -### you MUST call cleanupcvs(@files) before die() -### - - +my $dirtypatch = 0; print "Creating new directories\n"; foreach my $d (@dirs) { unless (mkdir $d) { warn "Could not mkdir $d: $!"; - $dirty = 1; + $dirtypatch = 1; } - `cvs add $d`; - if ($?) { - $dirty = 1; + print "Adding directory $d"; + if (system('cvs','add',$d)) { + $dirtypatch = 1; warn "Failed to cvs add directory $d -- you may need to do it manually"; } } -print "'Patching' binary files\n"; - -foreach my $f (@bfiles) { - # check that the file in cvs matches the "old" file - # extract the file to $tmpdir and compare with cmp - if (not(grep { $_ eq $f } @afiles)) { - my $tree = safe_pipe_capture('git-rev-parse', "$parent^{tree}"); - chomp $tree; - my $blob = `git-ls-tree $tree "$f" | cut -f 1 | cut -d ' ' -f 3`; - chomp $blob; - `git-cat-file blob $blob > $tmpdir/blob`; - if (system('cmp', '-s', $f, "$tmpdir/blob")) { - warn "Binary file $f in CVS does not match parent.\n"; - if (not $opt_f) { - $dirty = 1; - next; - } - } - } - if (not(grep { $_ eq $f } @dfiles)) { - my $tree = safe_pipe_capture('git-rev-parse', "$commit^{tree}"); - chomp $tree; - my $blob = `git-ls-tree $tree "$f" | cut -f 1 | cut -d ' ' -f 3`; - chomp $blob; - # replace with the new file - `git-cat-file blob $blob > $f`; - } - - # TODO: something smart with file modes - -} -if ($dirty) { - cleanupcvs(@files); - die "Exiting: Binary files in CVS do not match parent"; -} - ## apply non-binary changes my $fuzz = $opt_p ? 0 : 2; -print "Patching non-binary files\n"; +print "Patching\n"; if (scalar(@afiles)+scalar(@dfiles)+scalar(@mfiles) != scalar(@bfiles)) { - print `(git-diff-tree -p $parent -p $commit | patch -p1 -F $fuzz ) 2>&1`; -} - -my $dirtypatch = 0; -if (($? >> 8) == 2) { - cleanupcvs(@files); - die "Exiting: Patch reported serious trouble -- you will have to apply this patch manually"; -} elsif (($? >> 8) == 1) { # some hunks failed to apply - $dirtypatch = 1; + `git-diff-tree --binary -z -p $parent -p $commit >.cvsexportcommit.diff`; + if ($?) { + die("cannot diff"); + } + `GIT_DIR= git-apply -z -C$fuzz .cvsexportcommit.diff`; + if ($?) { + $dirtypatch = 1; + } else { + $opt_v || unlink(".cvsexportcommit.diff"); + } } foreach my $f (@afiles) { @@ -274,7 +246,7 @@ foreach my $f (@afiles) { system('cvs', 'add', $f); } if ($?) { - $dirty = 1; + $dirtypatch = 1; warn "Failed to cvs add $f -- you may need to do it manually"; } } @@ -282,19 +254,21 @@ foreach my $f (@afiles) { foreach my $f (@dfiles) { system('cvs', 'rm', '-f', $f); if ($?) { - $dirty = 1; + $dirtypatch = 1; warn "Failed to cvs rm -f $f -- you may need to do it manually"; } } print "Commit to CVS\n"; -print "Patch: $title\n"; -my $commitfiles = join(' ', @afiles, @mfiles, @dfiles); -my $cmd = "cvs commit -F .msg $commitfiles"; +print "Patch title: $title\n"; +my @commitfiles = map { unless (m/\s/) { '\''.$_.'\''; } else { $_; }; } (@afiles, @mfiles, @dfiles); +my $cmd = "cvs commit -F .msg @commitfiles"; if ($dirtypatch) { print "NOTE: One or more hunks failed to apply cleanly.\n"; - print "Resolve the conflicts and then commit using:\n"; + print "You'll need to apply the patch in .cvsexportcommit.diff manually\n"; + print "using a patch program. After applying the patch and resolving the\n"; + print "problems you may commit using:"; print "\n $cmd\n\n"; exit(1); } @@ -304,7 +278,6 @@ if ($opt_c) { print "Autocommit\n $cmd\n"; print safe_pipe_capture('cvs', 'commit', '-F', '.msg', @afiles, @mfiles, @dfiles); if ($?) { - cleanupcvs(@files); die "Exiting: The commit did not succeed"; } print "Committed successfully to CVS\n"; @@ -318,17 +291,6 @@ END exit(1); } -# ensure cvs is clean before we die -sub cleanupcvs { - my @files = @_; - foreach my $f (@files) { - system('cvs', '-q', 'update', '-C', $f); - if ($?) { - warn "Warning! Failed to cleanup state of $f\n"; - } - } -} - # An alternative to `command` that allows input to be passed as an array # to work around shell problems with weird characters in arguments # if the exec returns non-zero we die @@ -342,3 +304,16 @@ sub safe_pipe_capture { } return wantarray ? @output : join('',@output); } + +sub safe_pipe_capture_blob { + my $output; + if (my $pid = open my $child, '-|') { + local $/; + undef $/; + $output = (<$child>); + close $child or die join(' ',@_).": $! $?"; + } else { + exec(@_) or die "$! $?"; # exec() can fail the executable can't be found + } + return $output; +} diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh index 6e566d4..75b9f38 100755 --- a/t/t9200-git-cvsexportcommit.sh +++ b/t/t9200-git-cvsexportcommit.sh @@ -89,18 +89,17 @@ test_expect_success \ ! git cvsexportcommit -c $id )' -# Should fail, but only on the git-cvsexportcommit stage -test_expect_success \ - 'Fail to remove binary file more than one generation old' \ - 'git reset --hard HEAD^ && - cat F/newfile6.png >>D/newfile4.png && - git commit -a -m "generation 2 (again)" && - rm -f D/newfile4.png && - git commit -a -m "generation 3" && - id=$(git rev-list --max-count=1 HEAD) && - (cd "$CVSWORK" && - ! git cvsexportcommit -c $id - )' +#test_expect_success \ +# 'Fail to remove binary file more than one generation old' \ +# 'git reset --hard HEAD^ && +# cat F/newfile6.png >>D/newfile4.png && +# git commit -a -m "generation 2 (again)" && +# rm -f D/newfile4.png && +# git commit -a -m "generation 3" && +# id=$(git rev-list --max-count=1 HEAD) && +# (cd "$CVSWORK" && +# ! git cvsexportcommit -c $id +# )' # We reuse the state from two tests back here @@ -108,7 +107,7 @@ # This test is here because a patch for # fail with gnu patch, so cvsexportcommit must handle that. test_expect_success \ 'Remove only binary files' \ - 'git reset --hard HEAD^^^ && + 'git reset --hard HEAD^^ && rm -f D/newfile4.png && git commit -a -m "test: remove only a binary file" && id=$(git rev-list --max-count=1 HEAD) && @@ -142,4 +141,58 @@ test_expect_success \ diff F/newfile6.png ../F/newfile6.png )' +test_expect_success \ + 'New file with spaces in file name' \ + 'mkdir "G g" && + echo ok then >"G g/with spaces.txt" && + git add "G g/with spaces.txt" && \ + cp ../test9200a.png "G g/with spaces.png" && \ + git add "G g/with spaces.png" && + git commit -a -m "With spaces" && + id=$(git rev-list --max-count=1 HEAD) && + (cd "$CVSWORK" && + git-cvsexportcommit -c $id && + test "$(echo $(sort "G g/CVS/Entries"|cut -d/ -f2,3,5))" = "with spaces.png/1.1/-kb with spaces.txt/1.1/" + )' + +test_expect_success \ + 'Update file with spaces in file name' \ + 'echo Ok then >>"G g/with spaces.txt" && + cat ../test9200a.png >>"G g/with spaces.png" && \ + git add "G g/with spaces.png" && + git commit -a -m "Update with spaces" && + id=$(git rev-list --max-count=1 HEAD) && + (cd "$CVSWORK" && + git-cvsexportcommit -c $id + test "$(echo $(sort "G g/CVS/Entries"|cut -d/ -f2,3,5))" = "with spaces.png/1.2/-kb with spaces.txt/1.2/" + )' + +# This test contains ISO-8859-1 characters +test_expect_success \ + 'File with non-ascii file name' \ + 'mkdir Å && + echo Foo >Å/gårdetsågårdet.txt && + git add Å/gårdetsågårdet.txt && + cp ../test9200a.png Å/gårdetsågårdet.png && + git add Å/gårdetsågårdet.png && + git commit -a -m "Går det så går det" && \ + id=$(git rev-list --max-count=1 HEAD) && + (cd "$CVSWORK" && + git-cvsexportcommit -v -c $id && + test "$(echo $(sort Å/CVS/Entries|cut -d/ -f2,3,5))" = "gårdetsågårdet.png/1.1/-kb gårdetsågårdet.txt/1.1/" + )' + +test_expect_success \ + 'Mismatching patch should fail' \ + 'date >>"E/newfile5.txt" && + git add "E/newfile5.txt" && + git commit -a -m "Update one" && + date >>"E/newfile5.txt" && + git add "E/newfile5.txt" && + git commit -a -m "Update two" && + id=$(git rev-list --max-count=1 HEAD) && + (cd "$CVSWORK" && + ! git-cvsexportcommit -c $id + )' + ``` ## Junio C Hamano, 2006-11-15 06:08 Subject: Re: [PATCH] Make cvsexportcommit work with filenames with spaces and non-ascii characters. Message-ID: <7v8xidqm1e.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v8xidqm1e.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20061115005530.26560.18222.stgit@lathund.dewire.com> ``` Robin Rosenberg writes: > From: Robin Rosenberg > > 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 Thanks. Allow me to put this in freezer for a few days while releasing v1.4.4. Some comments below. > @@ -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/^.../); > 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). ``` ## Junio C Hamano, 2006-11-15 06:27 Subject: Re: [PATCH] Make cvsexportcommit work with filenames with spaces and non-ascii characters. Message-ID: <7vu011p6lc.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vu011p6lc.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <7v8xidqm1e.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano writes: > 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? That is, something like this... -- >8 -- [PATCH] apply --numstat: mark binary diffstat with - -, not 0 0 We do not even know number of lines so showing it as 0 0 is lying. This would also help Porcelains like cvsexportcommit. Signed-off-by: Junio C Hamano --- builtin-apply.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/builtin-apply.c b/builtin-apply.c index aad5526..b80ad2c 100644 --- a/builtin-apply.c +++ b/builtin-apply.c @@ -2116,7 +2116,11 @@ static void numstat_patch_list(struct pa for ( ; patch; patch = patch->next) { const char *name; name = patch->new_name ? patch->new_name : patch->old_name; - printf("%d\t%d\t", patch->lines_added, patch->lines_deleted); + if (patch->is_binary) + printf("-\t-\t"); + else + printf("%d\t%d\t", + patch->lines_added, patch->lines_deleted); if (line_termination && quote_c_style(name, NULL, NULL, 0)) quote_c_style(name, NULL, stdout, 0); else -- 1.4.4.rc2.g2a54 ```