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

[Lea Wiemann <lewiemann@gmail.com>]

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.

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

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