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

[Petr Baudis <pasky@suse.cz>]

On Tue, Jul 15, 2008 at 01:41:38AM +0200, Jakub Narebski wrote:
> On Mon, 14 July 2008, Petr Baudis wrote:
> > Here is an idea: Introduce Git::Command object that will have very
> > general interface and look like
> > 
> > 	my $c = Git::Command->new(['git', '--git-dir=.', 'cat-file', \
> > 		'-p', 'bla'], {pipe_out=>1})
> > 	...
> > 	$c->close();
> 
> Errr... how do you read from such a pipe?  <$c> I think wouldn't work,
> unless you would use some trickery...

That's good point; it might either be done using some trickery, or
$c->pipe. The idea behind having a special object for it though is to
have *unified* (no matter how simple) error handling. You might not
detect the command erroring out at the open time.

Is there a better approach for solving this?

> > and a Git::CommandFactory with a nicer interface that would look like
> > 
> > 	my $cf = Git::CommandFactory->new('git', '--git-dir=.');
> > 	my $c = $cf->output_pipe('cat-file', '-p', 'bla');
> > 	$c->close();
> > 
> > Then, Git::Repo would have a single Git::CommandFactory instance
> > pre-initialized with the required calling convention, and returned by
> > e.g. cmd() method. Then, from the user POV, you would just:
> > 
> > 	my $repo = Git::Repo->new;
> > 	$repo->cmd->output_pipe('cat-file', '-p', 'bla');
> > 
> > Or am I overdoing it?
> 
> You are probably overdoing it.
> 
> 
> I think it would be good to have the following interface
> 
> Git->output_pipe('ls-remotes', $URL, '--heads');

This is problematic; I think mixing the new and old interface within a
single class is very bad idea, we should have Git::Standalone or
something for this. Or, just, default Git::CommandFactory. ;-)

> [...]
> $r = Git::Repo->new(<git_dir>);
> $r->output_pipe('ls_tree', 'HEAD');
> [...]
> $nb = Git::Repo::NonBare->new(<git_dir>[, <working_area>]);
> $nb->output_pipe('ls-files');
> 
> 
> How can it be done with minimal effort, unfortunately I don't know...

Well, this interface is almost identical to what I delineated, except
that I have the extra ->cmd-> step there. But maybe, we could go with
your API and instead have Git::CommandFactory as a base of Git::Repo?
The hierarchy would be

	Git::CommandFactory - provides the cmd_pipe toolkit
		|
	    Git::Repo       - provides repository model
		|
	Git::Repo::NonBare  - additional working-copy-related methods

I think I will post a sample implementation sometime over the weekend.

> > Another thing is clearly describing how error handling is going to work.
> > I have not much against ditching Error.pm, but just saying "die + eval"
> > does not cut it - how about possible sideband data? E.g. the failure
> > mode of Git.pm's command() method includes passing the error'd command
> > output in the exception object. How are we going to handle it? Now, it
> > might be actually okay to say that we _aren't_ going to handle this if
> > it is deemed unuseful, but that needs to be determined too. I don't know
> > off the top of my head.
> 
> I think that the solution might be some output_pipe option on how to
> treat command exit status, command STDERR, and errors when invoking
> command (for example command not found).
> 
> Mentioned http://http://www.perl.com/pub/a/2002/11/14/exception.html
> explains why one might want to use Error.pm.

The arguments against its usage that popped up over the year(s?):

	(i) It is not standard practice in the Perl world

	(ii) It is syntactically ambiguous, c.f. Lea's report about
	the missing semicolon

	(iii) The usage of closures in this way has inherent memory leak
	issues

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