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

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

[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

[PATCH 2/2] git-svn.perl: fix a false-positive in the "already exists" test

[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

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

[Junio C Hamano]

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.

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

[Steven Walter]

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

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

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

[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

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

[Thomas Rast]

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

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

[Eric Wong]

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?

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

[Steven Walter]

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>

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

[Eric Wong]

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

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

[Steven Walter]

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>

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

[Eric Wong]

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!

[PATCH] git-svn.perl: fix a false-positive in the "already exists" test

[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

Re: [PATCH] git-svn.perl: fix a false-positive in the "already exists" test

[Eric Wong]

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>

Re: [PATCH] git-svn.perl: fix a false-positive in the "already exists" test

[Junio C Hamano]

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?

Re: [PATCH] git-svn.perl: fix a false-positive in the "already exists" test

[Steven Walter]

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>

Re: [PATCH] git-svn.perl: fix a false-positive in the "already exists" test

[Junio C Hamano]

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

Re: [PATCH] git-svn.perl: fix a false-positive in the "already exists" test

[Steven Walter]

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>