git-svn: doublecheck if really file or dir

Hi Eric,

Nice touch, It works like charm. However unfortunatelly now I think you introduced new bug :)

Quoted 32 lines · Eric Wong

Hi Andrej, I could not help thinking your patch was obscuring another bug. I think I have an alternative to your patch which fixes both our bugs. Can you give this a shot? Thanks.

--------------------------- 8< ----------------------------
Subject: [PATCH] git svn: info: correctly handle absolute path args

Calling "git svn info $(pwd)" would hit:
"Reading from filehandle failed at ..."
errors due to improper prefixing and canonicalization.

Strip the toplevel path from absolute filesystem paths to ensure downstream canonicalization routines are only exposed to paths tracked in git (or SVN).

Noticed-by Andrej Manduch <[email protected]>

Signed-off-by Eric Wong <[email protected]>

---
 git-svn.perl            | 21 +++++++++++++++------
 t/t9119-git-svn-info.sh | 10 ++++++++++
 2 files changed, 25 insertions(+), 6 deletions(-)

git-svn.perl +0 -1

index 1f41ee1..1f9582b 100755
@@ -1477,10 +1477,19 @@ sub cmd_commit_diff {
 	}
 }
 
-
 sub cmd_info {

git-svn.perl sub cmd_commit_diff { · line 1488

Quoted 10 lines

git-svn.perl +8 -2

-	my $path = canonicalize_path(defined($_[0]) ? $_[0] : ".");
-	my $fullpath = canonicalize_path($cmd_dir_prefix . $path);
+	my $path_arg = defined($_[0]) ? $_[0] : '.';
+	my $path = $path_arg;
+	if ($path =~ m!\A/!) {
+		my $toplevel = eval {
+			my @cmd = qw/rev-parse --show-toplevel/;
+			command_oneline(\@cmd, STDERR => 0);
+		};
+		$path =~ s!\A\Q$toplevel\E/?!!;

I have problem with this line ^^^

Suppose your $toplevel is "/sometning" and you type in command line something like that: "git svn info /somethingsrc" and as you see this should end up with error. However "$path =~ s!\A\Q$toplevel\E/?!!;" will just cut "/sometning" from "/somethingsrc" and and up with same answer as for "svn git info src" which is not equivalent query.

Second scenario is something which worries me more: If your query look like this: "git svn info /something//src" it will just end up with error because it will set $path to "/src" witch is outside of repository.

Second scenario can be fixed with this:

git-svn.perl +0 -0

index a69f0fc..00f9d01 100755

However I'm not sure if this will work on windows (where slashes are in different orientation).

Quoted 55 lines · Eric Wong

git-svn.perl +7 -3

+		$path = canonicalize_path($path);
+	} else {
+		$path = canonicalize_path($cmd_dir_prefix . $path);
+	}
 	if (exists $_[1]) {
 		die "Too many arguments specified\n";
 	}

@@ -1501,14 +1510,14 @@ sub cmd_info {
 	# canonicalize_path() will return "" to make libsvn 1.5.x happy,
 	$path = "." if $path eq "";
 
-	my $full_url = canonicalize_url( add_path_to_url( $url, $fullpath ) );
+	my $full_url = canonicalize_url( add_path_to_url( $url, $path ) );
 
 	if ($_url) {
 		print "$full_url\n";
 		return;
 	}
 
-	my $result = "Path: $path\n";
+	my $result = "Path: $path_arg\n";
 	$result .= "Name: " . basename($path) . "\n" if $file_type ne "dir";
 	$result .= "URL: $full_url\n";
 

@@ -1539,7 +1548,7 @@ sub cmd_info {
 	}
 
 	my ($lc_author, $lc_rev, $lc_date_utc);
-	my @args = Git::SVN::Log::git_svn_log_cmd($rev, $rev, "--", $fullpath);
+	my @args = Git::SVN::Log::git_svn_log_cmd($rev, $rev, "--", $path);
 	my $log = command_output_pipe(@args);
 	my $esc_color = qr/(?:\033\[(?:(?:\d+;)*\d*)?m)*/;
 	while (<$log>) {

t/t9119-git-svn-info.sh +10 -0

index ff19695..4f6e669 100755
@@ -74,6 +74,16 @@ test_expect_success 'info .' "
 	test_cmp_info expected.info-dot actual.info-dot
 	"
 
+test_expect_success 'info $(pwd)' '
+	(cd svnwc; svn info "$(pwd)") >expected.info-pwd &&
+	(cd gitwc; git svn info "$(pwd)") >actual.info-pwd &&
+	grep -v ^Path: <expected.info-pwd >expected.info-np &&
+	grep -v ^Path: <actual.info-pwd >actual.info-np &&
+	test_cmp_info expected.info-np actual.info-np &&
+	test "$(sed -ne \"/^Path:/ s!/svnwc!!\" <expected.info-pwd)" = \
+	     "$(sed -ne \"/^Path:/ s!/gitwc!!\" <actual.info-pwd)"
+	'
+
 test_expect_success 'info --url .' '
 	test "$(cd gitwc; git svn info --url .)" = "$quoted_svnrepo"
 	'

Quoted 2 lines · Andrej Manduch

Nice touch, It works like charm. However unfortunatelly now I think you introduced new bug :)

Quoted 20 lines · Eric Wong

Review

Quoted 11 lines

sub cmd_info {

(quoted diff) +8 -2

-	my $path = canonicalize_path(defined($_[0]) ? $_[0] : ".");
-	my $fullpath = canonicalize_path($cmd_dir_prefix . $path);
+	my $path_arg = defined($_[0]) ? $_[0] : '.';
+	my $path = $path_arg;
+	if ($path =~ m!\A/!) {
+		my $toplevel = eval {
+			my @cmd = qw/rev-parse --show-toplevel/;
+			command_oneline(\@cmd, STDERR => 0);
+		};
+		$path =~ s!\A\Q$toplevel\E/?!!;

I have problem with this line ^^^

Suppose your $toplevel is "/sometning" and you type in command line something like that: "git svn info /somethingsrc" and as you see this should end up with error. However "$path =~ s!\A\Q$toplevel\E/?!!;" will just cut "/sometning" from "/somethingsrc" and and up with same answer as for "svn git info src" which is not equivalent query.

Quoted 6 lines

Second scenario is something which worries me more: If your query look like this: "git svn info /something//src" it will just end up with error because it will set $path to "/src" witch is outside of repository.

Second scenario can be fixed with this:

Actualy this will be even better:

Signed-off-by Andrej Manduch <[email protected]>

git-svn.perl +1 -0

@@ -1483,6 +1483,7 @@ sub cmd_info {
 			my @cmd = qw/rev-parse --show-toplevel/;
 			command_oneline(\@cmd, STDERR => 0);
 		};
+		$path = canonicalize_path($path);
 		$path =~ s!\A\Q$toplevel\E/?!!;
 		$path = canonicalize_path($path);
 	} else {

Because this will have not problem with really weird query like: "git svn info /media/../media/something//src"