* [PATCH 1/2] git-svn.perl: perform deletions before anything else
@ 2012-02-09 18:55 Steven Walter
2012-02-09 18:55 ` [PATCH 2/2] git-svn.perl: fix a false-positive in the "already exists" test Steven Walter
` (2 more replies)
0 siblings, 3 replies; 17+ messages in thread
From: Steven Walter @ 2012-02-09 18:55 UTC (permalink / raw)
To: normalperson, git; +Cc: Steven Walter
If we delete a file and recreate it as a directory in a single commit,
we have to tell the server about the deletion first or else we'll get
"RA layer request failed: Server sent unexpected return value (405
Method Not Allowed) in response to MKCOL request"
---
git-svn.perl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index 570d83d..520b02b 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -5391,7 +5391,7 @@ sub DESTROY {
sub apply_diff {
my ($self) = @_;
my $mods = $self->{mods};
- my %o = ( D => 1, R => 0, C => -1, A => 3, M => 3, T => 3 );
+ my %o = ( D => -2, R => 0, C => -1, A => 3, M => 3, T => 3 );
foreach my $m (sort { $o{$a->{chg}} <=> $o{$b->{chg}} } @$mods) {
my $f = $m->{chg};
if (defined $o{$f}) {
--
1.7.9.4.ge7a0d
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 2/2] git-svn.perl: fix a false-positive in the "already exists" test 2012-02-09 18:55 [PATCH 1/2] git-svn.perl: perform deletions before anything else Steven Walter @ 2012-02-09 18:55 ` Steven Walter 2012-02-09 20:08 ` [PATCH 1/2] git-svn.perl: perform deletions before anything else Junio C Hamano [not found] ` <87mx8rrf5i.fsf@thomas.inf.ethz.ch> 2 siblings, 0 replies; 17+ messages in thread From: Steven Walter @ 2012-02-09 18:55 UTC (permalink / raw) To: normalperson, git; +Cc: Steven Walter open_or_add_dir checks to see if the directory already exists or not. If it already exists and is not a directory, then we fail. However, open_or_add_dir did not previously account for the possibility that the path did exist as a file, but is deleted in the current commit. In order to prevent this legitimate case from failing, open_or_add_dir needs to know what files are deleted in the current commit. Unfortunately that information has to be plumbed through a couple of layers. --- git-svn.perl | 43 ++++++++++++++++++++++++++----------------- 1 files changed, 26 insertions(+), 17 deletions(-) diff --git a/git-svn.perl b/git-svn.perl index 520b02b..351e9e3 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -5147,7 +5147,7 @@ sub rmdirs { } sub open_or_add_dir { - my ($self, $full_path, $baton) = @_; + my ($self, $full_path, $baton, $deletions) = @_; my $t = $self->{types}->{$full_path}; if (!defined $t) { die "$full_path not known in r$self->{r} or we have a bug!\n"; @@ -5156,7 +5156,7 @@ sub open_or_add_dir { no warnings 'once'; # SVN::Node::none and SVN::Node::file are used only once, # so we're shutting up Perl's warnings about them. - if ($t == $SVN::Node::none) { + if ($t == $SVN::Node::none || defined($deletions->{$full_path})) { return $self->add_directory($full_path, $baton, undef, -1, $self->{pool}); } elsif ($t == $SVN::Node::dir) { @@ -5171,17 +5171,18 @@ sub open_or_add_dir { } sub ensure_path { - my ($self, $path) = @_; + my ($self, $path, $deletions) = @_; my $bat = $self->{bat}; my $repo_path = $self->repo_path($path); return $bat->{''} unless (length $repo_path); + my @p = split m#/+#, $repo_path; my $c = shift @p; - $bat->{$c} ||= $self->open_or_add_dir($c, $bat->{''}); + $bat->{$c} ||= $self->open_or_add_dir($c, $bat->{''}, $deletions); while (@p) { my $c0 = $c; $c .= '/' . shift @p; - $bat->{$c} ||= $self->open_or_add_dir($c, $bat->{$c0}); + $bat->{$c} ||= $self->open_or_add_dir($c, $bat->{$c0}, $deletions); } return $bat->{$c}; } @@ -5238,9 +5239,9 @@ sub apply_autoprops { } sub A { - my ($self, $m) = @_; + my ($self, $m, $deletions) = @_; my ($dir, $file) = split_path($m->{file_b}); - my $pbat = $self->ensure_path($dir); + my $pbat = $self->ensure_path($dir, $deletions); my $fbat = $self->add_file($self->repo_path($m->{file_b}), $pbat, undef, -1); print "\tA\t$m->{file_b}\n" unless $::_q; @@ -5250,9 +5251,9 @@ sub A { } sub C { - my ($self, $m) = @_; + my ($self, $m, $deletions) = @_; my ($dir, $file) = split_path($m->{file_b}); - my $pbat = $self->ensure_path($dir); + my $pbat = $self->ensure_path($dir, $deletions); my $fbat = $self->add_file($self->repo_path($m->{file_b}), $pbat, $self->url_path($m->{file_a}), $self->{r}); print "\tC\t$m->{file_a} => $m->{file_b}\n" unless $::_q; @@ -5269,9 +5270,9 @@ sub delete_entry { } sub R { - my ($self, $m) = @_; + my ($self, $m, $deletions) = @_; my ($dir, $file) = split_path($m->{file_b}); - my $pbat = $self->ensure_path($dir); + my $pbat = $self->ensure_path($dir, $deletions); my $fbat = $self->add_file($self->repo_path($m->{file_b}), $pbat, $self->url_path($m->{file_a}), $self->{r}); print "\tR\t$m->{file_a} => $m->{file_b}\n" unless $::_q; @@ -5280,14 +5281,14 @@ sub R { $self->close_file($fbat,undef,$self->{pool}); ($dir, $file) = split_path($m->{file_a}); - $pbat = $self->ensure_path($dir); + $pbat = $self->ensure_path($dir, $deletions); $self->delete_entry($m->{file_a}, $pbat); } sub M { - my ($self, $m) = @_; + my ($self, $m, $deletions) = @_; my ($dir, $file) = split_path($m->{file_b}); - my $pbat = $self->ensure_path($dir); + my $pbat = $self->ensure_path($dir, $deletions); my $fbat = $self->open_file($self->repo_path($m->{file_b}), $pbat,$self->{r},$self->{pool}); print "\t$m->{chg}\t$m->{file_b}\n" unless $::_q; @@ -5357,9 +5358,9 @@ sub chg_file { } sub D { - my ($self, $m) = @_; + my ($self, $m, $deletions) = @_; my ($dir, $file) = split_path($m->{file_b}); - my $pbat = $self->ensure_path($dir); + my $pbat = $self->ensure_path($dir, $deletions); print "\tD\t$m->{file_b}\n" unless $::_q; $self->delete_entry($m->{file_b}, $pbat); } @@ -5392,10 +5393,18 @@ sub apply_diff { my ($self) = @_; my $mods = $self->{mods}; my %o = ( D => -2, R => 0, C => -1, A => 3, M => 3, T => 3 ); + my %deletions; + + foreach my $m (@$mods) { + if ($m->{chg} eq "D") { + $deletions{$m->{file_b}} = 1; + } + } + foreach my $m (sort { $o{$a->{chg}} <=> $o{$b->{chg}} } @$mods) { my $f = $m->{chg}; if (defined $o{$f}) { - $self->$f($m); + $self->$f($m, \%deletions); } else { fatal("Invalid change type: $f"); } -- 1.7.9.4.ge7a0d ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] git-svn.perl: perform deletions before anything else 2012-02-09 18:55 [PATCH 1/2] git-svn.perl: perform deletions before anything else Steven Walter 2012-02-09 18:55 ` [PATCH 2/2] git-svn.perl: fix a false-positive in the "already exists" test Steven Walter @ 2012-02-09 20:08 ` Junio C Hamano 2012-02-09 20:52 ` Steven Walter [not found] ` <87mx8rrf5i.fsf@thomas.inf.ethz.ch> 2 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2012-02-09 20:08 UTC (permalink / raw) To: Steven Walter; +Cc: normalperson, git Steven Walter <stevenrwalter@gmail.com> writes: > - my %o = ( D => 1, R => 0, C => -1, A => 3, M => 3, T => 3 ); > + my %o = ( D => -2, R => 0, C => -1, A => 3, M => 3, T => 3 ); I know this code arrangement dates back to cf52b8f (git-svn: fix several corner-case and rare bugs with 'commit', 2006-02-20), but somehow I find it extremely hard to follow. The absolute values do not matter (this is only used to sort the classes of operations), and the fact that A/M/T shares the same value does not help a stable sort result (as it is used as a key to sort {} that is not given any key other than $o{$ab->{chg}} to tie-break). I suspect that writing it this way my %o = (D => 0, C => 1, R => 2, A => 3, M => 4, T => 5) or even my $ord = 0; my %o = map { $_ => $ord++ } qw(D C R A M T); would make it much easier to follow. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] git-svn.perl: perform deletions before anything else 2012-02-09 20:08 ` [PATCH 1/2] git-svn.perl: perform deletions before anything else Junio C Hamano @ 2012-02-09 20:52 ` Steven Walter 2012-02-09 20:52 ` Steven Walter 0 siblings, 1 reply; 17+ messages in thread From: Steven Walter @ 2012-02-09 20:52 UTC (permalink / raw) To: gitster, normalperson, git > I suspect that writing it this way [...] would make it much easier to > follow. Agreed. New patch to follow, this time with sign-off. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] git-svn.perl: perform deletions before anything else 2012-02-09 20:52 ` Steven Walter @ 2012-02-09 20:52 ` Steven Walter 2012-02-12 7:03 ` Eric Wong 0 siblings, 1 reply; 17+ messages in thread From: Steven Walter @ 2012-02-09 20:52 UTC (permalink / raw) To: gitster, normalperson, git; +Cc: Steven Walter, Steven Walter From: Steven Walter <swalter@lexmark.com> If we delete a file and recreate it as a directory in a single commit, we have to tell the server about the deletion first or else we'll get "RA layer request failed: Server sent unexpected return value (405 Method Not Allowed) in response to MKCOL request" Signed-off-by: Steven Walter <stevenrwalter@gmail.com> --- git-svn.perl | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/git-svn.perl b/git-svn.perl index eeb83d3..06c9322 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -5374,7 +5374,7 @@ sub DESTROY { sub apply_diff { my ($self) = @_; my $mods = $self->{mods}; - my %o = ( D => 1, R => 0, C => -1, A => 3, M => 3, T => 3 ); + my %o = ( D => 0, C => 1, R => 2, A => 3, M => 4, T => 5 ); foreach my $m (sort { $o{$a->{chg}} <=> $o{$b->{chg}} } @$mods) { my $f = $m->{chg}; if (defined $o{$f}) { -- 1.7.5.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] git-svn.perl: perform deletions before anything else 2012-02-09 20:52 ` Steven Walter @ 2012-02-12 7:03 ` Eric Wong 2012-02-12 15:35 ` Steven Walter 0 siblings, 1 reply; 17+ messages in thread From: Eric Wong @ 2012-02-12 7:03 UTC (permalink / raw) To: Steven Walter; +Cc: gitster, git, Steven Walter Steven Walter <stevenrwalter@gmail.com> wrote: > Signed-off-by: Steven Walter <stevenrwalter@gmail.com> Thanks, shall I fixup 2/2 and assume you meant to Sign-off on that, too? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] git-svn.perl: perform deletions before anything else 2012-02-12 7:03 ` Eric Wong @ 2012-02-12 15:35 ` Steven Walter 2012-02-12 23:49 ` Eric Wong 0 siblings, 1 reply; 17+ messages in thread From: Steven Walter @ 2012-02-12 15:35 UTC (permalink / raw) To: Eric Wong; +Cc: gitster, git On Sun, Feb 12, 2012 at 2:03 AM, Eric Wong <normalperson@yhbt.net> wrote: > Steven Walter <stevenrwalter@gmail.com> wrote: >> Signed-off-by: Steven Walter <stevenrwalter@gmail.com> > > Thanks, shall I fixup 2/2 and assume you meant to Sign-off on that, too? Yes, thanks -- -Steven Walter <stevenrwalter@gmail.com> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] git-svn.perl: perform deletions before anything else 2012-02-12 15:35 ` Steven Walter @ 2012-02-12 23:49 ` Eric Wong 2012-02-15 17:47 ` Steven Walter 0 siblings, 1 reply; 17+ messages in thread From: Eric Wong @ 2012-02-12 23:49 UTC (permalink / raw) To: Steven Walter; +Cc: gitster, git Steven Walter <stevenrwalter@gmail.com> wrote: > On Sun, Feb 12, 2012 at 2:03 AM, Eric Wong <normalperson@yhbt.net> wrote: > > Steven Walter <stevenrwalter@gmail.com> wrote: > >> Signed-off-by: Steven Walter <stevenrwalter@gmail.com> > > > > Thanks, shall I fixup 2/2 and assume you meant to Sign-off on that, too? > > Yes, thanks Ugh, I got a bunch of test failures on t9100-git-svn-basic.sh with your updated 1/2 and a trivially merged 2/2: not ok - 7 detect node change from file to directory #2 not ok - 12 new symlink is added to a file that was also just made executable not ok - 13 modify a symlink to become a file not ok - 14 commit with UTF-8 message: locale: en_US.UTF-8 not ok - 16 check imported tree checksums expected tree checksums 1/2 alone seems to pass all existing tests. I would very much appreciate new test cases that can show exactly what's fixed by your patches (esp given the only times I run/use git-svn is when reviewing patches). Thanks!. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] git-svn.perl: perform deletions before anything else 2012-02-12 23:49 ` Eric Wong @ 2012-02-15 17:47 ` Steven Walter 2012-02-19 10:54 ` Eric Wong 0 siblings, 1 reply; 17+ messages in thread From: Steven Walter @ 2012-02-15 17:47 UTC (permalink / raw) To: Eric Wong; +Cc: gitster, git On Sun, Feb 12, 2012 at 6:49 PM, Eric Wong <normalperson@yhbt.net> wrote: > Steven Walter <stevenrwalter@gmail.com> wrote: >> On Sun, Feb 12, 2012 at 2:03 AM, Eric Wong <normalperson@yhbt.net> wrote: >> > Steven Walter <stevenrwalter@gmail.com> wrote: >> >> Signed-off-by: Steven Walter <stevenrwalter@gmail.com> >> > >> > Thanks, shall I fixup 2/2 and assume you meant to Sign-off on that, too? >> >> Yes, thanks > > Ugh, I got a bunch of test failures on t9100-git-svn-basic.sh with your > updated 1/2 and a trivially merged 2/2: > > not ok - 7 detect node change from file to directory #2 I believe that "test_must_fail" is incorrect for this case. "git svn set-tree" is succeeding, and the git commit is being faithfully recorded into the svn repository. If svn will allow us to do it, then I don't think git-svn should artificially fail in the case. This is using svn 1.6.17 What's the oldest version of svn supported by git-svn? Perhaps if I retry with that version of svn, I would see a failure. However, if libsvn-perl reports the failure correctly, isn't that good enough behavior? No need to fail in git-svn before even trying, IMHO. > not ok - 12 new symlink is added to a file that was also just made executable > not ok - 13 modify a symlink to become a file > not ok - 14 commit with UTF-8 message: locale: en_US.UTF-8 > not ok - 16 check imported tree checksums expected tree checksums The rest of these problems seem to have been cascading failures resulting from the unexpected success of "git svn set-tree" in test 7. This left the git and svn repositories in a different state. To get these to pass, I changed later references to "bar/zzz" (which is now a directory) to use "file" instead. I also had to update the expected checksum values for test 16. Is there a way to validate what the checksums should be, other than to look at it and say, "yup, the trees look okay?" > I would very much appreciate new test cases that can show exactly what's > fixed by your patches (esp given the only times I run/use git-svn is > when reviewing patches). Thanks!. In fact test 7 is exactly what I was trying to make work. The fact that "git svn set-tree" now succeeds in that case is proof that my change had the desired effect. I modified test 7 to verify that set-tree succeeds and that bar/zzz and bar/zzz/yyy get created in $SVN_TREE. Assuming you agree with the above analysis, should I squash the test changes into my 2/2, or would you prefer a separate patch? -- -Steven Walter <stevenrwalter@gmail.com> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] git-svn.perl: perform deletions before anything else 2012-02-15 17:47 ` Steven Walter @ 2012-02-19 10:54 ` Eric Wong 2012-02-20 14:17 ` [PATCH] git-svn.perl: fix a false-positive in the "already exists" test Steven Walter 0 siblings, 1 reply; 17+ messages in thread From: Eric Wong @ 2012-02-19 10:54 UTC (permalink / raw) To: Steven Walter; +Cc: gitster, git Steven Walter <stevenrwalter@gmail.com> wrote: > I don't think git-svn should artificially fail in the case. This is > using svn 1.6.17 > What's the oldest version of svn supported by git-svn? Perhaps if I > retry with that version of svn, I would see a failure. However, if > libsvn-perl reports the failure correctly, isn't that good enough > behavior? No need to fail in git-svn before even trying, IMHO. Originally (back in 2006/2007), the goal was to support SVN 1.1.x+. I'm not sure if I we ever lost support for such old versions. I use Debian stable for testing patches, and SVN is 1.6.12 there. Otherwise, whatever people are willing to support and send patches/bugreports for is good. > Is there a way to validate what the checksums should be, other than to > look at it and say, "yup, the trees look okay?" As far as I remember, that's how I originally wrote the tests. > Assuming you agree with the above analysis, should I squash the test > changes into my 2/2, or would you prefer a separate patch? Your analysis seems correct. I always prefer test changes to be combined with corresponding commits to avoid breakage during bisect. Thanks! ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] git-svn.perl: fix a false-positive in the "already exists" test 2012-02-19 10:54 ` Eric Wong @ 2012-02-20 14:17 ` Steven Walter 2012-02-22 0:33 ` Eric Wong 2012-02-22 2:16 ` Junio C Hamano 0 siblings, 2 replies; 17+ messages in thread From: Steven Walter @ 2012-02-20 14:17 UTC (permalink / raw) To: normalperson, gitster, git; +Cc: Steven Walter open_or_add_dir checks to see if the directory already exists or not. If it already exists and is not a directory, then we fail. However, open_or_add_dir did not previously account for the possibility that the path did exist as a file, but is deleted in the current commit. In order to prevent this legitimate case from failing, open_or_add_dir needs to know what files are deleted in the current commit. Unfortunately that information has to be plumbed through a couple of layers. Signed-off-by: Steven Walter <stevenrwalter@gmail.com> --- git-svn.perl | 43 ++++++++++++++++++++++++++----------------- t/t9100-git-svn-basic.sh | 33 ++++++++++++++++++--------------- 2 files changed, 44 insertions(+), 32 deletions(-) diff --git a/git-svn.perl b/git-svn.perl index 06c9322..c7a961d 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -5130,7 +5130,7 @@ sub rmdirs { } sub open_or_add_dir { - my ($self, $full_path, $baton) = @_; + my ($self, $full_path, $baton, $deletions) = @_; my $t = $self->{types}->{$full_path}; if (!defined $t) { die "$full_path not known in r$self->{r} or we have a bug!\n"; @@ -5139,7 +5139,7 @@ sub open_or_add_dir { no warnings 'once'; # SVN::Node::none and SVN::Node::file are used only once, # so we're shutting up Perl's warnings about them. - if ($t == $SVN::Node::none) { + if ($t == $SVN::Node::none || defined($deletions->{$full_path})) { return $self->add_directory($full_path, $baton, undef, -1, $self->{pool}); } elsif ($t == $SVN::Node::dir) { @@ -5154,17 +5154,18 @@ sub open_or_add_dir { } sub ensure_path { - my ($self, $path) = @_; + my ($self, $path, $deletions) = @_; my $bat = $self->{bat}; my $repo_path = $self->repo_path($path); return $bat->{''} unless (length $repo_path); + my @p = split m#/+#, $repo_path; my $c = shift @p; - $bat->{$c} ||= $self->open_or_add_dir($c, $bat->{''}); + $bat->{$c} ||= $self->open_or_add_dir($c, $bat->{''}, $deletions); while (@p) { my $c0 = $c; $c .= '/' . shift @p; - $bat->{$c} ||= $self->open_or_add_dir($c, $bat->{$c0}); + $bat->{$c} ||= $self->open_or_add_dir($c, $bat->{$c0}, $deletions); } return $bat->{$c}; } @@ -5221,9 +5222,9 @@ sub apply_autoprops { } sub A { - my ($self, $m) = @_; + my ($self, $m, $deletions) = @_; my ($dir, $file) = split_path($m->{file_b}); - my $pbat = $self->ensure_path($dir); + my $pbat = $self->ensure_path($dir, $deletions); my $fbat = $self->add_file($self->repo_path($m->{file_b}), $pbat, undef, -1); print "\tA\t$m->{file_b}\n" unless $::_q; @@ -5233,9 +5234,9 @@ sub A { } sub C { - my ($self, $m) = @_; + my ($self, $m, $deletions) = @_; my ($dir, $file) = split_path($m->{file_b}); - my $pbat = $self->ensure_path($dir); + my $pbat = $self->ensure_path($dir, $deletions); my $fbat = $self->add_file($self->repo_path($m->{file_b}), $pbat, $self->url_path($m->{file_a}), $self->{r}); print "\tC\t$m->{file_a} => $m->{file_b}\n" unless $::_q; @@ -5252,9 +5253,9 @@ sub delete_entry { } sub R { - my ($self, $m) = @_; + my ($self, $m, $deletions) = @_; my ($dir, $file) = split_path($m->{file_b}); - my $pbat = $self->ensure_path($dir); + my $pbat = $self->ensure_path($dir, $deletions); my $fbat = $self->add_file($self->repo_path($m->{file_b}), $pbat, $self->url_path($m->{file_a}), $self->{r}); print "\tR\t$m->{file_a} => $m->{file_b}\n" unless $::_q; @@ -5263,14 +5264,14 @@ sub R { $self->close_file($fbat,undef,$self->{pool}); ($dir, $file) = split_path($m->{file_a}); - $pbat = $self->ensure_path($dir); + $pbat = $self->ensure_path($dir, $deletions); $self->delete_entry($m->{file_a}, $pbat); } sub M { - my ($self, $m) = @_; + my ($self, $m, $deletions) = @_; my ($dir, $file) = split_path($m->{file_b}); - my $pbat = $self->ensure_path($dir); + my $pbat = $self->ensure_path($dir, $deletions); my $fbat = $self->open_file($self->repo_path($m->{file_b}), $pbat,$self->{r},$self->{pool}); print "\t$m->{chg}\t$m->{file_b}\n" unless $::_q; @@ -5340,9 +5341,9 @@ sub chg_file { } sub D { - my ($self, $m) = @_; + my ($self, $m, $deletions) = @_; my ($dir, $file) = split_path($m->{file_b}); - my $pbat = $self->ensure_path($dir); + my $pbat = $self->ensure_path($dir, $deletions); print "\tD\t$m->{file_b}\n" unless $::_q; $self->delete_entry($m->{file_b}, $pbat); } @@ -5375,10 +5376,18 @@ sub apply_diff { my ($self) = @_; my $mods = $self->{mods}; my %o = ( D => 0, C => 1, R => 2, A => 3, M => 4, T => 5 ); + my %deletions; + + foreach my $m (@$mods) { + if ($m->{chg} eq "D") { + $deletions{$m->{file_b}} = 1; + } + } + foreach my $m (sort { $o{$a->{chg}} <=> $o{$b->{chg}} } @$mods) { my $f = $m->{chg}; if (defined $o{$f}) { - $self->$f($m); + $self->$f($m, \%deletions); } else { fatal("Invalid change type: $f"); } diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh index b041516..4029f84 100755 --- a/t/t9100-git-svn-basic.sh +++ b/t/t9100-git-svn-basic.sh @@ -92,9 +92,11 @@ test_expect_success "$name" ' echo yyy > bar/zzz/yyy && git update-index --add bar/zzz/yyy && git commit -m "$name" && - test_must_fail git svn set-tree --find-copies-harder --rmdir \ - ${remotes_git_svn}..mybranch3' || true - + git svn set-tree --find-copies-harder --rmdir \ + ${remotes_git_svn}..mybranch3 && + svn_cmd up "$SVN_TREE" && + test -d "$SVN_TREE"/bar/zzz && + test -e "$SVN_TREE"/bar/zzz/yyy ' || true name='detect node change from directory to file #2' test_expect_success "$name" ' @@ -134,10 +136,10 @@ test_expect_success "$name" ' test -x "$SVN_TREE"/exec.sh' -name='executable file becomes a symlink to bar/zzz (file)' +name='executable file becomes a symlink to file' test_expect_success "$name" ' rm exec.sh && - ln -s bar/zzz exec.sh && + ln -s file exec.sh && git update-index exec.sh && git commit -m "$name" && git svn set-tree --find-copies-harder --rmdir \ @@ -148,14 +150,14 @@ test_expect_success "$name" ' name='new symlink is added to a file that was also just made executable' test_expect_success "$name" ' - chmod +x bar/zzz && - ln -s bar/zzz exec-2.sh && - git update-index --add bar/zzz exec-2.sh && + chmod +x file && + ln -s file exec-2.sh && + git update-index --add file exec-2.sh && git commit -m "$name" && git svn set-tree --find-copies-harder --rmdir \ ${remotes_git_svn}..mybranch5 && svn_cmd up "$SVN_TREE" && - test -x "$SVN_TREE"/bar/zzz && + test -x "$SVN_TREE"/file && test -h "$SVN_TREE"/exec-2.sh' name='modify a symlink to become a file' @@ -195,14 +197,15 @@ name='check imported tree checksums expected tree checksums' rm -f expected if test_have_prereq UTF8 then - echo tree bf522353586b1b883488f2bc73dab0d9f774b9a9 > expected + echo tree dc68b14b733e4ec85b04ab6f712340edc5dc936e > expected fi cat >> expected <<\EOF -tree 83654bb36f019ae4fe77a0171f81075972087624 -tree 031b8d557afc6fea52894eaebb45bec52f1ba6d1 -tree 0b094cbff17168f24c302e297f55bfac65eb8bd3 -tree d667270a1f7b109f5eb3aaea21ede14b56bfdd6e -tree 56a30b966619b863674f5978696f4a3594f2fca9 +tree c3322890dcf74901f32d216f05c5044f670ce632 +tree d3ccd5035feafd17b030c5732e7808cc49122853 +tree d03e1630363d4881e68929d532746b20b0986b83 +tree 149d63cd5878155c846e8c55d7d8487de283f89e +tree 312b76e4f64ce14893aeac8591eb3960b065e247 +tree 149d63cd5878155c846e8c55d7d8487de283f89e tree d667270a1f7b109f5eb3aaea21ede14b56bfdd6e tree 8f51f74cf0163afc9ad68a4b1537288c4558b5a4 EOF -- 1.7.3.4 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] git-svn.perl: fix a false-positive in the "already exists" test 2012-02-20 14:17 ` [PATCH] git-svn.perl: fix a false-positive in the "already exists" test Steven Walter @ 2012-02-22 0:33 ` Eric Wong 2012-02-22 2:16 ` Junio C Hamano 1 sibling, 0 replies; 17+ messages in thread From: Eric Wong @ 2012-02-22 0:33 UTC (permalink / raw) To: Steven Walter; +Cc: gitster, git Steven Walter <stevenrwalter@gmail.com> wrote: > open_or_add_dir checks to see if the directory already exists or not. > If it already exists and is not a directory, then we fail. However, > open_or_add_dir did not previously account for the possibility that the > path did exist as a file, but is deleted in the current commit. > > In order to prevent this legitimate case from failing, open_or_add_dir > needs to know what files are deleted in the current commit. > Unfortunately that information has to be plumbed through a couple of > layers. > > Signed-off-by: Steven Walter <stevenrwalter@gmail.com> Thanks, will push. Acked-by: Eric Wong <normalperson@yhbt.net> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] git-svn.perl: fix a false-positive in the "already exists" test 2012-02-20 14:17 ` [PATCH] git-svn.perl: fix a false-positive in the "already exists" test Steven Walter 2012-02-22 0:33 ` Eric Wong @ 2012-02-22 2:16 ` Junio C Hamano 2012-02-22 2:32 ` Steven Walter 1 sibling, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2012-02-22 2:16 UTC (permalink / raw) To: Steven Walter; +Cc: normalperson, gitster, git Steven Walter <stevenrwalter@gmail.com> writes: > diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh > index b041516..4029f84 100755 > --- a/t/t9100-git-svn-basic.sh > +++ b/t/t9100-git-svn-basic.sh > @@ -92,9 +92,11 @@ test_expect_success "$name" ' > echo yyy > bar/zzz/yyy && > git update-index --add bar/zzz/yyy && > git commit -m "$name" && > + git svn set-tree --find-copies-harder --rmdir \ > + ${remotes_git_svn}..mybranch3 && > + svn_cmd up "$SVN_TREE" && > + test -d "$SVN_TREE"/bar/zzz && > + test -e "$SVN_TREE"/bar/zzz/yyy ' || true Care to explain what this " || true" is doing here, please? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] git-svn.perl: fix a false-positive in the "already exists" test 2012-02-22 2:16 ` Junio C Hamano @ 2012-02-22 2:32 ` Steven Walter 2012-02-22 5:08 ` Junio C Hamano 0 siblings, 1 reply; 17+ messages in thread From: Steven Walter @ 2012-02-22 2:32 UTC (permalink / raw) To: Junio C Hamano; +Cc: normalperson, git On Tue, Feb 21, 2012 at 9:16 PM, Junio C Hamano <gitster@pobox.com> wrote: > Steven Walter <stevenrwalter@gmail.com> writes: > >> diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh >> index b041516..4029f84 100755 >> --- a/t/t9100-git-svn-basic.sh >> +++ b/t/t9100-git-svn-basic.sh >> @@ -92,9 +92,11 @@ test_expect_success "$name" ' >> echo yyy > bar/zzz/yyy && >> git update-index --add bar/zzz/yyy && >> git commit -m "$name" && >> + git svn set-tree --find-copies-harder --rmdir \ >> + ${remotes_git_svn}..mybranch3 && >> + svn_cmd up "$SVN_TREE" && >> + test -d "$SVN_TREE"/bar/zzz && >> + test -e "$SVN_TREE"/bar/zzz/yyy ' || true > > Care to explain what this " || true" is doing here, please? Ahh, good catch. I think the answer is that it shouldn't be there. It was originally there because of the "test_must_fail" line, I think (at least the other tests that use test_must_fail also have "|| true"). The tests all still pass with that "|| true" removed. Do you want to just fix that up, or a new version of the original patch, or a fix on top of the original patches? -- -Steven Walter <stevenrwalter@gmail.com> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] git-svn.perl: fix a false-positive in the "already exists" test 2012-02-22 2:32 ` Steven Walter @ 2012-02-22 5:08 ` Junio C Hamano 2012-02-23 23:17 ` Steven Walter 0 siblings, 1 reply; 17+ messages in thread From: Junio C Hamano @ 2012-02-22 5:08 UTC (permalink / raw) To: Steven Walter; +Cc: normalperson, git Steven Walter <stevenrwalter@gmail.com> writes: >>> + test -e "$SVN_TREE"/bar/zzz/yyy ' || true >> >> Care to explain what this " || true" is doing here, please? > > Ahh, good catch. I think the answer is that it shouldn't be there. > It was originally there because of the "test_must_fail" line, I think > (at least the other tests that use test_must_fail also have "|| > true"). Ok, that may explain the copy&paste error. But I do not think test_must_fail followed by || true makes much sense, either. The purpose of "test_must_fail" is to make sure the tested git command exits with non-zero status in a controlled way (i.e. not crash) so if the tested command that is expected to exit with non-zero status exited with zero status, the test has detected an *error*. E.g. if you know that the index and the working tree are different at one point in the test sequence, you would say: ... other setup steps ... && test_must_fail git diff --exit-code && ... and other tests ... so that failure by "git diff --exit-code" to exit with non-zero status (i.e. it did not find any difference when it should have) breaks the && cascade. I just took a quick look at t9100 but I think all " || true" can be safely removed. None of them is associated with test_must_fail in any way. For whatever reason, these test seem to do test_expect_success 'label of the test' ' body of the test ' || true for no good reason. > Do you want to just fix that up, or a new version of the original patch, > or a fix on top of the original patches? Eric queued the patch and then had me pull it as part of his history already, so it is doubly too late to replace it. Can you apply this patch and re-test? t/t9100-git-svn-basic.sh | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh index 4029f84..749b75e 100755 --- a/t/t9100-git-svn-basic.sh +++ b/t/t9100-git-svn-basic.sh @@ -65,7 +65,8 @@ test_expect_success "$name" " git update-index --add dir/file/file && git commit -m '$name' && test_must_fail git svn set-tree --find-copies-harder --rmdir \ - ${remotes_git_svn}..mybranch" || true + ${remotes_git_svn}..mybranch +" name='detect node change from directory to file #1' @@ -79,7 +80,8 @@ test_expect_success "$name" ' git update-index --add -- bar && git commit -m "$name" && test_must_fail git svn set-tree --find-copies-harder --rmdir \ - ${remotes_git_svn}..mybranch2' || true + ${remotes_git_svn}..mybranch2 +' name='detect node change from file to directory #2' @@ -96,7 +98,8 @@ test_expect_success "$name" ' ${remotes_git_svn}..mybranch3 && svn_cmd up "$SVN_TREE" && test -d "$SVN_TREE"/bar/zzz && - test -e "$SVN_TREE"/bar/zzz/yyy ' || true + test -e "$SVN_TREE"/bar/zzz/yyy +' name='detect node change from directory to file #2' test_expect_success "$name" ' @@ -109,7 +112,8 @@ test_expect_success "$name" ' git update-index --add -- dir && git commit -m "$name" && test_must_fail git svn set-tree --find-copies-harder --rmdir \ - ${remotes_git_svn}..mybranch4' || true + ${remotes_git_svn}..mybranch4 +' name='remove executable bit from a file' @@ -162,7 +166,7 @@ test_expect_success "$name" ' name='modify a symlink to become a file' test_expect_success "$name" ' - echo git help > help || true && + echo git help >help && rm exec-2.sh && cp help exec-2.sh && git update-index exec-2.sh && ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] git-svn.perl: fix a false-positive in the "already exists" test 2012-02-22 5:08 ` Junio C Hamano @ 2012-02-23 23:17 ` Steven Walter 0 siblings, 0 replies; 17+ messages in thread From: Steven Walter @ 2012-02-23 23:17 UTC (permalink / raw) To: Junio C Hamano; +Cc: normalperson, git Signed-Off-By: Steven Walter <stevenrwalter@gmail.com> On Wed, Feb 22, 2012 at 12:08 AM, Junio C Hamano <gitster@pobox.com> wrote: > Steven Walter <stevenrwalter@gmail.com> writes: > >>>> + test -e "$SVN_TREE"/bar/zzz/yyy ' || true >>> >>> Care to explain what this " || true" is doing here, please? >> >> Ahh, good catch. I think the answer is that it shouldn't be there. >> It was originally there because of the "test_must_fail" line, I think >> (at least the other tests that use test_must_fail also have "|| >> true"). > > Ok, that may explain the copy&paste error. > > But I do not think test_must_fail followed by || true makes much sense, > either. The purpose of "test_must_fail" is to make sure the tested git > command exits with non-zero status in a controlled way (i.e. not crash) > so if the tested command that is expected to exit with non-zero status > exited with zero status, the test has detected an *error*. E.g. if you > know that the index and the working tree are different at one point in the > test sequence, you would say: > > ... other setup steps ... && > test_must_fail git diff --exit-code && > ... and other tests ... > > so that failure by "git diff --exit-code" to exit with non-zero status > (i.e. it did not find any difference when it should have) breaks the && > cascade. > > I just took a quick look at t9100 but I think all " || true" can be safely > removed. None of them is associated with test_must_fail in any way. For > whatever reason, these test seem to do > > test_expect_success 'label of the test' ' > body of the test > ' || true > > for no good reason. > >> Do you want to just fix that up, or a new version of the original patch, >> or a fix on top of the original patches? > > Eric queued the patch and then had me pull it as part of his history > already, so it is doubly too late to replace it. > > Can you apply this patch and re-test? > > > t/t9100-git-svn-basic.sh | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh > index 4029f84..749b75e 100755 > --- a/t/t9100-git-svn-basic.sh > +++ b/t/t9100-git-svn-basic.sh > @@ -65,7 +65,8 @@ test_expect_success "$name" " > git update-index --add dir/file/file && > git commit -m '$name' && > test_must_fail git svn set-tree --find-copies-harder --rmdir \ > - ${remotes_git_svn}..mybranch" || true > + ${remotes_git_svn}..mybranch > +" > > > name='detect node change from directory to file #1' > @@ -79,7 +80,8 @@ test_expect_success "$name" ' > git update-index --add -- bar && > git commit -m "$name" && > test_must_fail git svn set-tree --find-copies-harder --rmdir \ > - ${remotes_git_svn}..mybranch2' || true > + ${remotes_git_svn}..mybranch2 > +' > > > name='detect node change from file to directory #2' > @@ -96,7 +98,8 @@ test_expect_success "$name" ' > ${remotes_git_svn}..mybranch3 && > svn_cmd up "$SVN_TREE" && > test -d "$SVN_TREE"/bar/zzz && > - test -e "$SVN_TREE"/bar/zzz/yyy ' || true > + test -e "$SVN_TREE"/bar/zzz/yyy > +' > > name='detect node change from directory to file #2' > test_expect_success "$name" ' > @@ -109,7 +112,8 @@ test_expect_success "$name" ' > git update-index --add -- dir && > git commit -m "$name" && > test_must_fail git svn set-tree --find-copies-harder --rmdir \ > - ${remotes_git_svn}..mybranch4' || true > + ${remotes_git_svn}..mybranch4 > +' > > > name='remove executable bit from a file' > @@ -162,7 +166,7 @@ test_expect_success "$name" ' > > name='modify a symlink to become a file' > test_expect_success "$name" ' > - echo git help > help || true && > + echo git help >help && > rm exec-2.sh && > cp help exec-2.sh && > git update-index exec-2.sh && -- -Steven Walter <stevenrwalter@gmail.com> ^ permalink raw reply [flat|nested] 17+ messages in thread
[parent not found: <87mx8rrf5i.fsf@thomas.inf.ethz.ch>]
[parent not found: <CAK8d-aJ3wi0e_NPunow-aBnhs1=o5K25r3e-Ha0m1U0ujTv7OA@mail.gmail.com>]
* Re: [PATCH 1/2] git-svn.perl: perform deletions before anything else [not found] ` <CAK8d-aJ3wi0e_NPunow-aBnhs1=o5K25r3e-Ha0m1U0ujTv7OA@mail.gmail.com> @ 2012-02-09 20:55 ` Thomas Rast 0 siblings, 0 replies; 17+ messages in thread From: Thomas Rast @ 2012-02-09 20:55 UTC (permalink / raw) To: Steven Walter; +Cc: normalperson, git Oops, as Steven noticed I accidentally hit the wrong reply button. So here's my earlier reply and his answer. Steven Walter <stevenrwalter@gmail.com> writes: > On Thu, Feb 9, 2012 at 2:16 PM, Thomas Rast <trast@inf.ethz.ch> wrote: >> Steven Walter <stevenrwalter@gmail.com> writes: >> >>> If we delete a file and recreate it as a directory in a single commit, >>> we have to tell the server about the deletion first or else we'll get >>> "RA layer request failed: Server sent unexpected return value (405 >>> Method Not Allowed) in response to MKCOL request" >> [...] >>> - my %o = ( D => 1, R => 0, C => -1, A => 3, M => 3, T => 3 ); >>> + my %o = ( D => -2, R => 0, C => -1, A => 3, M => 3, T => 3 ); >> >> You are making it delete first, but the original code seems to quite >> deliberately put deletion after R (rename?). Are you sure you're not >> breaking anything else? > > No, I'm not 100% sure of that. > > In fact, looking at cf52b8f063 where this code seems to have started, > it lists my case explicitly as one that subversion does not support: > > "a file is removed and a directory of the same name of the removed > file is created." > > One thing that might make a difference is that the "file" that removed > was actually a symlink. So either svn treats symlinks as a special > case to that rule, or else the limitation the commit was meant to > address is not present on recent versions of svn. I can run some > checks to see if that is the case. -- Thomas Rast trast@{inf,student}.ethz.ch ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2012-02-23 23:18 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-02-09 18:55 [PATCH 1/2] git-svn.perl: perform deletions before anything else Steven Walter
2012-02-09 18:55 ` [PATCH 2/2] git-svn.perl: fix a false-positive in the "already exists" test Steven Walter
2012-02-09 20:08 ` [PATCH 1/2] git-svn.perl: perform deletions before anything else Junio C Hamano
2012-02-09 20:52 ` Steven Walter
2012-02-09 20:52 ` Steven Walter
2012-02-12 7:03 ` Eric Wong
2012-02-12 15:35 ` Steven Walter
2012-02-12 23:49 ` Eric Wong
2012-02-15 17:47 ` Steven Walter
2012-02-19 10:54 ` Eric Wong
2012-02-20 14:17 ` [PATCH] git-svn.perl: fix a false-positive in the "already exists" test Steven Walter
2012-02-22 0:33 ` Eric Wong
2012-02-22 2:16 ` Junio C Hamano
2012-02-22 2:32 ` Steven Walter
2012-02-22 5:08 ` Junio C Hamano
2012-02-23 23:17 ` Steven Walter
[not found] ` <87mx8rrf5i.fsf@thomas.inf.ethz.ch>
[not found] ` <CAK8d-aJ3wi0e_NPunow-aBnhs1=o5K25r3e-Ha0m1U0ujTv7OA@mail.gmail.com>
2012-02-09 20:55 ` [PATCH 1/2] git-svn.perl: perform deletions before anything else Thomas Rast
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).