Re: [PATCH 2/3] add new Git::Repo API

Petr Baudis wrote:

Yup, I agree (and you don't sound too mentoring ^^). Thanks for the review and feedback.

Git::Repo is not interacting with working copies at all right now. Is there anything you think should be considered for its design?

Here's a write-up about the design (I'll probably move this into Git::Repo's man page):

---------- Git::Repo aims to provide low-level access to Git repositories. For instance, you can resolve object names (like "HEAD~2") to SHA1s, and inspect objects. It does not attempt to be a wrapper around the git plumbing or porcelain commands.

Error handling is simple: On a consistent repository, the Perl interface will never die. You can use the get_sha1 method to resolve arbitrary object names or check the existence of SHA1 hashes; get_sha1 will return undef if the object does not exist in the repository. Any SHA1 that is returned by get_sha1 can be safely passed to the other Git::Repo methods. ----------

Well, they're more like helper methods. Since they don't fit into the design goals of the Git::Repo API at all, I'd suggest we just underscore-prefix them and take them out of the man page. (The only reason why I hadn't done this is that gitweb uses $repo->cmd_output extensively, so it'd end up with a lot of underscore calls. But I suppose we can either alias _cmd_output to cmd_output in gitweb's CachedRepo subclass, or live with $repo->_cmd_output calls.) Does underscore-prefixing sound good to you?

If someone wants to come up with a consistent nice interface for calling git commands, sure. I wasn't actually trying to do that.

Yes, I think so. ;-) All we're talking about here is a wrapper around "open '-|'" calls (plus some workarounds for Windows I suppose).

I don't have much of a notion of a 'command' as an object in my head; your (snipped) example makes it look like you're trying to create a IO::Handle-compatible interface, which I think is way too much effort (and error-prone) for simple pipes. Also, a CommandFactory class just to catenate lists together seems like overkill to me.

Something like a command interface *may* make sense if it's tied to repositories or working copies, in which case it could automatically set --git-dir or --work-tree, but it's beyond the scope of what I'm trying to create here, and I don't think it's even overly useful.

Well, as I said, the fact that there are untested parts in Git.pm doesn't exactly make it trivial to refactor. Also, only very few parts can be cleanly moved to Git::Repo. In "grep '^sub [^_]' Git.pm" I find only cat_blob and perhaps hash_object to be eligible to be moved (though hash_object should probably live in a working-copy/non-bare-repo class, with an optional insert => 1 parameter). And even cat_blob is non-trivial to move unless you want the whole blob to be read into memory.

That's a lot of non-trivialness for very little gain. I doubt I'd even have enough time till the end of GSoC (minus vacation) to do this.

Creating a new (Git::Command) API is very much non-trivial, apart from the fact that I'm not convinced that we need Git::Command, and that a clean command interface neither falls out of Git.pm nor Git::Repo.

Sorry, I wasn't clear with my parenthesed remark: I actually think that Git::Repo is pretty good in terms of code and interface quality. It's just not *complete*, even in its limited scope, and I'm not attempting to make it complete.

I do think that someone who wants to extend Git::Repo (like Jakub with Git::Config) won't have much trouble doing so with the existing design.

Embarrassingly premature optimization here. ^_^  I'll fix it.

*ponders*  Every commit needs a repo and a SHA1, so those will never get
optional.  We can always add hash-based options after the two mandatory
arguments, but I don't even see any such possible options at the moment.
 (And if I turn out to be completely wrong, we can even move to a
hash-only argument list by checking the type of the first parameter.)
Really, I wouldn't worry.

No, you should never pass in an invalid SHA1 in the first place. The above piece of documentation is just a warning that bugs will show up delayed. IOW, this is not the right place to have your error handling.

If you're getting a SHA1 through the user-interface, check its existence with get_sha1 before passing it to the constructor.

Because the SHA1 might resolve to an object of the wrong type, which means you have to do error handling in Git::Object objects; that's the wrong place.

If tag-resolving is really needed, we can add an optional $type parameter to get_sha1, which will cause get_sha1 to resolve the object until a $type object is found, or return undef if the object is or resolves to an object of the wrong type.

I have resolving code in gitweb's git_get_sha1_or_die (which I didn't
implement in Git::Repo since it uses some customized error reporting).
The resolving code could conceivably be extracted and moved to get_sha1.
 I think there are a few things to ponder and maybe discuss, so I'd do
that in a separate patch (if I get around it before the end of the project).

Fixed.

I've replaced it with: "Objects are loaded lazily, and hence instantiation is free. Git::Object instances stringify to their SHA1s."

Done (and removed stringify).

Done, and removed them from @EXPORT_OK.

Sure, it can be made non-mandatory if it's needed, but there are so many possibilities for the exact time and place at which the repo directory should be resolved using rev-parse (if at all) that I'd rather leave this to the person who has an actual use-case for it. I'm not a fan of designing APIs before they are needed.

Which of those methods are you referring to? I think $repo->version might reasonably be removed (and the code re-added to gitweb); I'll do so unless you object. _get_git_cmd is already underscored, and repo_dir and git_binary only access attributes passed in through the constructor, so I think those three should stay.

Yup; I like get_object (I think I was planning to rename it and then didn't remember doing so before sending off the patch). Will rename it.

Yup, that's true. I'll move it into gitweb until we have Git::Tree (with a comment that it can be moved to Git::Tree once it exists).

Really? I think it's generally fine for get_refs to exist and to live in Git::Repo.

Its return value (currently an an arrayref of [$sha1, $object_type, $ref_name] arrayrefs) might need improvement though, and I find the $pattern parameter pretty suspect (in that it smells like a for-each-ref wrapper). Since get_refs is unused at the moment (gitweb ended up needing the slightly different show-ref), I'll remove it for now. (Same thing about me not being a fan of premature API design applies.)

I keep patches of everything I remove so other people will be able to use them as starting points; I'll probably post them as "FYI"-patches to the list at the end of my project, to preserve them for posterity.