Subject: Re: [PATCH 2/3 v2] add new Perl API: Git::Repo, Git::Commit, Git::Tag, and Git::RepoRoot

From: Junio C Hamano <[email-masked]>
Date: Mon, 18 Aug 2008 18:32:07 -0700

Lea Wiemann <[email-masked]> writes:

> +=head1 DESCRIPTION
> +...
> +The author, committer and message methods return Unicoded strings,

Unicoded?

> +decoded according to the "encoding" header, with UTF-8 and then
> +Latin-1 as fallbacks.  (These Unicode strings can contain code points
> +...
> +decoding for you, so you should not normally need this method.

Anyway, nicely written description.

> +=back
> +
> +=cut
> +
> +
> +sub tree {
> +	my $self = shift;
> +	$self->_load;
> +	return $self->{tree};
> +}
> +
> +sub parents {
> +	my $self = shift;
> +	$self->_load;
> +	return map { ref($self)->new($self->repo, $_) } @{$self->{parents}};
> +}
> +
> +sub author {
> +	my $self = shift;
> +	$self->_load;
> +	return $self->_decode($self->{author});
> +}
> +
> +sub committer {
> +	my $self = shift;
> +	$self->_load;
> +	return $self->_decode($self->{committer});
> +}
> +
> +sub message {
> +	my $self = shift;
> +	$self->_load;
> +	return $self->_decode($self->{message});
> +}
> +
> +sub encoding {
> +	my $self = shift;
> +	$self->_load;
> +	return $self->{encoding};
> +}
> +
> +# Auxiliary method to load (and parse) the commit object from the
> +# repository if it hasn't already been loaded.  Optional parameter:
> +# The raw contents of the commit object; the commit object will be
> +# retrieved from the repository if that parameter is not given.
> +sub _load {
> +	my ($self, $raw_text) = shift;
> +	return if exists $self->{message};  # already loaded
> +
> +	my $sha1 = $self->sha1;
> +	if (!defined $raw_text) {
> +		# Retrieve from the repository.
> +		(my $type, $raw_text) = $self->repo->get_object($sha1);
> +		die "$sha1 is a $type object (expected a commit object)"
> +		    unless $type eq 'commit';
> +	}
> +
> +	(my $header, $self->{message}) = split "\n\n", $raw_text, 2;
> +	# Parse header.
> +	for my $line (split "\n", $header) {
> +		local $/ = "\n"; # for chomp
> +		chomp($line);
> +		my ($key, $value) = split ' ', $line, 2;
> +		if ($key eq 'tree') {
> +			$self->{tree} = $value;
> +		} elsif ($key eq 'parent') {
> +			push @{$self->{parents}}, $value;
> +		} elsif ($key eq 'author') {
> +			$self->{author} = $value;
> +		} elsif ($key eq 'committer') {
> +			$self->{committer} = $value;
> +		} elsif ($key eq 'encoding') {
> +			$self->{encoding} = $value;
> +		} else {
> +			# Ignore unrecognized header lines.
> +		}
> +	}
> +	undef;
> +}

Aside from seeming repetitive and quite similar to Git::Tag::_load(), I
have to wonder how parent rewriting and grafts come into the picture.
Git::Repo::get_object is just a cat-file which means you are ignoring any
grafts.  As a design decision it is fine, but it needs to be documented.

Also if you run "per path history" using "rev-list $head -- $path" to
collect commits that touch the named $path, you might want to get
rewritten parents for each commit and use it in the presentation, but it
is something you cannot cache easily (i.e. you should not be reusing the
parent list rewritten with respect to other paths).

The tests looked fine, too.