Re: [PATCH 3/3] git-svn log: handle unreachable revisions like "svn log"

Benoit Sigoure <tsuna@xxxxxxxxxxxxx> · Sun, 11 Nov 2007 11:51:33 +0100

Hi David,
thanks for the patches, the series looks good to me, I added some  
comments below, for this patch.

On Nov 11, 2007, at 7:10 AM, David D Kilzer wrote:

When unreachable revisions are given to "svn log", it displays all  
commit
logs in the given range that exist in the current tree.  (If no commit
logs are found in the current tree, it simply prints a single  
commit log
separator.)  This patch makes "git-svn log" behave the same way.

Ten tests added to t/t9116-git-svn-log.sh.

Signed-off-by: David D Kilzer <ddkilzer@xxxxxxxxxx>
---
 git-svn.perl           |   58 +++++++++++++++++++++++++++ 
+--------------
 t/t9116-git-svn-log.sh |   66 +++++++++++++++++++++++++++++++++++++ 
+++++++++++
 2 files changed, 105 insertions(+), 19 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 39585d8..c584715 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -2257,9 +2257,10 @@ sub rev_db_get {
 }

 sub find_rev_before {
-	my ($self, $rev, $eq_ok) = @_;
+	my ($self, $rev, $eq_ok, $min_rev) = @_;

Could you please document this function?  I guess that you had to  
figure out what each argument was for, so please save the time of the  
contributors that will read this code after you :)

 	--$rev unless $eq_ok;
-	while ($rev > 0) {
+	$min_rev ||= 1;
+	while ($rev >= $min_rev) {
 		if (my $c = $self->rev_db_get($rev)) {
 			return ($rev, $c);
 		}
@@ -2268,6 +2269,19 @@ sub find_rev_before {
 	return (undef, undef);
 }

+sub find_rev_after {
+	my ($self, $rev, $eq_ok, $max_rev) = @_;
+	++$rev unless $eq_ok;
+	$max_rev ||= $self->rev_db_max();
+	while ($rev <= $max_rev) {
+		if (my $c = $self->rev_db_get($rev)) {
+			return ($rev, $c);
+		}
+		++$rev;
+	}
+	return (undef, undef);
+}
+

Too much code duplication.  It should be possible to write a sub  
find_rev_ (or _find_rev, don't know what's the naming convention for  
internal details) that takes a 5th argument, an anonymous sub that  
does the comparison.  So that basically, find_rev_before will be  
something along these (untested) lines:

sub find_rev_before {
	my ($self, $rev, $eq_ok, $min_rev) = @_;
	return find_rev_($self, $rev, $eq_ok, $min_rev, sub { my ($a, $b) =  
@_; return $a >= $b; });
}

 sub _new {
 	my ($class, $repo_id, $ref_id, $path) = @_;
 	unless (defined $repo_id && length $repo_id) {
@@ -3587,19 +3601,19 @@ sub git_svn_log_cmd {
 			push @cmd, $c;
 		}
 	} elsif (defined $r_max) {
-		my ($c_min, $c_max);
-		$c_max = $gs->rev_db_get($r_max);
-		$c_min = $gs->rev_db_get($r_min);
-		if (defined $c_min && defined $c_max) {
-			if ($r_max > $r_min) {
-				push @cmd, "--boundary", "$c_min..$c_max";
-			} else {
-				push @cmd, "--boundary", "$c_max..$c_min";
-			}
-		} elsif ($r_max > $r_min) {
-			push @cmd, $c_max;
+		if ($r_max < $r_min) {
+			($r_min, $r_max) = ($r_max, $r_min);
+		}
+		my (undef, $c_max) = $gs->find_rev_before($r_max, 1, $r_min);
+		my (undef, $c_min) = $gs->find_rev_after($r_min, 1, $r_max);
+		# If there are no commits in the range, both $c_max and $c_min
+		# will be undefined.  If there is at least 1 commit in the
+		# range, both will be defined.
+		return () if !defined $c_min;

Fair enough but I'd strengthen the test by writing something like:
		return () if not defined $c_min || not defined $c_max;
unless you can prove that `rev_db_get' can never return `undef' or  
something like that.

+		if ($c_min eq $c_max) {
+			push @cmd, '--max-count=1', $c_min;
 		} else {
-			push @cmd, $c_min;
+			push @cmd, '--boundary', "$c_min..$c_max";
 		}
 	}
 	return (@cmd, @files);
@@ -3705,9 +3719,13 @@ sub show_commit_changed_paths {
 	print "Changed paths:\n", @{$c->{changed}};
 }

+sub commit_log_separator {
+    return ('-' x 72) . "\n";
+}
+

This is basically a constant, I think that declaring it with a  
prototype helps Perl to optimize it:
sub commit_log_separator() {

 sub show_commit_normal {
 	my ($c) = @_;
-	print '-' x72, "\nr$c->{r} | ";
+	print commit_log_separator(), "r$c->{r} | ";
 	print "$c->{c} | " if $show_commit;
 	print "$c->{a} | ", strftime("%Y-%m-%d %H:%M:%S %z (%a, %d %b %Y)",
 				 localtime($c->{t_utc})), ' | ';
diff --git a/t/t9116-git-svn-log.sh b/t/t9116-git-svn-log.sh
index 5000892..56dd8fe 100755
--- a/t/t9116-git-svn-log.sh
+++ b/t/t9116-git-svn-log.sh
@@ -59,4 +65,64 @@ test_expect_success 'test descending revision  
range' "
[...]
+
+echo  
---------------------------------------------------------------------- 
-- > expected-separator

This will choke on shells with buggy/fragile `echo'.  I think it'd be  
safer to use printf here.


Cheers,

--
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory


Attachment:
PGP.sig

Description: This is a digitally signed message part




