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

On Nov 11, 2007, at 3:20 PM, David D. Kilzer wrote:

Benoit Sigoure [off-list ref] wrote:

quoted

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

quoted

 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 :)

What is the format for documenting functions in git Perl scripts?   
I haven't
see any "perlpod" use anywhere.  Do you just want comments before  
the function?

This method returns the git commit hash and svn revision of the  
first svn
revision that exists on the current branch that is less than $rev (or
less-than-or-equal-to $rev if $eq_ok is true).

Personally, I don't care.  Maybe Eric has his own preference.  For  
me, as long as the code is documented one way or another, that's fine  
by me.  Under-documented code hinders new contributors, so I think  
it's important to add documentation whenever possible.

Please note that I don't have a full understanding of how  
find_rev_before()
works (other than it's computing an offset into a sparse? data file  
based on
the revision number) since I'm still new to git.

quoted

quoted

+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; });
}

I think that combining find_rev_before() and find_rev_after() would  
greatly
sacrifice readability of the code in exchange for removing ~10  
lines of code.
Also, you must do more than just replace the comparison in the while 
() loop:

- before() decrements $rev while after() increments it
- stop limits are different ($max_rev versus $min_rev)

This is what such a method might look like (untested).  Since you  
already
requested find_rev_before() be documented, is this really going to  
help?

sub find_rev_ {
	my ($self, $rev, $eq_ok, $is_before, $limit_rev) = @_;
	($is_before ? --$rev : ++$rev) unless $eq_ok;
	$limit_rev ||= ($is_before ? 1 : $self->rev_db_max());
	while ($is_before ? $rev >= $limit_rev : $rev <= $limit_rev) {
		if (my $c = $self->rev_db_get($rev)) {
			return ($rev, $c);
		}
			$is_before ? --$rev : ++$rev;
	}
	return (undef, undef);
}

Defining wrapper functions would help, but I still think it's just  
as clear to
keep the two methods separate.

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

sub find_rev_after() {
	my ($self, $rev, $eq_ok, $max_rev) = @_;
	return $self->find_rev_($rev, $eq_ok, 0, $max_rev);
}

Do you agree, or do you think the methods should still be combined?

Er... you're right, I overlooked the differences between the two  
functions.  So merging them would make the code more complex than it  
needs to be, or so I think.

Thanks!

-- 
Benoit Sigoure aka Tsuna
EPITA Research and Development Laboratory