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

From: Lea Wiemann <lewiemann@gmail.com>
Date: Jul 14, 2008, 02:29 UTC
Message-ID: <487ABA01.1050106@gmail.com>
In-Reply-To: <200807140128.44923.jnareb@gmail.com>

Jakub Narebski wrote:

> I think it would be perhaps better to explain relationship and purpose
> of each class in more detail, including Git::Repo.

Noted, will do.

>>   [Git.pm] tries to do (a) WC access, (b) repo access,
>>   and (c) frontend error handling (with sensible error messages).
> 
> I can see (b) and (c), but I have trouble seeing (a).

Well, Git.pm operates on working copies in the constructor (obviously), but also wc_{path,subdir,chdir} and hash_and_insert_object.

>>   every working copy has a repository associated with it
> 
> Please remember that the opposite relation is also true.

True. *nods*

>>   but I'd probably let [Git.pm] die a slow death
> 
> I'm not so sure if it is a way to go.  Most git commands wants to just 
> invoke other git commands safely,

Good point. Perhaps the command functionality of Git.pm and Git::Repo could be extracted into something like Git::Cmd.

> Non OO things, like ability to write  print color('reset') . "\n";
> is also important.

Perhaps, though you might not get around some instantiation to specify the semantics of the color command: Honor color configuration in .gitconfig or .git/config? Honor non-terminal stdout? Honor command line? I suspect that in the end non-OO functions end up being wrappers around OO interfaces that simply specify a set of reasonable defaults.

> I'm not sure if using Error module was a good idea for
> frontend error handling.

As a general rule, I'd try to not use program exceptions as a means to do frontend error handling, unless you're trying hard to keep the frontend minimalist. Even if you don't care about i18n, different frontends have different needs for their error reporting styles. Also, things like failed SHA1-lookups might be an error to one frontend but not an error to another frontend, so you'd have to implement an exception hierarchy to make fine-granular catching possible.

On top of that, this kind of exception handling doesn't seem very much like typical Perl style.

> How would you like to catch errors from frontend in Git::Repo and 
> friends?

Handle them yourself -- Git::Repo doesn't die unless a fatal (i.e. unexpected) error occurs:

($sha1, $type) = $repo->get_sha1('HEAD:/my/file'); if (! defined $sha1 || $type ne 'blob') { ... handle error ... } $contents = $repo->cat_file($sha1); ... work with contents ...

Also note how there's one well-defined (and known) error point: $sha1 being undefined, or the $type being wrong. The $repo methods *cannot* throw errors unless they're fatal, so you can for instance call cat_file and assume that everything goes right.

> What is max_exit_code

It allows you call the git binary without dying if it exits with non-zero status; see the cmd_output documentation for details.

The idea is that a non-zero exit status always indicates an internal (fatal) error, unless you specify that it's OK.

>> - It's buggy and untested.  Neither of these is a problem by itself,
>>   but the combination is deadly.
> 
> Haven't you added t/t9700-perl-git.sh?

Yes (and it alleviated the problem), but I couldn't test the areas where
the untestedness actually hits (e.g. the missing semicolon I mentioned).
 IOW, t9700 is only testing the parts that are working anyway.

> What I worry about is that dependence on Git.pm or Git::Repo would make 
> gitweb installation too hard for some.

If I'm not mistaken you can always drop the perl/Git directory next to gitweb.cgi. (I'll add that to the installation notes.)

Show 5 quoted lines

>> Unrelatedly, should I add copyright notices at the bottom of each perl
>> module so they are displayed in the perldoc/man pages?
> 
> Well, most manpages have information about who made them... which means 
> who was initial author, usually, and/or who is current maintainer.

I don't really care about being credited as the initial author, and I'm honestly not sure if I'll be able to maintain the modules in the long run.

Should I perhaps add some note along the lines of "Direct questions and patches to git@vger"?