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

From: Petr Baudis <pasky@suse.cz>
Date: Jul 18, 2008, 18:19 UTC
Message-ID: <20080718181940.GW10151@machine.or.cz>
In-Reply-To: <4880DC6C.7090708@gmail.com>

On Fri, Jul 18, 2008 at 08:09:48PM +0200, Lea Wiemann wrote:

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

One of the points of the API is to abstract these out.

Show 14 quoted lines

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

I'm fine with that, as long as the version that enters into master will have a pony so that we stay with a single pony within the codebase in the end, not two ponies with differently shaped saddles.

But as I said, I'm going to work on that.

Show 8 quoted lines

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

But the API is still obnoxiously elaborate, as I complained in another mail.

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

Wait, this doesn't compute here. The error handling is fine, but it is actually not fine for gitweb. Can't we make it fine for everyone?

-- 
				Petr "Pasky" Baudis
As in certain cults it is possible to kill a process if you know
its true name.  -- Ken Thompson and Dennis M. Ritchie