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

From: Jakub Narebski <jnareb@gmail.com>
Date: Jul 13, 2008, 23:28 UTC
Message-ID: <200807140128.44923.jnareb@gmail.com>
In-Reply-To: <1215738665-5153-1-git-send-email-LeWiemann@gmail.com>

On Fri, 11 July 2008 03:11, Lea Wiemann napisał:

> This also adds the Git::Commit and Git::Tag classes, which are used by
> Git::Repo, the Git::Object base class, and the Git::RepoRoot helper
> factory class.

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

No signoff - is it deliberate, or just omission?

Show 16 quoted lines

> ---
> Here's some elaboration on why I didn't use or extend Git.pm.
> 
> Please note before starting a reply to this: This is not an argument;
> I'm just explaining why I implemented it the way I did.  So please
> don't try to argue with me about what I should or should have done.
> I'm not going to refactor Git::Repo to use Git.pm or vice versa; it's
> really a much more non-trivial task than you might think at first
> glance.
> 
> Anyways, the following bullet points are my reasons for not extending
> Git.pm:
> 
> - Git.pm doesn't do what I want: It's designed to provide access to
>   working copies.  Extending it to have more repository-access
>   functions might have resulted in a mess.

I don't quite understand. True, Git->repository(...) has a bit strange [optional] parameters, but you should remember that IIRC it predates possibility of workdir / worktree / working copy separation. That I think is why it supports non-bare repositories (default), and bare repositories, not separating working copy from repository.

Also, from what I understand and remember, Git.pm was created to have
one place, one single implementation for safe and operating system 
independent (which means that it works with ActiveState Perl on 
Windows) "pipe" and "backticks".  Everything else is just convenience.

>   Some long-term thoughts on this issue: I don't think Git.pm's design
>   approach is sensible: It 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).

Show 5 quoted lines

>   Those things should really be separated; e.g. one could write a
>   Git::WC class that *has* a Git::Repo instance (since every working
>   copy has a repository associated with it); so you can use $wc =
>   Git::WC->new to access the working copy, and $wc->repo to access its
>   repository.

Please remember that the opposite relation is also true. Non-bare repository has working copy / worktree associated with it, either implicitly (.git/.. is working copy), or explicitly (core.worktree).

Also even if worktree and repository are separated, there are a few files in non-bare repository which refer to worktree condition, namely $GIT_DIR/HEAD, $GIT_DIR/logs/HEAD and $GIT_DIR/index.

>   Git.pm will obviously have to stay since a few git 
>   commands use it,

The following commands use Git.pm: contrib/examples/git-remote.perl, git-add--interactive.perl (helper script), git-cvsexportcommit, git-svn, git-send-email.

The following commands are written in Perl, but *do not* use Git.pm: git-archimport (which probably should go to contrib, and be replaced by fast-import / fast-export based Bazaar import), git-cvsimport, git-cvsserver, git-relink (rarely used now, I think).

>   but I'd probably let it die a slow death, and (cleanly!) copy
>   functionality to a Git::WC module (and perhaps a Git::Frontend
>   module) in the long run.

I'm not so sure if it is a way to go. Most git commands wants to just invoke other git commands safely, either creating pipeline, or reading whole input; sometimes command output is not important.

Non OO things, like ability to write

  print color('reset') . "\n";

is also important.

Show 8 quoted lines

> - It would have needed refactoring and more features.
> 
>   - For example, the Error module should be removed (and normal die
>     and eval should be used instead).  At some point when I was trying
>     to debug it through gitweb, Git.pm would error out somewhere and I
>     would randomly get either a blank page (usually) or an error page
>     (rarely).  I suspect that this is because of the Error module or
>     some strange interaction with another.

I'm not sure if using Error module and syntactic sugar in the form of try { ... } catch <class> with { ... } was a good idea for (c), frontend error handling. Or if it was a good idea (the explanation in http://www.perl.com/pub/a/2002/11/14/exception.html is compelling) if it was implemented fully, completely, and correctly.

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

