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

[Petr Baudis <pasky@suse.cz>]

On Sat, Jul 19, 2008 at 09:07:55PM +0200, Jakub Narebski wrote:
> 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();

I think this is horribly ugly, you would be *much* better keeping the
filehandle within $c if going this way.

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

Maybe blessed filehandle is the simplest way; it seems that in case we
need anything more complex later, it should be possible to replace it
with an IO::Handle subclass, but that feels like overengineering now.

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

I think exported functions should have all a git_ prefix.

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

Yes, but that does not mean Git::Repo must not inherit from
Git::CmdFactory. Think of Git::CmdFactory as maybe a kind of Java-sense
interface to a degree.

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

Yes, that should work fine, with the Git::Cmd subclasses looking into
the singleton.

BTW, I don't like Git::Cmd for the factory interface, since the methods
create Git::Command objects and then the naming does not make any sense.
So I'm going to use class names Git::CmdFactory and Git::Cmd for the
first prototype (since "Command" _is_ too long), unless you have better
but still clear names.

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