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

[Lea Wiemann <lewiemann@gmail.com>]

Junio C Hamano wrote:
> Lea Wiemann <lewiemann@gmail.com> writes:
>> +The author, committer and message methods return Unicoded strings,
> 
> Unicoded?

Fixed. :)

>> +# Auxiliary method to load (and parse) the commit object from the
>> +# repository if it hasn't already been loaded. [...]
>> +sub _load {
>
> Aside from seeming repetitive and quite similar to Git::Tag::_load(),

I didn't see a clean way to extract common code since there are
differences in how some headers are parsed (e.g. 'parents' is an array).

> 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.

I'm not sure if I understand grafts correctly, but would something like
the following piece of text in the documentation for
Git::Commit->parents cover it?

"Note that this method does not currently respect grafts.  Hence, if you
are operating on a repository that contains grafts (e.g. a shallow
clone), the commit objects returned by the 'parents' method may not
exist in the repository; your program will die if you call an attribute
accessor method (such as 'author' or 'message') of any parent of an
object with grafted parents.  Thus, if you want your program to work
with shallow clones, you will have to check the .git/info/grafts file
yourself before accessing any parents.

You must not rely on this behavior.  In the future, the 'parents' method
may return the grafted parents instead of the parents recorded in the
commit object."

> 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,

You're referring to gitweb here, right?  (The perl API doesn't use
rev-list.)  I don't think gitweb is currently tested with grafted
repositories at all, but in general it should respect rewritten parents,
since the parse_commit function uses rev-list without the 'raw' format,
and thus it should get the rewritten parents.

-- Lea