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

From: Lea Wiemann <lewiemann@gmail.com>
Date: Jul 18, 2008, 18:09 UTC
Message-ID: <4880DC6C.7090708@gmail.com>
In-Reply-To: <20080718164828.GT10151@machine.or.cz>

Petr Baudis wrote:

> [$repo->_cmd_output:]
> 
> we _need_ such a wrapper _publically_, because it tends to be
> actually the main use-case of Git.pm,

Well, sure, I happen to not be convinced, but it *may* be useful. The point I'm trying to make is that it's not part of what I'm writing here.

Show 5 quoted lines

> as part of your gitweb migration to Git::Repo, you will temporarily
> introduce calls to _cmd_output(), the "internal" API. :-) Sure, it's
> only temporary, but many won't have the luxury to adjust the Git::Repo
> API to provide all the operations they need, and ultimately they will
> need to defer to the pipe interface.

Yup, and I'm actually fine with that. (I'll probably alias _cmd_output to cmd_output in gitweb, just to make it clear that it is, for the purpose of gitweb, a *supported* mode of operation.) If the Git::Repo::_cmd_output API goes away, you'll have to insert a few lines of code in gitweb, but that's it. Really, no big deal.

Also, gitweb isn't using cmd_output because it needs a pipe interface, but because it needs a caching layer in between -- most applications would do just fine with open calls.

> As I said, majority of Git API usage is actually the pipe API. So we
> should figure out how to provide it. I agree that it's not immediately
> within your scope, but you are introducing new Perl API and this just
> needs to be embedded somewhere there consistently.

Sure, but pleeeease not as part of this patch series! :-) Look, our conversation is going something like this:

Lea: Here's a Perl API that fell out of my gitweb development for free.
Petr: I want a pony with the API!
Lea: But I don't have a pony.  Can we please just go with the Perl API
as a start, even if I don't supply ponies with it?

(Cf. the very cute <http://c2.com/cgi/wiki?IwantaPony>.)

>> If you're getting a SHA1 through the user-interface, check its existence
>> with get_sha1 before passing it to the constructor.
> 
> But that's an expensive operation, you need extra Git exec for this,

For the gazillionth time in this thread, there is no extra exec. It's a write to a bidirectional cat-file --batch-check pipe. It's not expensive. Really. ;-)

Show 5 quoted lines

>> I have resolving code in gitweb's git_get_sha1_or_die
> 
> The thing that concerns me about this is that this might show that your
> approach to error handling is not flexible enough for some real-world
> usage and this might be a design mistake - is that not so?

I don't think so; the error handling is fine. Given that I want fine-granular error reporting for gitweb, there *needs* to be a git_get_sha1_or_die function; you can't move that into the API.

-- Lea