From: Benoit Sigoure Date: Sun, 11 Nov 2007 10:51:33 GMT Subject: Re: [PATCH 3/3] git-svn log: handle unreachable revisions like "svn log" Message-ID: In-Reply-To: <1194761435-7286-4-git-send-email-ddkilzer@kilzer.net> 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 > --- > 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