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

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

quoted

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

quoted

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