>     [...]                                (Besides, Error is not in
>     the Perl distribution, so it would be an unnecessary dependency
>     for gitweb; the only one as far as I can see.)

Git Makefile installs Error module from local copy if one is not available. perl-Git module (which would be unfortunately required to use gitweb; well, that or you could install it locally) has "perl(Error)" as one of dependencies. perl-Error module is a standard module and I think it can be found in any modern Linux distribution; if not, see first sentence in this paragraph.

>   - Also, I needed something like max_exit_code and a custom path to
>     the git binary; adding max_exit_code would have been non-trivial.

What is max_exit_code (and why for example you couldn't just create your 
own derivative of Error)?

>   Now I'm all in favor of re-using existing code, but refactoring
>   Git.pm would have taken *much* longer than simply writing a new
>   module.  I'm working on caching for gitweb, not on implementing the
>   next great Perl API for Git.  (And Git::Repo isn't great, FTR.)

Well, I can understand that.

Besides, having more than one implementation of some new feature was quite common in early stages of feature development. See for example wit and gitweb, git-applymbox and git-am, git-annotate and git-blame.

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

Show 10 quoted lines

>   E.g. I was trying to refactor the 
>   'repository' constructor (to be able to do instantaneous
>   instantiation) and stumbled upon a missing semicolon that rendered
>   the surrounding code syntactically correct but obviously buggy (line
>   214 on master).  Adding a semicolon there seemed to cause other
>   errors, and given that (a) I don't understand what the code does and
>   no test or comment tells me what it should, and (b) it doesn't work
>   (or maybe it half-works?), I lost all my confidence that I could do
>   anything resembling a (behavior-preserving) refactoring on that
>   code.

This is not that the code should not be rewritten... but git-blame (and git log -S, aka pickaxe search) can be created to find who is the author of said code, and ask him via email (probably Cc-ing git mailing list).

>   I might have been able to work with this particular problem, but
>   such a problem (buggy and completely untested code) are indicative
>   that the rest of the code might bear similar surprises.

I think that the pipe and command code might be tested best, as it is collation and condensation of many different "safe pipe" (etc.) code fragments.

Show 8 quoted lines

> - It's overly lengthy, and it's a lot of code for not much
>   functionality.  Git.pm has 1200 LOC, with a large stack of tiny
>   subroutines to handle pipes, and a complicated error handling
>   system.  I can make out ~400 lines of actual Git functionality, plus
>   a 100-lines constructor (all including documentation).  The part of
>   Git::Repo that overlaps with Git.pm (in terms of duplicate
>   functionality) seems to be in the range of 150-200 lines, and it's
>   mostly pipe-handling.  That's not a whole lot.

Pipe handling is IMHO most important part. Well, other parts such as color(...) are important too, but not for gitweb.

Show 17 quoted lines

> - When I decided I didn't want to use Git.pm, it took me 2-3 hours to
>   replicate the functionality in Git.pm that I needed; it would have
>   taken me *much* longer to extend Git.pm to do what I want.  Again,
>   I'm not working on the next great Git Perl API.
> 
> So where do we go with Git.pm and Git::Repo?  I would suggest that
> they both stay.
> 
> Thus we'd have two APIs (both of them obviously incomplete).  If
> there's enough objection to having two competing official APIs, I'll
> be happy to move Git::Repo to the gitweb directory, and make it a
> gitweb-specific thing (though it's not gitweb-specific at all in terms
> of functionality); I really don't care, as my goal is to get caching
> for gitweb working.  Again, if you're bothered by the fact that
> there's duplicate functionality at all, please don't complain, but
> send patches to reconcile the APIs; I will not take care of that
> myself, since it's a very much non-trivial task.

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

> Unrelatedly, should I add copyright notices at the bottom of each perl
> module so they are displayed in the perldoc/man pages?  I'm not
> generally a fan of such notices, since they tend to establish code
> ownership, but if it's desired I can add them.

Well, most manpages have information about who made them... which means who was initial author, usually, and/or who is current maintainer.

[Comments on patch itself in separate email, later]

-- 
Jakub Narebski
Poland