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

[Jakub Narebski <jnareb@gmail.com>]

On Fri, 18 July 2008, Petr Baudis wrote:
> 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?

I don't know if it is _better_ approach, but the _alternate_ approach
would be to use:

 	my $c = Git::Command->new(['git', '--git-dir=.', 'cat-file', \
 		'-p', 'bla'], {out=>my $fh, err=>undef})
	... 	
	while (my $line = <$fh>) {
	...
 	$c->close();

And trickery would be to use blessed filehandle, or what?  Or perhaps
extending IO::Handle (but not all like using object methods for I/O
handles)?

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

I forgot that we cannot obsolete / replace old interface.  Nevertheless
it would be nice to be able to use for example

	Git::Cmd->output_pipe('ls-remotes', $URL, '--heads');

but also

	output_pipe('myscript.sh', <arg1>, <arg2>);

See also below for alternative interfaces to Git::Cmd->output_pipe();

> > [...]
> > $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.

Thanks.

I think this is a very good idea.  Although... you mix somewhat here
relationships.  Relationship between Git::CommandFactory (Git::Cmd?)
is a bit different than relationship between Git::Repo and
Git::Repo::NonBare.  Git::Repo::NonBare is a case of Git::Repo which
additionally knows where its working copy (Git::WC?) is, and where
inside working copy we are (if we are inside working copy).  Git::Repo
uses Git::CommandFactory to route calls to git commands, and to
provide default '--git-dir=<repo_path>' argument.


What I'd like to have is a way to easily set in _one_ place where git
binary can be found, even if we are using different repositories, call
git commands not related to git repository.

Should we use

	Git::Cmd->output_pipe('ls-remotes', $URL, '--heads');
or
	output_pipe(GIT, 'ls-remotes', $URL, '--heads');
or
	output_pipe($GIT, 'ls-remotes', $URL, '--heads');
or
	output_pipe($Git::GIT, 'ls-remotes', $URL, '--heads');

we would want to be able to set where git binary is once (and for all),
for example via

	Git::Cmd->set_git('/usr/local/bin/git');

or something like that.

-- 
Jakub Narebski
Poland