Re: [PATCH 1/2] git-svn.perl: perform deletions before anything else

Steven Walter [off-list ref] 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.

From: Steven Walter <redacted>

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

I suspect that writing it this way [...] would make it much easier to
follow.

Agreed.  New patch to follow, this time with sign-off.

Steven Walter [off-list ref] wrote:

Signed-off-by: Steven Walter <redacted>

Thanks, shall I fixup 2/2 and assume you meant to Sign-off on that, too?

On Sun, Feb 12, 2012 at 2:03 AM, Eric Wong [off-list ref] wrote:

Steven Walter [off-list ref] wrote:

quoted

Signed-off-by: Steven Walter <redacted>

Thanks, shall I fixup 2/2 and assume you meant to Sign-off on that, too?

Yes, thanks
-- 
-Steven Walter [off-list ref]

Steven Walter [off-list ref] wrote:

On Sun, Feb 12, 2012 at 2:03 AM, Eric Wong [off-list ref] wrote:

quoted

Steven Walter [off-list ref] wrote:

quoted

Signed-off-by: Steven Walter <redacted>

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!.

On Sun, Feb 12, 2012 at 6:49 PM, Eric Wong [off-list ref] wrote:

Steven Walter [off-list ref] wrote:

quoted

On Sun, Feb 12, 2012 at 2:03 AM, Eric Wong [off-list ref] wrote:

quoted

Steven Walter [off-list ref] wrote:

quoted

Signed-off-by: Steven Walter <redacted>

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 [off-list ref]

Steven Walter [off-list ref] 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!

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

Steven Walter [off-list ref] 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 <redacted>

Thanks, will push.
Acked-by: Eric Wong <redacted>