git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Petr Baudis <pasky@suse.cz>
To: Lea Wiemann <lewiemann@gmail.com>
Cc: Jakub Narebski <jnareb@gmail.com>,
	git@vger.kernel.org, John Hawley <warthog19@eaglescrag.net>
Subject: Re: [PATCH 2/3] add new Git::Repo API
Date: Fri, 18 Jul 2008 20:19:40 +0200	[thread overview]
Message-ID: <20080718181940.GW10151@machine.or.cz> (raw)
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.

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

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

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

  reply	other threads:[~2008-07-18 18:20 UTC|newest]

Thread overview: 63+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-07-11  1:06 [PATCH 0/3] Git::Repo API and gitweb caching Lea Wiemann
2008-07-11  1:10 ` [PATCH 1/3 v9] gitweb: add test suite with Test::WWW::Mechanize::CGI Lea Wiemann
2008-07-11  1:11 ` [PATCH 2/3] add new Git::Repo API Lea Wiemann
2008-07-13 21:38   ` Junio C Hamano
2008-07-14  1:04     ` Lea Wiemann
2008-07-13 23:28   ` Jakub Narebski
2008-07-14  2:29     ` Lea Wiemann
2008-07-14  1:40   ` Petr Baudis
2008-07-14 22:19     ` Lea Wiemann
2008-07-18 16:48       ` Petr Baudis
2008-07-18 17:05         ` Jakub Narebski
2008-07-18 17:17           ` Petr Baudis
2008-07-18 18:09         ` Lea Wiemann
2008-07-18 18:19           ` Petr Baudis [this message]
2008-07-18 18:23           ` Johannes Schindelin
2008-07-19 20:54         ` Statictics on Git.pm usage in git commands (was: [PATCH 2/3] add new Git::Repo API) Jakub Narebski
2008-07-19 21:14           ` Petr Baudis
2008-07-20  0:16             ` Jakub Narebski
2008-07-20 21:38               ` Petr Baudis
2008-07-20 10:38           ` Johannes Schindelin
2008-07-20 10:49             ` Petr Baudis
2008-07-20 12:33               ` Johannes Schindelin
2008-07-20 12:58                 ` Petr Baudis
2008-07-20 13:21                   ` Johannes Schindelin
2008-07-14 23:41     ` [PATCH 2/3] add new Git::Repo API Jakub Narebski
2008-07-15  0:11       ` Lea Wiemann
2008-07-18 16:54       ` Petr Baudis
2008-07-19  0:03         ` Jakub Narebski
2008-07-19 19:07         ` Jakub Narebski
2008-07-20 21:36           ` Petr Baudis
2008-07-20 21:50             ` Jakub Narebski
2008-07-16 18:21   ` Jakub Narebski
2008-07-16 20:32     ` Lea Wiemann
2008-07-17 23:49       ` Jakub Narebski
2008-07-18 13:40         ` Lea Wiemann
2008-07-18 15:35           ` Jakub Narebski
2008-07-18 16:51             ` Lea Wiemann
2008-07-11  1:11 ` [PATCH 3/3] gitweb: use new Git::Repo API, and add optional caching Lea Wiemann
2008-07-14 21:23   ` Jakub Narebski
2008-07-14 23:03     ` Lea Wiemann
2008-07-14 23:14       ` Jakub Narebski
2008-07-14 23:56         ` Lea Wiemann
2008-07-15  0:52           ` Jakub Narebski
2008-07-15  1:16             ` Lea Wiemann
2008-07-15  1:28               ` Johannes Schindelin
2008-07-15  1:44                 ` J.H.
2008-07-15  1:50                 ` Lea Wiemann
2008-07-15  2:03                   ` J.H.
2008-07-11  1:21 ` [PATCH 0/3] Git::Repo API and gitweb caching Johannes Schindelin
2008-07-11  9:33 ` Jakub Narebski
2008-07-11 14:07   ` Lea Wiemann
2008-07-11 16:27     ` Abhijit Menon-Sen
2008-07-12 15:08       ` Jakub Narebski
2008-07-19  5:35 ` Lea Wiemann
2008-08-18 19:34 ` Lea Wiemann
2008-08-18 19:39   ` [PATCH 1/3 v10] gitweb: add test suite with Test::WWW::Mechanize::CGI Lea Wiemann
2008-08-19  1:17     ` Junio C Hamano
2008-08-19 14:37       ` Lea Wiemann
2008-08-18 19:39   ` [PATCH 2/3 v2] add new Perl API: Git::Repo, Git::Commit, Git::Tag, and Git::RepoRoot Lea Wiemann
2008-08-19  1:32     ` Junio C Hamano
2008-08-19 15:06       ` Lea Wiemann
2008-08-19 13:51     ` Lea Wiemann
2008-08-18 19:39   ` [PATCH 3/3 v2] gitweb: use new Git::Repo API, and add optional caching Lea Wiemann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080718181940.GW10151@machine.or.cz \
    --to=pasky@suse.cz \
    --cc=git@vger.kernel.org \
    --cc=jnareb@gmail.com \
    --cc=lewiemann@gmail.com \
    --cc=warthog19@eaglescrag.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).