Git::Repo API and gitweb caching

Hi everyone,

As follow-ups to this message, I'll be sending three patches for

1) adding the Mechanize tests,
2) adding the Git::Repo API, and (the important part:)
3) making gitweb use the Git::Repo API, and adding caching to gitweb.

The patches apply on master or next: they're viewable live in action, with cache statistics temporarily enabled at the bottom of each page,
at: http://odin3.kernel.org/git-lewiemann/

Patch (3) basically makes two large changes in one patch, but it was pretty hard to separate them during development. I could try to split them up after the fact, but it would take at least an hour or two, since the changes that introduce caching are spread all over the code. I don't think that having separate commits ([a] use Git::Repo API, [b] add caching) brings enough benefit to justify the effort.

There are some other changes in (3) as well, but they fell out as part of the refactoring, so I didn't separate them either -- same thing.

The Mechanize tests succeed before and after patch (3) is applied, so I'm reasonably confident that my refactoring didn't introduce any (major) bugs.

And since you all are curious about the API thing :-), I've added some notes about why I didn't use Git.pm in the patch message of patch (2).

To all reviewers: Since the patches are quite long, I suggest that for anything but major changes that require either discussion or work on my end, you simply send a patch that applies on top of my patches. (Just sending a patch with a bunch of trivial/small changes without comment should be fine; the reasons for simple improvements are normally obvious.)

On my to-do list:

- Benchmarks. I'm planning to time a replay of kernel.org's gitweb logs on the test server, with and without caching. Nothing fancy. (The performance of the test setup on odin3.kernel.org is not representative of gitweb's actual performance under load.)

- Implement support for Last-Modified or ETags, since those basically fall out for free with the current implementation. (This will require mod_perl, since CGI doesn't allow for accessing arbitrary request headers AFAIK.) That will make the site a tad more responsive, I hope, and it will also hugely reduce the load for RSS/Atom requests, which currently make up almost half of all requests to kernel.org's gitweb and get served in full each time (i.e. "200 OK" instead of "304 Not Modified").

- Make gitweb use more parts of the Git::Repo API; in particular, the commit and tag parsing code should be ripped out, and gitweb should use the (much prettier) Git::Commit/Git::Tag API instead. Perhaps some more functions (like ls_tree) can be generalized into the API as well; I went the easy route for now and simply replaced most "open '-|'" calls with $repo->cmd_output calls.

-- Lea

This test uses Test::WWW::Mechanize::CGI to check gitweb's output. It also uses HTML::Lint, XML::Parser, and Archive::Tar (if present, each) to validate the HTML/XML/tgz output, and checks all links on the tested pages if --long-tests is given.

---
Last version is v8, in another thread at
<http://mid.gmane.org/[email protected]>;
major changes since v8:

- Test tag and blobdiff views.

- Test pickaxe and grep search functions.

 Makefile                    |    1 +
 t/t9503-gitweb-Mechanize.sh |  144 ++++++++++
 t/t9503/test.pl             |  607 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 752 insertions(+), 0 deletions(-)
 create mode 100755 t/t9503-gitweb-Mechanize.sh
 create mode 100755 t/t9503/test.pl

index e105d0d..15f210e 100644
@@ -1248,6 +1248,7 @@ GIT-CFLAGS: .FORCE-GIT-CFLAGS
 
 GIT-BUILD-OPTIONS: .FORCE-GIT-BUILD-OPTIONS
 	@echo SHELL_PATH=\''$(SHELL_PATH_SQ)'\' >$@
+	@echo PERL_PATH=\''$(PERL_PATH_SQ)'\' >>$@
 
 ### Detect Tck/Tk interpreter path changes
 ifndef NO_TCLTK

1.5.6.2.456.g63fc0

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

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). 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. Git.pm will obviously have to stay since a few git commands use it, 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.

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

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

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

- It's buggy and untested. Neither of these is a problem by itself, but the combination is deadly. 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.

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.

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

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

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.

So, here's the Git::Repo (and friends) API, plus tests of course: ;-)

 perl/Git/Commit.pm       |  163 +++++++++++++++++++
 perl/Git/Object.pm       |   81 ++++++++++
 perl/Git/Repo.pm         |  397 ++++++++++++++++++++++++++++++++++++++++++++++
 perl/Git/RepoRoot.pm     |  103 ++++++++++++
 perl/Git/Tag.pm          |  155 ++++++++++++++++++
 perl/Makefile            |    1 +
 perl/Makefile.PL         |   10 +-
 t/t9710-perl-git-repo.sh |   49 ++++++
 t/t9710/test.pl          |  165 +++++++++++++++++++
 9 files changed, 1123 insertions(+), 1 deletions(-)
 create mode 100644 perl/Git/Commit.pm
 create mode 100644 perl/Git/Object.pm
 create mode 100644 perl/Git/Repo.pm
 create mode 100644 perl/Git/RepoRoot.pm
 create mode 100644 perl/Git/Tag.pm
 create mode 100755 t/t9710-perl-git-repo.sh
 create mode 100755 t/t9710/test.pl

new file mode 100644
index 0000000..a9bc304
@@ -0,0 +1,163 @@
+=head1 NAME
+
+Git::Commit - Object-oriented interface to Git commit objects.
+
+=cut
+
+use strict;
+use warnings;
+
+
+package Git::Commit;
+
+use base qw(Git::Object);
+
+use constant _MESSAGE => 'M';
+use constant _ENCODING => 'E';
+use constant _TREE => 'T';
+use constant _PARENTS => 'P';
+use constant _AUTHOR => 'A';
+use constant _COMMITTER => 'C';
+
+
+# Keep documentation in one place to save space.
+
+=head1 METHODS
+
+=over
+
+=item $commit = Git::Commit->new($repo, $sha1)
+
+Return a new Git::Commit instance for a commit object with $sha1 in
+repository $repo.
+
+Calls to this method are free, since it does not check whether $sha1
+exists and has the right type.  However, accessing any of the commit
+object's properties will fail if $sha1 is not a valid commit object.
+
+Note that $sha1 must be the SHA1 of a commit object; tag objects are
+not dereferenced.
+
+=item $obj->repo
+
+Return the Git::Repo instance this object was instantiated with.
+
+=item $obj->sha1
+
+Return the SHA1 of this commit object.
+
+=item $commit->tree
+
+Return the tree this commit object refers to.
+
+=item $commit->parents
+
+Return a list of zero or more parent commit objects.  Note that commit
+objects stringify to their respective SHA1s, so you can alternatively
+treat this as a list of SHA1 strings.
+
+=item $commit->authors
+
+Return the author string of this commit object.
+
+=item $commit->committer
+
+Return the committer string of this commit object.
+
+=item $commit->message
+
+Return the undecoded commit message of this commit object.
+
+=item $commit->encoding
+
+Return the encoding header of the commit object.
+
+=back
+
+=cut
+
+
+sub tree {
+	my $self = shift;
+	$self->_load;
+	$self->{_TREE()};
+}
+
+sub parents {
+	my $self = shift;
+	$self->_load;
+	map { ref($self)->new($self->repo, $_) } @{$self->{_PARENTS()}};
+}
+
+sub author {
+	my $self = shift;
+	$self->_load;
+	$self->{_AUTHOR()} or '';
+}
+
+sub committer {
+	my $self = shift;
+	$self->_load;
+	$self->{_COMMITTER()} or '';
+}
+
+sub message {
+	my $self = shift;
+	$self->_load;
+	$self->{_MESSAGE()};
+}
+
+sub encoding {
+	my $self = shift;
+	$self->_load;
+	$self->{_ENCODING()};
+}
+
+# Auxiliary method to load (and parse) the commit object from the
+# repository if it hasn't already been loaded.
+
+sub _load {
+	my($self, $raw_text) = shift;
+	return if defined $self->{_MESSAGE()};  # already loaded
+
+	my $sha1 = $self->sha1;
+	if (!defined $raw_text) {
+		# Retrieve from the repository.
+		(my $type, $raw_text) = $self->repo->cat_file($sha1);
+		die "$sha1 is a $type object (expected a commit object)"
+		    unless $type eq 'commit';
+	}
+
+	(my $header, $self->{_MESSAGE()}) = split "\n\n", $raw_text, 2;
+	# Parse header.
+	for my $line (split "\n", $header) {
+		local $/ = "\n"; # for chomp
+		chomp($line);
+		my($key, $value) = split ' ', $line, 2;
+		if ($key eq 'tree') {
+			$self->{_TREE()} = $value;
+		} elsif ($key eq 'parent') {
+			push @{$self->{_PARENTS()}}, $value;
+		} elsif ($key eq 'author') {
+			$self->{_AUTHOR()} = $value;
+		} elsif ($key eq 'committer') {
+			$self->{_COMMITTER()} = $value;
+		} elsif ($key eq 'encoding') {
+			$self->{_ENCODING()} = $value;
+		} else {
+			die "unrecognized commit header $key";
+		}
+	}
+	undef;
+}
+
+
+=head1 NOTES
+
+You will usually want to call $repo->get_commit($sha1) instead of
+instantiating this class directly; see L<Git::Repo>.
+
+=cut
+
+
+1;

index 5e079ad..a70e78a 100644
@@ -27,6 +27,7 @@ $(makfile): ../GIT-CFLAGS Makefile
 	echo install: >> $@
 	echo '	mkdir -p $(instdir_SQ)' >> $@
 	echo '	$(RM) $(instdir_SQ)/Git.pm; cp Git.pm $(instdir_SQ)' >> $@
+	echo '	mkdir -p $(instdir_SQ)/Git; $(RM) $(instdir_SQ)/Git/*.pm; cp Git/*.pm $(instdir_SQ)/Git' >> $@
 	echo '	$(RM) $(instdir_SQ)/Error.pm; \
 	cp private-Error.pm $(instdir_SQ)/Error.pm' >> $@
 	echo instlibdir: >> $@

index 320253e..6c62160 100644
@@ -8,7 +8,15 @@ instlibdir:
 MAKE_FRAG
 }
 
-my %pm = ('Git.pm' => '$(INST_LIBDIR)/Git.pm');
+# Note that when changing %pm, you'll have to remove pm_to_blib rather
+# than the blib directory to .force the .pm files to be re-installed
+# to blib/lib.
+my %pm = ('Git.pm' => '$(INST_LIBDIR)/Git.pm',
+	  'Git/Repo.pm' => '$(INST_LIBDIR)/Git/Repo.pm',
+	  'Git/RepoRoot.pm' => '$(INST_LIBDIR)/Git/RepoRoot.pm',
+	  'Git/Commit.pm' => '$(INST_LIBDIR)/Git/Commit.pm',
+	  'Git/Tag.pm' => '$(INST_LIBDIR)/Git/Tag.pm',
+	  'Git/Object.pm' => '$(INST_LIBDIR)/Git/Object.pm');
 
 # We come with our own bundled Error.pm. It's not in the set of default
 # Perl modules so install it if it's not available on the system yet.

1.5.6.2.456.g63fc0

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?

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.

I can see (b) and (c), but I have trouble seeing (a).

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.

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

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.

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?

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.

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

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.

Haven't you added t/t9700-perl-git.sh?

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

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

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

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

Noted, will do.

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

True. *nods*

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

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.

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.

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.

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.

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.

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

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

Thanks to Jakub for a nice writeup too. I will try to cover mainly design issues and do not try to dive too much into technical arguments about current Git.pm, since I agree with the general idea of designing a new API; Jakub covered most of that pretty well, I think.

Maybe I sound too mentoring at places; I tried to tone that down as much as possible, and of course what Lea should be working on is her and hers official mentor call. On the other hand, if something general gets into the Git tree, I'd like to make sure it's something we can live happily with for long time, not just a hack tailored for gitweb caching.

I really miss some more detailed writeup on the envisioned design here. And if we are redoing the API in a better way, we better should have some vision.

Most importantly, how is Git::Repo interacting with working copies, and how is it going to interact with them as the new OO model shapes up? You mention briefly Git::WC later, but it is not really clear how the interaction should work.

First, I don't think it's good idea at all to put the pipe-related stuff to Git::Repo - this is botched up API just like the current one. This all is independent from any repository instances, in fact it's perfectly valid to call standalone remote commands (like ls-remote or, actually, clone).

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

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?

Git::Repo considers only bare repositories. Now, since "working copy" by itself has nothing to do with Git and is just an ordinary directory tree, I think Git::WC does not make that much sense, but something like Git::Repo::Nonbare certainly would. This would be a Git::Repo subclass with:

* Different constructor

* Different Git::CommandFactory instance

* Git::Index object aside the existing ones (like Git::Config, Git::RefTree, ...)

* Some kind of wc_root() method to help directory navigation

And that pretty much covers it?

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.

Frankly, I'm not really happy about this attitude. "In my dark corner, I have developed this. Now behold - take it or leave it be; I'm not going to argue."

That said, I do agree that better-structured object API would be beneficial, and I think what you posted here is a good starting point. However, I'm not going to settle with seeing a completely parallel and independent code pop up alongside the existing Git.pm.

Instead, I believe the best course is to gradually translate all the Git.pm functionality to the new OO model, leaving Git.pm as a compatibility wrapper. Now, if you believe this is a non-trivial task, please tell us why.

It should be actually very easy to start with moving all the pipe functionality to Git::Command. This should let us get rid of a lot of technical code, which can be reused in the cleanly rebuilt API. Aside of the object management + error handling, the rest of Git.pm is then actually rather boring and unimportant, especially given Jakub's hopefully upcoming Git::Config.

I will be happy to send in patches factoring out the pipe handling to Git::Command as soon as we agree about the missing design details and the way of error handling. Then, making use of that in Git::Repo should get us a long way ahead.

Wait, I can't make sense out of this paragraph. If Git.pm sucks, we can work on new API. But we better _make_ it great. Or someone else comes by next year and says "oh, but it's buggy and needs refactoring, let's throw it away and redesign it!"

I agree that your main objective is caching for gitweb, but that's not what everything revolves around for the rest of us. If you chose the way of caching within the Git API and introducing the API to gitweb, I think you should spend the effort to deal with the API properly now.

..snip..

..snip..

Now we are quickly getting messy again. This should definitely live in Git::Tree.

Again, the refs should be properly integrated into the object structure.

				Petr "Pasky" Baudis
GNU, n. An animal of South Africa, which in its domesticated state
resembles a horse, a buffalo and a stag. In its wild condition it is
something like a thunderbolt, an earthquake and a cyclone. -- A. Pierce

Once again: if you are adding some large amount of code, you'd better describe "whys" of it.

There are three classes of git commands:

1. standalone i.e. those that doesn't require even repository, like e.g. git-ls-remote, git-clone or git-init (or git wrapper, like for example in "git --version").
2. those that require repository (and should use --git-dir=<path>), like for example git-cat-file, git-log / git-rev-list, git-for-each-ref / git-show-refs, git-diff-tree, git-ls-tree;
3. those that require both repository and working copy (and should probably use both --git-dir=<path> and --work-tree=<path>), like git-commit, git-clean, git-ls-files (the last one can require only index).
3'. those that require both repository and working copy, and whose
behavior depends on where in working copy we (current directory) is[*1*].

*All* git commands should know path to "git" wrapper binary, or where $GIT_EXEC_PATH is.

[*1*] It is (besides pointer to Git::Repo instance) what Git::WC differs from simple directory: the pointer where we are, and wc_path(), wc_subdir(), [wc_cdup(),] and wc_chdir(<SUBDIR>) methods.

Errr... how do you read from such a pipe? <$c> I think wouldn't work, unless you would use some trickery...

You are probably overdoing it.

I think it would be good to have the following interface

Git->output_pipe('ls-remotes', $URL, '--heads'); [...]
$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, it does provide also current subdir pointer, pointer to git repository it is associated with, and a few methods to examine and change both.

Good idea, I think.

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.

[...]

I think the idea is that gitweb caching as it is implemented (data caching) requires some kind of Perl API, and that existing Git.pm didn't cut -- therefore Git::Repo and friends was created. But the focus is gitweb caching, not Perl API (besides Perl API having to be usable).

Jakub Narebski
Poland

I'd make it cmd => ['ls-remotes', $URL, '--heads'] to avoid making the interface inflexible, and perhaps add a git_binary option to specify the binary path. (Not that I'm suggesting something like this should be added right now; but perhaps it'll be needed at some point.)

You read my design notes about error handling at the top of <[email protected]>, and you noticed there's max_exit_code in cmd_output? With the approach to error handling I described, open/exit errors can cause the method to die, and stderr can be ignored.

If you're trying to implement a more sophisticated error handling approach, be warned that you're opening a can of worms. ;) If you're aiming for something like that, it should be kept out of Git::Repo (which has very coherent error handling as-is), and preferably be discussed under a separate subject, since it's way beyond the scope of this patch.

In order to keep this mail within sensible size, I have trimmed some of the bits - basically, the default reply is usually ranging from "Fair enough" to "Great!" :-)

I think this is one problematic point we keep hitting - my opinion is that we _need_ such a wrapper _publically_, because it tends to be actually the main use-case of Git.pm, and that this wrapper should be:

(i) both available standalone for commands like ls-remote

(ii) and available as part of Git::Repo instance, to have the right arguments passed to Git automagically

So you define cmd_output() and seem to argue that this command should not be called directly and is not interesting for the outside. But the experience shows that the pipe interface is actually the _most_ used part of the Git Perl API, and in fact you mentioned that as part of your gitweb migration to Git::Repo, you will temporarily introduce calls to _cmd_output(), the "internal" API. :-) Sure, it's only temporary, but many won't have the luxury to adjust the Git::Repo API to provide all the operations they need, and ultimately they will need to defer to the pipe interface.

As I said, majority of Git API usage is actually the pipe API. So we should figure out how to provide it. I agree that it's not immediately within your scope, but you are introducing new Perl API and this just needs to be embedded somewhere there consistently.

But that's an expensive operation, you need extra Git exec for this, while all the Git commands can do the checks for you, if you give them the chance.

I was doing pretty much this thing in Cogito (initially out of necessity) and it made it ungodly slow for any kind of batch operations.

See above why I think you should reconsider requiring the explicit "resolving" step.

The thing that concerns me about this is that this might show that your approach to error handling is not flexible enough for some real-world usage and this might be a design mistake - is that not so? I didn't look at the code.

Sorry, you're right about repo_dir and possibly git_binary. My main concern was about the command pipe handling itself, but I elaborated on that above already.

Just a note, the thing is that you might want to add some methods for inspecting and mutating the refs, and at the same time this is not a repository-specific concept, but you can get the same structure from git ls-remote call. That's why I think it would make sense to make a separate object out of it. But that's moot point now that the API won't be there yet.

				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

Lea probably would point out that thanks to '--batch-check' option to git-cat-file, and "reuse => 1" option to ->get_bidi_pipe you would need only one extra Git exec... BUT it is one extra Git exec per entire Perl script (one per request, for example, for gitweb). And it wouldn't help batching Perl script commands.

Jakub Narebski
Poland

Even so, I don't like this restriction simply since it makes the usage more complicated - in my commandline tool, I will be forced to write

	my $csha1 = $repo->get_sha1($ARGV[0]);
	$csha1 or die "a random inconsistent error message";
	$repo->get_commit($csha1)

instead of

	$repo->get_commit($ARGV[0]) # die if unresolved

and I don't understand why. Me and 80% of the scripts don't *care* about some more graceful error handling, and if gitweb and the other 20% (or less) do, *they* should do the extra work, not me.

				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

This made me curious which parts of Git.pm, the current Git Perl API, are used most.

Here is some statistics on the usage of Git Perl API among built-in and contrib commands.

sources: git-cvsexportcommit, git-send-email, git-svn, and helper script git-add--interactive. There are Perl scripts which do not use Git.pm: git-archimport (which I think should be obsoleted or moved to contrib), git-cvsimport, git-cvsserver, git-relink. This means that half of Perl scripts use Git Perl API.

The situation is worse for scripts in 'contrib/'. From those, only contrib/examples/git-remote.perl uses Git.pm; neither blameview, continuous, git-import and import-tars in fast-import, setgitperms and update-paranoid in hooks, stats, nor other Perl scripts in examples (git-rerere, git-svnimport) include "use Git".

Below there are stats on how different commands from Git.pm are used in mentioned Perl scripts:

1. git-add--interactive.perl uses Git.pm mainly to access color
configuration: 2 ->get_colorbool, 8 ->get_color, 1 ->config. It also uses once ->repository() constuctor, and once ->repo_path().

This means that future Git::Config _has_ to have support for color or colorbool valued configuration values.

2. git-cvsexportcommit.perl uses (besides ->repository() constructor) only once ->config; so I guess that current interface wrapping git-config should stay, because parsing whole config for such situation would be overkill.

What is strange that git-cvsexportcommit.perl defines its own safe_pipe_capture and xargs_safe_pipe_capture (!), and uses them, open pipeline, backticks, and system() to call git commands.

3. git-send-email.perl uses 5 config, 2 config_bool, 2 ident_person (for author and for committer), 1 version, and of course once ->repository() constructor.

Here we can see how to work around current API to: it uses Git::config(@repo, "sendemail.identity") form, where
my $repo = eval { Git->repository() };
my @repo = $repo ? ($repo) : ();
to make it work both with git repository (using repo config), and outside/without git repository, using only user and system git config.

4. git-svn.perl (which is if I checked correctly third largest scripted git command, after gitk-gui/gitk-wish and gitweb/gitweb.perl) uses:

3 x repository
28 x command_oneline
19 x command_noisy
18 x command
14 x command_output_pipe
1 x command_input_pipe
1 x git_try_cmd (and many "eval { command()/command_oneline() }") 2 x cat_blob
1 x hash_and_insert_object
1 x get_colorbool

(The above are all if I have not made mistake when counting commands)

Side note: git-svn is command which would get most out of Git::Config, as it currently uses combination of "git config -l" and "git config --get" (I guess that the code in question predates machine-parseable "git config -l -z").

Note that all those statictics doesn't count how many times some Git.pm method was called, only how many times it occurs in the code.

Jakub Narebski
Poland

Note that I referred both to scripts that are part of Git and
(actually more) to various third-party scripts I wrote externally.

Thanks!

I have actually once converted blameview to use Git locally, but in
the end never got around to submit it; I wonder if I have the source still around somewhere. Not so important, I guess.

I don't understand. Parsing whole config happen will either happen in
git-config or in Git::Config, and the performance difference is so tiny that it is not really worth the API complexity, I believe.

With the envisioned model, it could use $git which would be either
a reference to a Git::Standalone singleton or Git::Repo instance.

				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

So that the code would look like the following, instead:

  my $git = new Git::Cmd; # or Git::Standalone, or Git::CommandFactory
  my $repo = eval { Git->repository() };
  $git = $repo if $repo;

and later use

  $git->config('sendemail.identity');

By the way, git-svn can use command(...) instead of $repo->command(...) because it sets $ENV{'GIT_DIR'} if it is unset... but I don't see where Git.pm inserts 'git' to commands list...

Jakub Narebski
Poland

In _execv_git_cmd(), or did I misunderstand your question?

(I think that level of indirection is probably residuum of the XS interface.)

Petr "Pasky" Baudis

Hi,

You call that "worse"? Pasky tried to convert all Perl scripts at once IIRC, and my numerous problems just _making_ the Git scripts led me to rewrite a few Perl scripts in C, so I could safely exclude the Perl scripts from my personal fork.

I guess that it was this experience which prevented more of the old scripts from being converted.

But your mention of git-add--interactive actually brings up my pet-peeve:
this script is the only Perl script needed for common operations, i.e. the only reason msysGit has to ship bloated with Perl.

Ciao,
Dscho

Hi,

I don't remember any concrete report of such problems ever reaching
me; exactly what trouble are you hitting with the Perl scripts using Git.pm? I will be glad to try to fix it.

_Many_ people seem to be using git-svn, whether we like it or not. ;-)
Also, isn't git-send-mail rather commonly used? (I wouldn't know, me using stg mail.)

				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

Hi,

They reached you:

http://article.gmane.org/gmane.comp.version-control.git/23153

http://thread.gmane.org/gmane.comp.version-control.git/22764/focus=22778

Yes, those are very old mails, but they _do_ explain why the old Perl scripts avoided Git.pm.

Well, they will just stop using it on msysGit, since nobody fixes git-svn on msysGit.

send-mail does not work on Windows. See
http://code.google.com/p/msysgit/issues/detail?id=27

Yes, you read correctly. The issue exists since almost the birth of msysGit. Torgil seems to have stopped working on it altogether. Two other people have "starred" the issue, wanting to be notified of fixes but not wanting to work on them.

To me, it seems that either the Windows folk is a lazy bunch of bums, or they just do not care enough. Or both.

Note: I want to express very loudly here that I do not count Hannes Sixt as Windows folk. Even if he seems to like cmd.exe for some perverse reason, it is undisputable that his effort, and his effort alone, brought the MinGW port to where it is now.

To sum it up, I think we can safely remove Perl from the msysGit installer once add--interactive is a builtin. The download will be substantially smaller, and Perl on msysGit was never exactly a speed demon, so it's probably a good change from several angles.

Ciao,
Dscho

Hi,

running Git in-place without correctly setting the prefix is not
supported anyway; git wrapper will still be using non-builtin commands from the prefix location instead of your fresh build.

It seems I fixed this right away?

Petr "Pasky" Baudis

Hi,

There were none. Anyway, because Git makes it _really_ hard to run in-place, I gave in. I run Git from $HOME/bin now.

That is not what I remember. I remember that I had the next issue right away. I remember that there were problems with ActiveState Perl+Cygwin. And I remember that the Git.xs was not merged in the end.

Anyway, this whole discussion is moot, methinks.

Let's concentrate back on the real issue: merging the two as-of-now incompatible Perl modules. It really would be nice to have one pony with one saddle in the end, so that the use of Lea's GSoC project is not limited to gitweb with enabled caching.

Ciao,
Dscho

Well, sure, I happen to not be convinced, but it *may* be useful. The point I'm trying to make is that it's not part of what I'm writing here.

Yup, and I'm actually fine with that. (I'll probably alias _cmd_output to cmd_output in gitweb, just to make it clear that it is, for the purpose of gitweb, a *supported* mode of operation.) If the Git::Repo::_cmd_output API goes away, you'll have to insert a few lines of code in gitweb, but that's it. Really, no big deal.

Also, gitweb isn't using cmd_output because it needs a pipe interface, but because it needs a caching layer in between -- most applications would do just fine with open calls.

Sure, but pleeeease not as part of this patch series! :-) Look, our conversation is going something like this:

Lea: Here's a Perl API that fell out of my gitweb development for free.
Petr: I want a pony with the API!
Lea: But I don't have a pony. Can we please just go with the Perl API as a start, even if I don't supply ponies with it?

(Cf. the very cute <http://c2.com/cgi/wiki?IwantaPony>.)

For the gazillionth time in this thread, there is no extra exec. It's a write to a bidirectional cat-file --batch-check pipe. It's not expensive. Really. ;-)

I don't think so; the error handling is fine. Given that I want fine-granular error reporting for gitweb, there *needs* to be a git_get_sha1_or_die function; you can't move that into the API.

-- Lea

One of the points of the API is to abstract these out.

I'm fine with that, as long as the version that enters into master will have a pony so that we stay with a single pony within the codebase in the end, not two ponies with differently shaped saddles.

But as I said, I'm going to work on that.

But the API is still obnoxiously elaborate, as I complained in another mail.

Wait, this doesn't compute here. The error handling is fine, but it is actually not fine for gitweb. Can't we make it fine for everyone?

				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

Hi,

Johannes: *mumbles* no ponies? *mumbles further* Them cowboy coders keep the ponies for themselves, eh? *mumbles and gets something to eat*

Ciao,
Dscho

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?

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

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.

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

What I would want from the solution is an easy way to set up path to git binary for _all_ git command calls, and for invoking command which does not need git repository to run (like git-ls-remote).

Jakub Narebski
Poland

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

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

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

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

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 think exported functions should have all a git_ prefix.

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.

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

I agree. Nevertheless one inheritance is "interface", second is "subclass"... but I don't think that matters at all. Just being nitpicky.

Jakub Narebski
Poland

Yup, I agree (and you don't sound too mentoring ^^). Thanks for the review and feedback.

Git::Repo is not interacting with working copies at all right now. Is there anything you think should be considered for its design?

Here's a write-up about the design (I'll probably move this into Git::Repo's man page):

----------
Git::Repo aims to provide low-level access to Git repositories. For instance, you can resolve object names (like "HEAD~2") to SHA1s, and inspect objects. It does not attempt to be a wrapper around the git plumbing or porcelain commands.

Error handling is simple: On a consistent repository, the Perl interface will never die. You can use the get_sha1 method to resolve arbitrary object names or check the existence of SHA1 hashes; get_sha1 will return undef if the object does not exist in the repository. Any SHA1 that is returned by get_sha1 can be safely passed to the other Git::Repo methods.
----------

Well, they're more like helper methods. Since they don't fit into the design goals of the Git::Repo API at all, I'd suggest we just underscore-prefix them and take them out of the man page. (The only reason why I hadn't done this is that gitweb uses $repo->cmd_output extensively, so it'd end up with a lot of underscore calls. But I suppose we can either alias _cmd_output to cmd_output in gitweb's CachedRepo subclass, or live with $repo->_cmd_output calls.) Does underscore-prefixing sound good to you?

If someone wants to come up with a consistent nice interface for calling git commands, sure. I wasn't actually trying to do that.

Yes, I think so. ;-) All we're talking about here is a wrapper around "open '-|'" calls (plus some workarounds for Windows I suppose).

I don't have much of a notion of a 'command' as an object in my head; your (snipped) example makes it look like you're trying to create a IO::Handle-compatible interface, which I think is way too much effort (and error-prone) for simple pipes. Also, a CommandFactory class just to catenate lists together seems like overkill to me.

Something like a command interface *may* make sense if it's tied to repositories or working copies, in which case it could automatically set --git-dir or --work-tree, but it's beyond the scope of what I'm trying to create here, and I don't think it's even overly useful.

Well, as I said, the fact that there are untested parts in Git.pm doesn't exactly make it trivial to refactor. Also, only very few parts can be cleanly moved to Git::Repo. In "grep '^sub [^_]' Git.pm" I find only cat_blob and perhaps hash_object to be eligible to be moved (though hash_object should probably live in a working-copy/non-bare-repo class, with an optional insert => 1 parameter). And even cat_blob is non-trivial to move unless you want the whole blob to be read into memory.

That's a lot of non-trivialness for very little gain. I doubt I'd even have enough time till the end of GSoC (minus vacation) to do this.

Creating a new (Git::Command) API is very much non-trivial, apart from the fact that I'm not convinced that we need Git::Command, and that a clean command interface neither falls out of Git.pm nor Git::Repo.

Sorry, I wasn't clear with my parenthesed remark: I actually think that Git::Repo is pretty good in terms of code and interface quality. It's just not *complete*, even in its limited scope, and I'm not attempting to make it complete.

I do think that someone who wants to extend Git::Repo (like Jakub with Git::Config) won't have much trouble doing so with the existing design.

Embarrassingly premature optimization here. ^_^ I'll fix it.

*ponders* Every commit needs a repo and a SHA1, so those will never get optional. We can always add hash-based options after the two mandatory arguments, but I don't even see any such possible options at the moment.
(And if I turn out to be completely wrong, we can even move to a
hash-only argument list by checking the type of the first parameter.) Really, I wouldn't worry.

No, you should never pass in an invalid SHA1 in the first place. The above piece of documentation is just a warning that bugs will show up delayed. IOW, this is not the right place to have your error handling.

If you're getting a SHA1 through the user-interface, check its existence with get_sha1 before passing it to the constructor.

Because the SHA1 might resolve to an object of the wrong type, which means you have to do error handling in Git::Object objects; that's the wrong place.

If tag-resolving is really needed, we can add an optional $type parameter to get_sha1, which will cause get_sha1 to resolve the object until a $type object is found, or return undef if the object is or resolves to an object of the wrong type.

I have resolving code in gitweb's git_get_sha1_or_die (which I didn't implement in Git::Repo since it uses some customized error reporting). The resolving code could conceivably be extracted and moved to get_sha1.
I think there are a few things to ponder and maybe discuss, so I'd do
that in a separate patch (if I get around it before the end of the project).

Fixed.

I've replaced it with: "Objects are loaded lazily, and hence instantiation is free. Git::Object instances stringify to their SHA1s."

Done (and removed stringify).

Done, and removed them from @EXPORT_OK.

Sure, it can be made non-mandatory if it's needed, but there are so many possibilities for the exact time and place at which the repo directory should be resolved using rev-parse (if at all) that I'd rather leave this to the person who has an actual use-case for it. I'm not a fan of designing APIs before they are needed.

Which of those methods are you referring to? I think $repo->version might reasonably be removed (and the code re-added to gitweb); I'll do so unless you object. _get_git_cmd is already underscored, and repo_dir and git_binary only access attributes passed in through the constructor, so I think those three should stay.

Yup; I like get_object (I think I was planning to rename it and then didn't remember doing so before sending off the patch). Will rename it.

Yup, that's true. I'll move it into gitweb until we have Git::Tree (with a comment that it can be moved to Git::Tree once it exists).

Really? I think it's generally fine for get_refs to exist and to live in Git::Repo.

Its return value (currently an an arrayref of [$sha1, $object_type, $ref_name] arrayrefs) might need improvement though, and I find the $pattern parameter pretty suspect (in that it smells like a for-each-ref wrapper). Since get_refs is unused at the moment (gitweb ended up needing the slightly different show-ref), I'll remove it for now. (Same thing about me not being a fan of premature API design applies.)

I keep patches of everything I remove so other people will be able to use them as starting points; I'll probably post them as "FYI"-patches to the list at the end of my project, to preserve them for posterity.

This probably is something the person who mentors you as a GSoC student should tell you, and shouldn't come from me, but I am going to mention it anyway.

I thought GSoC program was about giving participants a chance to acquire skills to work better with open source community, and I firmly believe that a major component of that skillset is about working with people, probably even more so than about working with code. Of course you need have a certain basic coding skills and disciplines, but it is clear to everybody that you are beyond that threashold already. I would have preferred to see the first paragraph of yours stated more diplomaticly; it sounds rather unilateral ultimatum to me.

Having said that, I agree with your reasoning. People who care enough can help refactoring Git.pm to build on top of Git::Repo (this would include adding missing Git::WC and possibly others), but I agree that would largely be outside of the scope of gitweb caching.

This patch was not signed-off, but I assume it was simply forgotten and not deliberate?

No, thanks for telling me. I'm actually keeping John quite busy with the server side on kernel.org, and wouldn't currently expect him to look at the patches I'm sending.

Apologies; I was trying to hold off developers who'd see duplicate functionality and run off at me screaming "unacceptable! impure!!one". I should've phrased it more carefully.

Yup. In the unlikely case it gets applied as-is, feel free to add it.

(I'll reply to Jakub's email shortly, unless I fall asleep in the meantime. ^^)

[Here is promised patch review]

Does splitting into many small files is really necessary?

Good that you provided test suite.

  $commit = $repo->commit($sha1);

	$output = $repo->cmd_output('show', 'HEAD');

	$output = $repo->cmd_output(['show', 'HEAD'],
	                            max_exit_code=>undef);

                                                                   ^

Again.

Tag object format is meant to be extensible; for example (unfortunately abandoned) git-notes implementation added 'keyword' (IIRC) header to tag object format.

Jakub Narebski
Poland

Thanks. A new patch series will follow today (hopefully).

For brevity, I'm incorporating all your suggested changes unless noted otherwise.

I think it's better. People shouldn't get an error message if they write "use Git::Commit". Also we cannot split later if we don't split now since people would write "use Git::Repo" and then access Git::Commit; subsequently splitting the API would (might?) cause breakage, I believe.

;-)

Unknown headers are now ignored.

Yeah, but this part is only a bunch of trivial accessor methods. If the module grows and the documentation needs to be split, it can be done later. No need to be purist here. ;-) Also ...

Several of the methods actually only exist in the Git::Object base class. I still documented them in the Commit and Tag modules since having to look up methods in base class documentation can be a tad annoying, especially if the base class is never used by users of the API.

You understand correctly; I'd call it "lazy loading" though. I've added that term for clarity.

I think the constructor shouldn't be internal (= underscore-prefixed, you mean?), since the Commit/Tag APIs are usable on their own.

No, they don't. (Clarified that in the documentation.)

Array. I've replaced "list" with "array" in case it helps clarity.

If we add a Git::Tree API, the Git::Tree objects will stringify to their SHA1s, so we shouldn't have compatibility issues. I've changed the documentation of $commit->tree to this:

"Return an object that stringifies to the SHA1 of the tree that this commit object refers to. (Currently this returns an actual string, but don't rely on it.)"

Yes, but we don't actually care about those effective parents for the purpose of the Git::Commit API. IOW, the effective parent should be managed by the code that created a list of revisions, not by the Git::Commit API.

Yup; that's why I wrote "{author,committer} *string*". ;)

Yes, just raw data. Decoding is too tricky (i.e. not guaranteed to work) to just add a simple method to the API; IOW, it needs error handling and perhaps fallback encodings.

Yes, but the API doesn't use any of those commands internally, if that's what you're worried about.

No. (Otherwise I'd have written that ;-).)

Yes, I'd believe so. I basically wanted to make sure that those methods always return a string; do you think that this is a bad idea?

$repo->cat_file (now renamed to get_object) actually doesn't fork but uses a pipe (cat-file --batch); I don't think it should be a performance issue.

Because we happen to get the raw text in a single string from the Repo API. (It shouldn't be a performance/memory issue for Commit or Tag objects at all. ;-))

Yes, changed, but ...

... let's not complicate the API unnecessarily. If a new header pops up we can immediately add it to the Commit/Tag API.

I don't really care, and it's too much work to come up with a test case for this. ;-) If the repository is borked to the point of invalid header names, it's fine for Git::Commit to behave undefinedly.

Intuition. ;-) I think I'd read $repo->commit as "the commit of the repository", akin to $commit->tree, which doesn't make sense here.

It could represent any object, though I don't see a need for Git::Blob right now (though it's possible that it's needed later).

Why? I don't think it's necessary.

It signifies that this module won't run with Perl <5.6.2. I've had to bump it to 5.008 (Perl 5.8); more about that in the message announcing the next version of the patch series.

I wouldn't use discovery magic here, at least for now, since it's non-trivial to get it right (and it interacts with possible future extensions of the API, like Git::WC). Such a feature can be implemented if/when it's needed.

I don't think something Git::Cmd is a good idea (as I pointed out in my reply to Petr, <[email protected]>), or at least it shouldn't be implemented as part of this patch series. This method is really just supposed to return an argument for exec*p, nothing more.

No. It's not helpful for error handling (which should happen in the caller), and it's not helpful for bug detection (since it will die on the first access to the repo anyway), but it causes performance penalty that can be significant for programs like gitweb.

I find repo_dir somewhat clearer (and I don't like having more than one name per method). We're not trying to mimic or wrap standard git commands here, anyway.

Yup; it's been deleted anyway.

As I wrote in my reply to Petr, Git::Repo is not trying to be a wrapper around git binaries, so this method really shouldn't be part of the official API -- it's just auxiliary; I'll underscore-prefix it.

It's not part of the API anyway, so no need for complicated calling conventions, IMO.

Apart from the fact that I don't do cargo-cult programming? ;-) Git.pm forks, whereas Git::Repo uses open, '-|', so it's actually different (and it's not possible to copy the code).

Why, what should it do? This just opens a pipe, nothing more. No need for introducing complicated concepts.

You found it below. :-) (If you had snipped this, I wouldn't have spent time finding and pasting an example. ;-))

I wouldn't -- see my blurb about error handling at the top of my reply to Petr (<[email protected]>). You're not supposed to pass anything that you didn't get from get_sha1 into Git::Commit or Git::Tag constructors, or your error handling is invariably broken.

Yes to both questions. :-)

Yes, but only for generic command calls, and with a somewhat unpleasant (cache-specific) interface. It'd need a bit of work for the API.

No. ;-) Doing fine without those.

This one has been removed as well since it would belong into Git::Tree.

Thanks. Since get_path isn't in the Git::Repo API anymore and gitweb's get_path subroutine didn't handle quoted filenames even before my patches, I'll only mark it as TODO for now. ;-)

This has been removed as well, since it's not used and the interface would need work.

Feel free to add it. ;-) (It might take some work to come up with a decent interface for that method.)

No. If it's needed it could reasonably be extracted from gitweb though (I think).

Yeah. At some point. ;-)

Sure, for the message.

No, since 'validate' sounds like it would have to do error handling.

If you mean that this should check if the object exists (and has the advertised type), the user of the API should test for "defined $tag->repo->get_sha1($tag->object)" or somesuch and do error handling themselves.

-- Lea

I was wondering (but forgot to ask) why for some files you had continuous block of documentation, and in others documentation interspersed with documented code.

I think that providing documentation for all methods for "front-end" class, even for those that are implemented in [abstract] base class, is a very good idea. Better to have everything in one place, or at least in "porcelain" documentation.

True. Or to be more exact in a Git::Revlist (or somesuch) container class.

We would probably want _in the future_ to return some object wrapper, which stringifies to value of author and committer headers (to author and committer string), but allows to extract (and format) parts of it, for example

$commit->author->name
$commit->author->email
$commit->author->date(format => RFC2822)

or perhaps

$commit->author{'email'}

I'd rather then have _git_ convert it to UTF=8 for us (using --encoding=<encoding> option to git-log/git-rev-list), see below on using git-log.

I'm not worried about; I think using git-log would be better (see also below)

It don't know if it is good or bad idea, but you should have mentioned it in the documentation.

[HERE IS MAIN PART OF THIS RESPONSE]

It is (much) better than forking git-cat-file for each commit shown on the list; nevertheless I think that it would be better to use git-log to generate list (or Git::Revlist) of Git::Commit objects. It is one fork less, but what more important you don't have to access repository twice for the very same objects.

Let me elaborate: if I understand correctly for log-like views you propose to first run simple git-rev-list with appropriate starting point and limiters (--skip, --max-count, -- <pathname>), perhaps using '--parents' option to get parents in simplified/rewritten history, which would traverse history getting commit objects, but outputting only fragments of info, then feed list of revisions (perhaps via cache, i.e. excluding objects which are in cache) to 'git cat-file --batch' open two-directional pipeline.

What I propose instead is to provide alternate method to fully instantiate Git::Commit object (in addition to ->_load), which would fill fields by parsing git-log / git-rev-list --headers output (what gitweb currently does in parse_commits).

On the other hand... "git cat-file --batch" should have commits to be accessed in filesystem cache, which means in memory; but it is possible that they wouldn't be in cache because of I/O pressure (git-rev-list and git-cat-file are separate processes). And checking if object is in cache can be simpler... if less effective. If you generate Git::Commit objects via parsing git-log / git-rev-list output, then you can limit history further by excluding starting points from cache.

[END OF MAIN PART]

I was not asking what this mean, but why do you need to set up lower bound on Perl version. What feature pre 5.6.2 Perl lacks...

Requiring 5.8 is bad. What feature pre 5.8 Perl lacks, that you absolutely cannot go without it? There will be complaints.

I think that _not using_ Git::Cmd (or somesuch) API results in botched, horrible API like (in the 3/3 patch in this series):

  our $git_version = $repo_root->repo(directory => 'dummy')->version;

Aaaaaargh! My eyes!

  out $git_version = Git->version;

(Unless it is not needed any longer, or not used any longer; if it is so, then perhaps implementing Git::Cmd as generic wrapper around git commands, hiding for example ActivePerl hack, could be left for later).

Just a question: was this reply only to him, or to all?

I think that $repo->cmd_output(%opts) is a great shortcut for invoking Git->cmd_output with '--git-dir=<repo>' added automatically. So it should be left, but perhaps under different implementation.

Actually magic open, '-|' _does_ forks, only implicitely. So Git.pm does generate the same or almost the same code, but it work (around) with ActiveState Perl.

O.K.

I can understand this simpler, although less than optimal, and geared mainly towards gitweb needs.

Errr... "yes" to first question means that 'reuse' option makes sense _only_ for get_bidi_pipe? If so, why it is present in other commands?

See above.

Why do you _need_ name_rev, if you are not to include git-describe equivalent.

See above, in comment about Git::Commit.

I meant here equivalent of "git tag -v <tag>"

Jakub Narebski
Poland

Yup, good idea. They'll even stay strings, they'll just be blessed.

Yeah, I guess the API should actually decode it. You wouldn't want to have the message in UTF-8 but in Unicode (I suggest you read man perlunitut if you haven't done so). We cannot have git do the decoding, since (apart from the fact that it doesn't smell right) it isn't guaranteed to emit valid UTF-8 (thanks Junio for the pointer):

Lea Wiemann: Does anyone know off the top of their heads how git handles
character decoding errors in commands like git log? [...]
Junio 'gitster' Hamano: silently punt and show the original unmolested. Junio 'gitster' Hamano: cf. pretty.c:pretty_print_commit()

So we're not guaranteed to be able to, in turn, decode git's output into actual characters since it might just be byte soup.

Hence, how about this fallback strategy:

1. Decode according to the encoding header.
2. Decode as UTF-8 (passing through byte soup is often equivalent to decoding UTF-8 since many terminals use UTF-8, and trying UTF-8 is reasonably safe).
3. Decode as Latin1.

(Not that the fallbacks will matter a lot in practice, I think.)

You're confused; it's not one fork less, it's a write to a pipe less. (Pleeeease look at the code before you write something. It's there, in this very thread.) And I don't believe the "access the repository twice" thing is anywhere near an actual issue. To summarize, you're asking me to (a) write code and (presumably) (b) add something to the interface of a public API, based on some (most probably faulty) assumptions about performance? You should really read
<http://c2.com/cgi/wiki?PrematureOptimization>.

Yup, it's an option, though currently it's a single cached call to git log (or git rev-list).

Yes, but this would need a method in the API, it's not an optimization that falls out for free. Cluttering an API for some obscure (= very doubtful) optimization? Bad Idea.(tm)

No. Page cache turnover time is at least around 10 seconds (and that's under fairly artificial conditions), definitely not in the millisecond range.

It isn't used any longer -- I really suggest you read the whole thread before replying. ;-)

To all, otherwise I wouldn't have Cc'ed the list.

FTR, yes it is simpler, but no, it is not really geared toward gitweb needs, and it's definitely not "less than optimal" in the sense of being worse than the exception-based error handling Git.pm does. Trust on me on this one. ;-)

Yes, and no, it isn't present in other commands. (Hey, could you please check the code before posting? Really.)

I needed it for gitweb. As I said, I'm not trying to create a complete API. A describe_rev (or so) method can be added later, if and when it's needed. (As I said, I don't think writing APIs without at least one use case is a good idea anyway.)

I guess it could be added. As with describe_rev, I won't add it myself, in particular not as part of this patch series.

-- Lea

You mean perluniintro(1) here, isn't it?

Besides if decoding is done in Perl API, we can convert it simply to Perl internal form (which, IIUC, in modern Perl is UTF-8 and marked as such).

Well, if that is the case then Perl API has to do conversion, that is the only sensible way.

Code is there, in gitweb, in parse_commits subroutine, or rather in parse_commit_text subroutine.

[cut]

But I can agree that possible (and possibly minuscule) performance improvement is not worth introducing new API and complicating (I think) gitweb code.

O.K. Still I think that putting cmd_output and other in Git::Repo is not a good API. I'd rather route calling git commands via Git or Git::Cmd object (but Git::Repo would have Git/Git::Cmd object which automatically adds '--git-dir=<path>', and possibly also '--work-dir=<path>').

By the way, would you prefer if I commented on 3/3 patch as it is now, taking into account what I remember from discussion on this and 2/3 patch (latter only as relevant), or would you rather I wait for next round (next version) of patches?

[...]

Errr... I guess I misspoke. I should not say 'geared toward gitweb needs', as perhaps it is 'created according [at least somewhat] to what gitweb would need'.

Jakub Narebski
Poland

FWIW, Perl strings actually contain non-encoded Unicode code points. IOW, they're not byte strings.

Yup; that's why I'm underscore-prefixing it (and taking it out of the man page) for the next version of this patch.

I suggest you wait for the next version of the patch series, which I'll post in a few hours. There are quite a few changes; I'll list them in the parent message for the series.

-- Lea

Gitweb now uses the Git::Repo API; this change is behavior-preserving, except for slightly more aggressive error handling; see below.

This patch also adds an optional caching layer for caching repository data in memory and (for larger cacheable items, like blobs, snapshots, or diffs) on disk.

Other minor changes:

- Gitweb would previously accept invalid input and either (a) display nothing, (b) display an obscure error message, or (c) proceed as normal since the parameter happens to be unused in the particular code path used. This has changed in that gitweb will check for parameter correctness more aggressively, and display meaningful error messages. This change is only relevant if you manually edit gitweb's CGI parameters, since gitweb only generates valid links.

- Empty projects:

- Only display summary link for empty projects in project list to avoid broken links (yielding 404).

- Slim down summary page for empty projects to avoid some broken links and unnecessary vertical space.

- Sort empty projects at the bottom of the project list when sorting by last change.

- Add test for empty projects to t9503 (the Mechanize test), now that there no broken links anymore.

- For HTML pages, remove the "Expires" HTTP response header, and add "Cache-Control: no-cache" instead. This is because pages can contain dynamic content (like the subject of the latest commit), so the Expires headers would be wrong.

This makes gitweb's responsiveness slightly worse, but it will get much better once If-Last-Modified is implemented. It's better to be correct than to be convenient here, since having to press the reload button makes for lousy user experience (IOW, users should be able to always trust gitweb's output).

Raw diffs and blobs still get the Expires header, where appropriate.

- Add a $page_info option to display cache stats at the bottom of each page; the option is named generically to allow for adding non-cache page info there at some point (timings perhaps?).

---
It's all documented of course :-), but for the impatient here's a snippet for gitweb_config.perl to activate caching:

use Cache::Memcached;
$cache = Cache::Memcached->new( { servers => ['localhost:11211'], compress_threshold => 1000 } ); $large_cache_root = '/home/lewiemann/gitweb-cache';
$large_cache_case_sensitive = 1;
# Invalidate cache on changes to gitweb without version number bump;

# useful for development.
$cache_key = (stat '/home/lewiemann/gitweb')[9] . (stat '/home/lewiemann/gitweb/gitweb.cgi')[9];
# Display detailed cache info at the bottom of each page.
$page_info = 2;

A live demo is here: http://odin3.kernel.org/git-lewiemann/

 Makefile                               |   27 +-
 gitweb/README                          |   14 +
 gitweb/gitweb.css                      |   16 +
 gitweb/gitweb.perl                     | 1415 +++++++++++++++++++-------------
 t/t9500-gitweb-standalone-no-errors.sh |    4 +-
 t/t9503-gitweb-Mechanize.sh            |   10 +
 t/t9503/test.pl                        |    5 +
 7 files changed, 911 insertions(+), 580 deletions(-)

index 15f210e..a65aa4d 100644
@@ -1099,25 +1099,28 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
 perl/perl.mak: GIT-CFLAGS perl/Makefile perl/Makefile.PL
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
 
+PERL_USE_LIB_REWRITE = \
+	-e '1{' \
+	-e '	s|\#!.*perl|\#!$(PERL_PATH_SQ)|' \
+	-e '	h' \
+	-e '	s=.*=use lib (split(/:/, \$$ENV{GITPERLLIB} || \"@@INSTLIBDIR@@\"));=' \
+	-e '	H' \
+	-e '	x' \
+	-e '}' \
+	-e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
+	-e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g'
+
 $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
-	sed -e '1{' \
-	    -e '	s|#!.*perl|#!$(PERL_PATH_SQ)|' \
-	    -e '	h' \
-	    -e '	s=.*=use lib (split(/:/, $$ENV{GITPERLLIB} || "@@INSTLIBDIR@@"));=' \
-	    -e '	H' \
-	    -e '	x' \
-	    -e '}' \
-	    -e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
-	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
-	    [email protected] >$@+ && \
+	sed $(PERL_USE_LIB_REWRITE) [email protected] >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
 
 gitweb/gitweb.cgi: gitweb/gitweb.perl
 	$(QUIET_GEN)$(RM) $@ $@+ && \
-	sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \
+	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
+	sed $(PERL_USE_LIB_REWRITE) \
 	    -e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \
 	    -e 's|++GIT_BINDIR++|$(bindir)|g' \
 	    -e 's|++GITWEB_CONFIG++|$(GITWEB_CONFIG)|g' \

@@ -1125,7 +1128,7 @@ gitweb/gitweb.cgi: gitweb/gitweb.perl
 	    -e 's|++GITWEB_HOME_LINK_STR++|$(GITWEB_HOME_LINK_STR)|g' \
 	    -e 's|++GITWEB_SITENAME++|$(GITWEB_SITENAME)|g' \
 	    -e 's|++GITWEB_PROJECTROOT++|$(GITWEB_PROJECTROOT)|g' \
-	    -e 's|"++GITWEB_PROJECT_MAXDEPTH++"|$(GITWEB_PROJECT_MAXDEPTH)|g' \
+	    -e 's|++GITWEB_PROJECT_MAXDEPTH++|$(GITWEB_PROJECT_MAXDEPTH)|g' \
 	    -e 's|++GITWEB_EXPORT_OK++|$(GITWEB_EXPORT_OK)|g' \
 	    -e 's|++GITWEB_STRICT_EXPORT++|$(GITWEB_STRICT_EXPORT)|g' \
 	    -e 's|++GITWEB_BASE_URL++|$(GITWEB_BASE_URL)|g' \

index 6908036..fb5a027 100644
@@ -214,6 +214,20 @@ not include variables usually directly set during build):
    Rename detection options for git-diff and git-diff-tree. By default
    ('-M'); set it to ('-C') or ('-C', '-C') to also detect copies, or
    set it to () if you don't want to have renames detection.
+ * $cache
+   To activate caching, set this to an instance of a Cache::Cache
+   compatible cache (e.g. Cache::Memcached).
+ * $large_cache_root
+   A directory in which gitweb will store potentially large cache
+   items (like snapshots or blobs).  Cache items in this directory are
+   not deleted, so you should clean it up yourself periodically.
+   Default: undef (no caching for large items).
+ * $large_cache_case_sensitive
+   Set this to 1 if the file system on which the large cache resides
+   is case sensitive; it will result in a shorter file names.
+ * $page_info
+   Set to 1 to display cache statistics at the bottom of each page;
+   can be used to check whether your cache works.
 
 
 Projects list file format

index aa0eeca..849bb8e 100644
@@ -71,6 +71,22 @@ div.page_footer_text {
 	font-style: italic;
 }
 
+div.page_info {
+	color: #777777;
+	padding: 4px 8px;
+	margin-top: 0.5em;
+	background-color: #ecebe8;
+}
+
+div.page_info p {
+	margin: 0px;
+	padding: 0px;
+}
+
+pre.cache_list {
+	margin: 0.5em 2em;
+}
+
 div.page_body {
 	padding: 8px;
 	font-family: monospace;

index ae7082b..e04fb5f 100755
@@ -54,7 +54,7 @@ gitweb_run () {
 	# written to web server logs, so we are not interested in that:
 	# we are interested only in properly formatted errors/warnings
 	rm -f gitweb.log &&
-	perl -- "$(pwd)/../../gitweb/gitweb.perl" \
+	"$PERL_PATH" -- "$(pwd)/../../gitweb/gitweb.cgi" \
 		>/dev/null 2>gitweb.log &&
 	if grep -q -s "^[[]" gitweb.log >/dev/null; then false; else true; fi
 

@@ -71,7 +71,7 @@ safe_chmod () {
 
 . ./test-lib.sh
 
-perl -MEncode -e 'decode_utf8("", Encode::FB_CROAK)' >/dev/null 2>&1 || {
+"$PERL_PATH" -MEncode -e 'decode_utf8("", Encode::FB_CROAK)' >/dev/null 2>&1 || {
     test_expect_success 'skipping gitweb tests, perl version is too old' :
     test_done
     exit

index 53f2a8a..c0558e5 100755
@@ -89,6 +89,16 @@ test_expect_success 'set up test repository' '
 	test_tick && git pull . b
 '
 
+# set up empty repository
+# create this as a subdirectory of trash directory; not pretty, but simple
+test_expect_success 'set up empty repository' '
+
+	mkdir empty.git &&
+	cd empty.git &&
+	git init --bare &&
+	cd ..
+'
+
 # set up gitweb configuration
 safe_pwd="$("$PERL_PATH" -MPOSIX=getcwd -e 'print quotemeta(getcwd)')"
 large_cache_root="../t9503/large_cache.tmp"

index b0a8269..2d83158 100755
@@ -274,6 +274,11 @@ if (test_page '', 'project list (implicit)') {
 		"title contains $site_name");
 	$mech->content_contains('t9503-gitweb-Mechanize test repository',
 		'lists test repository (by description)');
+	if (follow_link( { text => 'empty.git' }, 'empty git repository')) {
+		# Just check that the empty.git summary page is linked
+		# and doesn't die.
+		$mech->back;
+	}
 }
 
 

1.5.6.2.456.g63fc0

Good.

It was suggested to split this into separate commit from the following change, for making it easier to test (you can check that behavior is the same, with the exception of error handling) and review (smaller patch to read and review).

As it was said, if feasible it would be good idea to put this change into separate commit.

I understand that this change deals with treating invalid specifiers, which point to either object that do not exists, are ambiguous, or point to object of invalid type. Gitweb does check "syntactic" validity of input (of CGI parameters) already, even those that are not used for selected action.

BTW. such check was not feasible before implementing --batch and/or --batch-check options to git-cat-file; I think that possibly one more fork is not much price to pay for better error checking.

Good. The only thing that *might* be controversial is putting empty projects at the bottom of sorted by age (by last change) projects list, instead of at top.

I don't think it is a good change.

Gitweb generates two types of views (pages): transient and immutable. An example of transient view (transient page/action) is for example RSS feed, or summary page. When project (repository) is updated, they can change.

The opposite are immutable pages. They are pages/actions/views where all specifiers are given by full SHA-1; to be more exact all specifiers that are needed to reconstruct object are given by SHA-1. (It is enough to have sufficient check for immutability, i.e. such that if check succeeds, then page is immutable, but it doesn't need to be true in reverse.)

Gitweb sets expires to '+1d' which is one day to pages it considers immutable, while not defining expires for other pages (which results, I think, in lack of expires header). We could have set it to "forever", which in terms of Expires: HTTP header is half a year (from what I remember).

Now I don't see *any* reason to not set long expires for immutable pages; I don't know if forbidding to cache transient pages even if in fact they are generated dynamically is a good idea... Note that if caching is enabled, you can set expires to either time-to-expire of cache entries (simpler), or time left to live to invalidation of item in cache (better, but more complicated) perhaps also setting Age:
header to appropriate value.

Sidenote: we would probably want to use Expires: for HTTP/1.0 requests, and Cache-Control: max-age=<seconds> for HTTP/1.1 requests. But that might be left as improvement for later...

Great idea!

Nice.

IIRC you can use any Cache::Cache compatibile (is it explained later what it means?) cache here; IMVHO it would be nice if this info would be also in commit message.

Errr... I understand that it is your _private_ configuration, just copied here verbatim, but I don't think '/home/lewiemann/gitweb-cache' is a good example: '/tmp/gitweb-cache' perhaps, that I can understand.

What should be used in production? "$cache_key = $version;"?

Besides hardcoding those paths is not a good idea. You can always use $ENV{'SCRIPT_FILENAME'}, or dirname of it.

Errr... what does "$page_info = <n>;" mean?

Nice. Thanks.

[...]

Very good.

[Comments on patch itself in separate email, later]

Jakub Narebski
Poland

Yup; I'll probably send updated patches tomorrow night (also for patch 2/3).

Yes, that's right. (I don't believe we have any point where ambiguity might come up though.)

Yup; let's see if anyone objects though. If I sort the list by "Last Change", I usually want to see projects with recent activity, not dead project, at the top, which is why I changed it (since I was touching that line anyway).

Hm; I thought transient titles could slip in (e.g. try opening the tree of some commit and remove the hb parameter; the URL will seem cacheable, but the page contains the title of the HEAD commit), but I can't find any URL right now where mainline actually sets a wrong Expires header. I'll look into it; if you don't see me posting about it again I'll re-add the Expires header.

Gitweb's cache is actually never out-of-date, and cache invalidation happens automatically. It uses some (long) expiry times to guard against non-standard modification of the repository, but it's nothing the HTTP client should be concerned with.

I'll add that.

Yup. ;-) Or /var/cache/gitweb.

No, nothing. $version is used automatically as a cache key; I'll add that to the documentation for $cache_key.

That one doesn't exist with my thttpd, or any other environment variable that'd be usable. It's just a hack anyway, so hardcoded paths are OK. :) I don't think gitweb should check its own mtime by default.

Display no (0) / short (1) / detailed (2) page (cache) info at the bottom of each page. It's documented in gitweb.perl.

Thanks!

Could you explain then how gitweb cache is invalidated?

The _reasoning_ behind setting Expires:/Cache-Control: max-age= header for gitweb with caching is that browser doesn't need to even try to cache-validate or access page for the time we *know* that output would not change[*1*] because it is from the cache.

[*1*] In significant way: changing relative dates/time doesn't count.

Jakub Narebski
Poland

Sure; from gitweb.perl:

# Transient cache entries (like get_sha1('HEAD')) are automatically

# invalidated when an mtime of either the repository's root directory
# or of the refs directory or any subdirectory changes.  This
# mechanism *should* detect changes to the repository reliably if you
# only use git or rsync to write to it,

IOW, gitweb will do a small number of (inexpensive) stat calls on those directories each time it's called, and use the most recent mtime as part of the cache key for transient entries. Hence those transient entries will automatically become invalid once the most recent mtime changes.

(If any of the relevant directories has been modified since the last time gitweb checked, gitweb will re-scan the whole tree to check for new directories, and record their mtimes as well. See get_last_modification if you're interested in more gory details.)

The punchline is, the cache never returns outdated data.

I'll try to read and comment on patch itself soon.

Nice idea... for project pages. I'm not so sure about projects_list page, if wouldn't be better to have expire time for *this* page. You would have/have to stat a lot of files/directories to detect changes.

BTW. some summary of the above should be IMHO in the commit message. In short, you should write, I think, that caching mechanism uses any Cache::Cache compatible cache for caching data, that large objects can be cached on filesystem (or perhaps not, at it is a detail), that cache validity is checked by stat-ing refs area.

In this case you don't have any basis to set expires for transient views; on the other hand situation doesn't differ much from serving static files (wrt. cache validation) so perhaps no expires but no "no-cache" would be a good solution.

P.S. is there any cache evision mechanism (to limit cache size) in gitweb cache, or just those implemented by caching backend?

Jakub Narebski
Poland

It doesn't seem to be too much of a performance issue (it takes ~500ms to generate the project list on kernel.org), and project lists aren't requested often enough to be a good optimization target.

I'll see how it performs when the OS's page cache is cold though; if it's too slow then I might revisit the issue.

*nods*

Gitweb provides a view on a live repository, so I don't think caching is usually what you want (e.g. static pages really tend to change much less frequently). (And yes, my Opera *does* seem to cache those pages unless you add no-cache.)

Really, I don't think the performance penalty of revalidation will be significant enough to justify risking stale data. IOW, it's pretty fast. Check out repo.or.cz's performance (and repo.or.cz doesn't even use Last-Modified).

For the $cache, it's in the caching backend, for the $large_cache, there is no mechanism, so you need to clean up yourself. See the patch. ;-)

Hi,

Wasn't the main page (i.e. the projects list) the reason why kernel.org has its own little caching mechanism in the first place?

And did Pasky not report recently that repo.or.cz got substantially less loaded with some caching of its own?

warthog? Pasky?

Ciao,
Dscho

Partially - it was by *far* the biggest problem we were facing, and it likely has gotten a lot better since I went on a rampage and did gitweb-caching.

That said there's a relatively small sample-set of files that gets hit more than others it seems (from stuff I was looking at a while ago) and limiting the general disk i/o on the kernel.org machines is *always* a good thing (the less we go to disk the better) so while it started that way it has been very beneficial, if it does chew up something like 15-20G of disk on those machines.)

- John 'Warthog9' Hawley

Yes, you need *some* caching mechanism. Once you have that, it's fine, at least if the page cache is hot. (The x-thousand stat calls that my caching implementation issues don't actually take that much time; I suspect the ~1000 calls to memcached are the more expensive [and optimizable] part, though I'd have to benchmark that.)

Mainline vs. my caching implementation (both with hot page cache) on odin3.kernel.org:

$ time wget -qO/dev/null http://localhost/git-lewiemann/vanilla/
real    0m3.070s
$ time wget -qO/dev/null http://localhost/git-lewiemann/
real    0m0.719s

To continue with that benchmarking it seems if gitweb-caching isn't hot it takes about 3 seconds, if it is I'm about 7x faster than Lea's ;-)

$ time wget -qO/dev/null http://localhost/git/

real    0m2.952s
user    0m0.001s
sys     0m0.004s
$ time wget -qO/dev/null http://localhost/git/

real    0m0.108s
user    0m0.001s
sys     0m0.001s

But regardless of who's faster - I would still argue there are two reasons to at least have some caching, even if it's crappy:

1) Prevent the thundering heard problem - many requests for the same thing all generating the same data is bad, and kills I/O

2) Relatively static data can be generated once and stick around for a bit and serve more requests more efficiently. Now I agree that invalidating the cache on a new mtime is better than my current algorithm (which is purely time based on the cache data vs. the original data, with some allowances for back-off due to load).

- John 'Warthog9' Hawley

Somehow I didn't get Cc-ed this patch...

If I remember correctly Mechanize tests detected some bugs in gitweb (nice!), but there were swept under the rug, i.e. put as TODO.

Does that mean that those errors were corrected, or that refactoring "just" didn't break anything more?

Do you plan to compare other gitweb caching implementations? I mean here old kernel.org caching by J.H., and repo.or.cz caching (only for projects list page) by Pasky.

http()
Called with no arguments returns the list of HTTP environment vari- ables, including such things as HTTP_USER_AGENT, HTTP_ACCEPT_LANGUAGE, and HTTP_ACCEPT_CHARSET, corresponding to the like-named HTTP header fields in the request. Called with the name of an HTTP header field, returns its value. Capitalization and the use of hyphens versus under- scores are not significant.

For example, all three of these examples are equivalent:

$requested_language = http('Accept-language'); $requested_language = http('Accept_language'); $requested_language = http('HTTP_ACCEPT_LANGUAGE');

In addition to these, the header lines received from the client, if any, are placed into the environment with the prefix HTTP_ followed by the header name. Any '-' characters in the header name are changed to '_' characters. The server may exclude any headers which it has already processed, such as Authorization, Content-type, and Content-length. If necessary, the server may choose to exclude any or all of these headers if including them would exceed any system environment limits.

So you _can_ access 'If-Modified-Since', 'If-None-Match' (by web browsers) and 'If-Not-Modified-Since', 'If-Match' (by caches) from CGI.

Currently only one "shortcut" is that gitweb respects HEAD request (returning only HTTP headers) for feeds, and if I remember correctly only for feeds. I don't know however how much work gitweb does before HEAD request shortcut.

I think that ls_tree and git-ls-tree output parsing should be generalized into Git::Tree API as well.

I'll try to review the rest of patches by tomorrow...

Jakub Narebski
Poland

Yup, nobody got Cc'ed; apologies.

Refactoring really just didn't break anything more; it could be that something got corrected accidentally, but I'd be surprised.

Yup, sure.

Right, you'd think the request headers should be accessible this way, but apparently not all of them are.

If you take this script, ...

#!/usr/bin/perl
use CGI qw(http);
print "Content-type: text/plain\n";
print "Last-Modified: Thu, 03 Jul 2008 22:39:42 GMT\n\n"; print "Header: ", http('If-Last-Modified');

... then my browser (according to LiveHTTPHeaders) sends an If-Last-Modified header, but it doesn't get through to the CGI script. It does work if you test Accept-Language. (Try print env to get an idea of what gets through.) It happens on Apache and thttpd. Apparently this part somehow applies:

I honestly have no idea why the If-Last-Modified headers gets eaten (and googling didn't help), but I assume that at least it'll be possible to access all headers with mod_perl.

Wait, are you sure caches would use those headers (I believe only the latter actually exists BTW), or did you fall prey to a thinko? ;)

Yes, and I think it does help performance-wise, but only a really small fraction of the RSS/Atom requests actually use HEAD. Most use GET.

True, though I'm still not sure how to make element access work pretty and fast. I'll keep pondering it for a while.

Thanks!

I've put splitting it on my list; I'm not sure though if I'll get around to doing it today. Everyone, comments on patch (3) are still appreciated in the meantime. ;-) I'll integrate any suggestions/patches when I split it up.

-- Lea

If-Not-Modified-Since should really be If-Unmodified-Since.

But where's the thinko? To send If-Modified-Since or similar with a request, you would need to have a cached copy of the resource and use its Last-Modified, for example. Sure, you could do it without one, but what would be the point?

-- ams

Actually the thinko was use of If-Unmodified-Since: and If-Match:.

Both If-Unmodified-Since and If-Match can be used and should be used when requesting *partial document transfers* (when you need to ensure that document is unchanged before you fetch next part of it). Additionally If-Match can be used for concurrenty control when you do a two-way communication using GET and PUT, in a PUT request to ensure that you are modifying correct document.

They are not used, I think, by caches to refresh cached data.

Jakub Narebski
Poland
ShadeHawk on #git

Hi,

FWIW there are a few reasons why splitting up (3) might be the thing you really want to do, even if it takes an hour or two:

- it makes reviewing much easier,
- it makes subsequent revisions of the patches easier to review,
- it make it easier to cherry-pick changes, should not all be equally liked,
- it makes finding bugs much easier (both spotting during review and bisecting), and
- it is good for documentation purposes, should someone read the commit log.

Now, after weighing the benefit (especially in terms of hours spared others) against the downsides, you might want to reconsider your stance.

Ciao,
Dscho

I unfortunately didn't have enough time to finish the update to the patch series before my vacation; I'll therefore be posting the next series only around Aug 12th-13th. I suggest you wait with any reviewing till I've posted the next version, since there'll be quite a few changes; also I'll post patch 3 (the gitweb patch) as (at least) two separate patches, which will hopefully help with reviewability.

Alright, off to my vacation now -- I'll be completely offline till Aug 10th.

See you then!

-- Lea

I'll be sending the next version of the patch series as a follow-up to this message. As the GSoC pencils-down deadline is in -0.5h, I'll try to get the code out first and then I'll post some comments on a few aspects it.

I unfortunately didn't end up being able to split up the third patch (use Perl API in Gitweb, and add caching layer), since the two changes are too intricately linked to be properly separated (I actually tried splitting it two times, two different ways, and it just didn't work). The only thing that's really not caching-specific to the third patch (i.e. where using the Perl API merely prettifies the code) is where Gitweb now uses $repo->get_sha1. Those few occurrences are only a very small part of the patch, and I don't think they'd warrant ripping it apart.

Here's the list of changes since the last patch series:

Patch 1/1 (Mechanize tests):

- typo

Patch 2/3 (Perl API):

- remove get_refs, get_path, and version methods

- rename cat_file to get_object

- remove unnecessary cmd_output and get_bidi_pipe methods -- they shouldn't be part of this API (because they don't fit into the design), and keeping and underscore-prefixing them isn't worth the code complexity -- so the Git::Repo methods now use 'open' calls directly

- rename the 'directory' options of RepoRoot->new, Repo->new, and RepoRoot->repo to 'repo_dir' and 'root_dir' respectively

- ignore (rather than die on) unrecognized header lines in tag/commit objects

- do not use unnecessary hash index optimization

- add design notes

- underscore-prefix assert_opts and assert_sha1

- remove dependency of the test suite (t9710) on non-standard modules; Git::Repo should now run on vanilla Perl 5.8

- respect PERL_PATH in test suite

- add "use perl 5.008" statement (so it dies for Perl <5.8) -- it needs Perl 5.8 for Unicode support

- do not "or" author/committer/tagger/type with '' (it's unnecessary and silly)

- improved documentation, per Jakub's suggestions mostly

- author, committer, tagger, and message now all return Unicode strings per <[email protected]>

- some minor cleanups, typos

- remove warning about Git::Repo being unstable; this is a silly excuse that in theory allows you to make incompatible changes ("we told you so"), but in practice never holds up

Patch 3/3 (Gitweb):

- add installation note for the Git perl package to INSTALL

- accomodate removals/renamings in Git::Repo API

- remove two TODO markers in t9503 (gitweb tests) -- works now thanks to Git::Repo

- some minor cleanups, typos

- do not remove the Expires header (as was the case in the previous patch)

- remove option to assume case sensitive file system to unclutter the code -- the cache will now work on any file system

- elaborate on inefficient refs/ traversal code in get_last_modification method

This test uses Test::WWW::Mechanize::CGI to check gitweb's output. It also uses HTML::Lint, XML::Parser, and Archive::Tar (if present, each) to validate the HTML/XML/tgz output, and checks all links on the tested pages if --long-tests is given.

---
 Makefile                    |    1 +
 t/t9503-gitweb-Mechanize.sh |  144 ++++++++++
 t/t9503/test.pl             |  604 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 749 insertions(+), 0 deletions(-)
 create mode 100755 t/t9503-gitweb-Mechanize.sh
 create mode 100755 t/t9503/test.pl

index ca418fc..35779a7 100644
@@ -1289,6 +1289,7 @@ GIT-CFLAGS: .FORCE-GIT-CFLAGS
 GIT-BUILD-OPTIONS: .FORCE-GIT-BUILD-OPTIONS
 	@echo SHELL_PATH=\''$(subst ','\'',$(SHELL_PATH_SQ))'\' >$@
 	@echo TAR=\''$(subst ','\'',$(subst ','\'',$(TAR)))'\' >>$@
+	@echo PERL_PATH=\''$(subst ','\'',$(PERL_PATH_SQ))'\' >>$@
 
 ### Detect Tck/Tk interpreter path changes
 ifndef NO_TCLTK

1.6.0.90.gf3f7b

The Git::Repo class provides low-level access to Git repositories. The Git::Commit and Git::Tag classes represent commit and tag objects and allow for accessing their respective properties (like 'author', 'tagger', etc.); Git::Object serves as a common base class for them. Git::RepoRoot is a helper factory class to instantiate Git::Repo objects with common properties for repositories under a common root directory.

---
 perl/Git/Commit.pm       |  179 ++++++++++++++++++++++++++++++
 perl/Git/Object.pm       |   94 ++++++++++++++++
 perl/Git/Repo.pm         |  274 ++++++++++++++++++++++++++++++++++++++++++++++
 perl/Git/RepoRoot.pm     |  103 +++++++++++++++++
 perl/Git/Tag.pm          |  169 ++++++++++++++++++++++++++++
 perl/Makefile            |    1 +
 perl/Makefile.PL         |   10 ++-
 t/t9710-perl-git-repo.sh |   49 ++++++++
 t/t9710/test.pl          |  136 +++++++++++++++++++++++
 9 files changed, 1014 insertions(+), 1 deletions(-)
 create mode 100644 perl/Git/Commit.pm
 create mode 100644 perl/Git/Object.pm
 create mode 100644 perl/Git/Repo.pm
 create mode 100644 perl/Git/RepoRoot.pm
 create mode 100644 perl/Git/Tag.pm
 create mode 100755 t/t9710-perl-git-repo.sh
 create mode 100755 t/t9710/test.pl

new file mode 100644
index 0000000..58e502f
@@ -0,0 +1,179 @@
+=head1 NAME
+
+Git::Commit - Object-oriented interface to Git commit objects.
+
+=head1 DESCRIPTION
+
+Git::Commit is a class representing a commit object in a Git
+repository.  It stringifies to the commit object's SHA1.
+
+=cut
+
+use strict;
+use warnings;
+
+
+package Git::Commit;
+
+use base qw(Git::Object);
+
+
+# Keep documentation in one place to save space.
+
+=head1 METHODS
+
+=head2 General Methods
+
+=over
+
+=item $commit = Git::Commit->new($repo, $sha1)
+
+Return a new Git::Commit instance for a commit object with $sha1 in
+repository $repo.
+
+The commit object is loaded lazily.  Hence, calls to this method are
+free, and it does not check whether $sha1 exists and has the right
+type.  However, accessing any of the commit object's properties will
+fail if $sha1 is not a valid commit object.
+
+Note that $sha1 must be the SHA1 of a commit object; tag objects are
+not dereferenced.
+
+The author, committer and message methods return Unicoded strings,
+decoded according to the "encoding" header, with UTF-8 and then
+Latin-1 as fallbacks.  (These Unicode strings can contain code points
+greater than 256 and are *not* UTF-8 strings; see man perlunitut on
+how Perl handles Unicode.)
+
+You will usually want to call $repo->get_commit($sha1) instead of
+instantiating this class directly; see L<Git::Repo>.
+
+=item $obj->repo
+
+Return the Git::Repo instance this object was instantiated with.
+
+=item $obj->sha1
+
+Return the SHA1 of this commit object, as provided at instantiation time.
+
+=back
+
+=head2 Property Methods
+
+Calling any of these methods will cause the commit object to be loaded
+from the repository, if it hasn't been loaded already.
+
+=over
+
+=item $commit->tree
+
+Return an object that stringifies to the SHA1 of the tree that this
+commit object refers to.  (Currently this returns an actual string,
+but don't rely on it.)
+
+=item $commit->parents
+
+Return an array of zero or more parent commit objects.  Note that
+commit objects stringify to their respective SHA1s, so you can
+alternatively treat this as an array of SHA1 strings.
+
+=item $commit->author
+
+Return the author of this commit object as a Unicode string.
+
+=item $commit->committer
+
+Return the committer of this commit object as a Unicode string.
+
+=item $commit->message
+
+Return the commit message of this commit object as a Unicode string.
+
+=item $commit->encoding
+
+Return the encoding header of the commit object, or undef if no
+encoding header is present; note that Git::Commit does the necessary
+decoding for you, so you should not normally need this method.
+
+=back
+
+=cut
+
+
+sub tree {
+	my $self = shift;
+	$self->_load;
+	return $self->{tree};
+}
+
+sub parents {
+	my $self = shift;
+	$self->_load;
+	return map { ref($self)->new($self->repo, $_) } @{$self->{parents}};
+}
+
+sub author {
+	my $self = shift;
+	$self->_load;
+	return $self->_decode($self->{author});
+}
+
+sub committer {
+	my $self = shift;
+	$self->_load;
+	return $self->_decode($self->{committer});
+}
+
+sub message {
+	my $self = shift;
+	$self->_load;
+	return $self->_decode($self->{message});
+}
+
+sub encoding {
+	my $self = shift;
+	$self->_load;
+	return $self->{encoding};
+}
+
+# Auxiliary method to load (and parse) the commit object from the
+# repository if it hasn't already been loaded.  Optional parameter:
+# The raw contents of the commit object; the commit object will be
+# retrieved from the repository if that parameter is not given.
+sub _load {
+	my ($self, $raw_text) = shift;
+	return if exists $self->{message};  # already loaded
+
+	my $sha1 = $self->sha1;
+	if (!defined $raw_text) {
+		# Retrieve from the repository.
+		(my $type, $raw_text) = $self->repo->get_object($sha1);
+		die "$sha1 is a $type object (expected a commit object)"
+		    unless $type eq 'commit';
+	}
+
+	(my $header, $self->{message}) = split "\n\n", $raw_text, 2;
+	# Parse header.
+	for my $line (split "\n", $header) {
+		local $/ = "\n"; # for chomp
+		chomp($line);
+		my ($key, $value) = split ' ', $line, 2;
+		if ($key eq 'tree') {
+			$self->{tree} = $value;
+		} elsif ($key eq 'parent') {
+			push @{$self->{parents}}, $value;
+		} elsif ($key eq 'author') {
+			$self->{author} = $value;
+		} elsif ($key eq 'committer') {
+			$self->{committer} = $value;
+		} elsif ($key eq 'encoding') {
+			$self->{encoding} = $value;
+		} else {
+			# Ignore unrecognized header lines.
+		}
+	}
+	undef;
+}
+
+
+1;

index e3dd1a5..4572dc5 100644
@@ -31,6 +31,7 @@ $(makfile): ../GIT-CFLAGS Makefile
 	echo install: >> $@
 	echo '	mkdir -p "$(instdir_SQ)"' >> $@
 	echo '	$(RM) "$(instdir_SQ)/Git.pm"; cp Git.pm "$(instdir_SQ)"' >> $@
+	echo '	mkdir -p "$(instdir_SQ)/Git"; $(RM) "$(instdir_SQ)"/Git/*.pm; cp Git/*.pm "$(instdir_SQ)/Git"' >> $@
 	echo '	$(RM) "$(instdir_SQ)/Error.pm"' >> $@
 	'$(PERL_PATH_SQ)' -MError -e 'exit($$Error::VERSION < 0.15009)' || \
 	echo '	cp private-Error.pm "$(instdir_SQ)/Error.pm"' >> $@

index 320253e..6c62160 100644
@@ -8,7 +8,15 @@ instlibdir:
 MAKE_FRAG
 }
 
-my %pm = ('Git.pm' => '$(INST_LIBDIR)/Git.pm');
+# Note that when changing %pm, you'll have to remove pm_to_blib rather
+# than the blib directory to .force the .pm files to be re-installed
+# to blib/lib.
+my %pm = ('Git.pm' => '$(INST_LIBDIR)/Git.pm',
+	  'Git/Repo.pm' => '$(INST_LIBDIR)/Git/Repo.pm',
+	  'Git/RepoRoot.pm' => '$(INST_LIBDIR)/Git/RepoRoot.pm',
+	  'Git/Commit.pm' => '$(INST_LIBDIR)/Git/Commit.pm',
+	  'Git/Tag.pm' => '$(INST_LIBDIR)/Git/Tag.pm',
+	  'Git/Object.pm' => '$(INST_LIBDIR)/Git/Object.pm');
 
 # We come with our own bundled Error.pm. It's not in the set of default
 # Perl modules so install it if it's not available on the system yet.

1.6.0.90.gf3f7b

Gitweb now uses the Git::Repo API; this change is behavior-preserving, except for slightly more aggressive error handling; see below.

This patch also adds an optional caching layer for caching repository data using any Cache::Cache-compatible caching engine (typically Cache::Memcached). Larger cacheable items, like blobs, snapshots or diffs, can be cached on disk.

Other minor changes:

- Gitweb would previously accept invalid input and either (a) display nothing, (b) display an obscure error message, or (c) proceed as normal since the parameter happens to be unused in the particular code path used. This has changed in that gitweb will check for parameter correctness more aggressively, and display meaningful error messages. This change is only relevant if you manually edit gitweb's CGI parameters, since gitweb only generates valid links.

- Empty projects:

- Only display summary link for empty projects in project list to avoid broken links (yielding 404).

- Slim down summary page for empty projects to avoid some broken links and unnecessary vertical space.

- Sort empty projects at the bottom of the project list when sorting by last change.

- Add test for empty projects to t9503 (the Mechanize test), now that there no broken links anymore.

---
 Makefile                               |   25 +-
 gitweb/INSTALL                         |   11 +-
 gitweb/README                          |   11 +
 gitweb/gitweb.css                      |   16 +
 gitweb/gitweb.perl                     | 1371 +++++++++++++++++++------------
 t/t9500-gitweb-standalone-no-errors.sh |    4 +-
 t/t9503-gitweb-Mechanize.sh            |   10 +
 t/t9503/test.pl                        |   19 +-
 8 files changed, 911 insertions(+), 556 deletions(-)

index 35779a7..cab42d8 100644
@@ -1128,25 +1128,28 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): perl/perl.mak
 perl/perl.mak: GIT-CFLAGS perl/Makefile perl/Makefile.PL
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' $(@F)
 
+PERL_USE_LIB_REWRITE = \
+	-e '1{' \
+	-e '	s|\#!.*perl|\#!$(PERL_PATH_SQ)|' \
+	-e '	h' \
+	-e '	s=.*=use lib (split(/:/, \$$ENV{GITPERLLIB} || \"@@INSTLIBDIR@@\"));=' \
+	-e '	H' \
+	-e '	x' \
+	-e '}' \
+	-e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
+	-e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g'
+
 $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
-	sed -e '1{' \
-	    -e '	s|#!.*perl|#!$(PERL_PATH_SQ)|' \
-	    -e '	h' \
-	    -e '	s=.*=use lib (split(/:/, $$ENV{GITPERLLIB} || "@@INSTLIBDIR@@"));=' \
-	    -e '	H' \
-	    -e '	x' \
-	    -e '}' \
-	    -e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
-	    -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
-	    [email protected] >$@+ && \
+	sed $(PERL_USE_LIB_REWRITE) [email protected] >$@+ && \
 	chmod +x $@+ && \
 	mv $@+ $@
 
 gitweb/gitweb.cgi: gitweb/gitweb.perl
 	$(QUIET_GEN)$(RM) $@ $@+ && \
-	sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \
+	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` && \
+	sed $(PERL_USE_LIB_REWRITE) \
 	    -e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \
 	    -e 's|++GIT_BINDIR++|$(bindir)|g' \
 	    -e 's|++GITWEB_CONFIG++|$(GITWEB_CONFIG)|g' \

index 26967e2..c4ab593 100644
@@ -6,8 +6,15 @@ First you have to generate gitweb.cgi from gitweb.perl using
 gitweb.css, git-logo.png and git-favicon.png) to their destination.
 For example if git was (or is) installed with /usr prefix, you can do
 
-	$ make prefix=/usr gitweb/gitweb.cgi  ;# as yourself
-	# cp gitweb/git* /var/www/cgi-bin/    ;# as root
+	$ make prefix=/usr gitweb/gitweb.cgi           ;# as yourself
+	# cp gitweb/git* /var/www/cgi-bin/             ;# as root
+
+If the Git Perl package isn't already installed system-wide, you also
+need to copy the perl/blib/lib/Git directory to your cgi-bin
+directory:
+
+	$ make prefix=/usr -C perl                     ;# as yourself
+	# cp -r perl/blib/lib/Git /var/www/cgi-bin/    ;# as root
 
 Alternatively you can use autoconf generated ./configure script to
 set up path to git binaries (via config.mak.autogen), so you can write

index 825162a..81ec2ae 100644
@@ -214,6 +214,17 @@ not include variables usually directly set during build):
    Rename detection options for git-diff and git-diff-tree. By default
    ('-M'); set it to ('-C') or ('-C', '-C') to also detect copies, or
    set it to () if you don't want to have renames detection.
+ * $cache
+   To activate caching, set this to an instance of a Cache::Cache
+   compatible cache (e.g. Cache::Memcached).
+ * $large_cache_root
+   A directory in which gitweb will store potentially large cache
+   items (like snapshots or blobs).  Cache items in this directory are
+   not deleted, so you should clean it up yourself periodically.
+   Default: undef (no caching for large items).
+ * $page_info
+   Set to 1 to display cache statistics at the bottom of each page;
+   can be used to check whether your cache works.
 
 
 Projects list file format

index aa0eeca..849bb8e 100644
@@ -71,6 +71,22 @@ div.page_footer_text {
 	font-style: italic;
 }
 
+div.page_info {
+	color: #777777;
+	padding: 4px 8px;
+	margin-top: 0.5em;
+	background-color: #ecebe8;
+}
+
+div.page_info p {
+	margin: 0px;
+	padding: 0px;
+}
+
+pre.cache_list {
+	margin: 0.5em 2em;
+}
+
 div.page_body {
 	padding: 8px;
 	font-family: monospace;

index 46ba19b..8edbfbd 100755
@@ -54,7 +54,7 @@ gitweb_run () {
 	# written to web server logs, so we are not interested in that:
 	# we are interested only in properly formatted errors/warnings
 	rm -f gitweb.log &&
-	perl -- "$TEST_DIRECTORY/../gitweb/gitweb.perl" \
+	"$PERL_PATH" -- "$TEST_DIRECTORY/../gitweb/gitweb.perl" \
 		>/dev/null 2>gitweb.log &&
 	if grep -q -s "^[[]" gitweb.log >/dev/null; then false; else true; fi
 

@@ -71,7 +71,7 @@ safe_chmod () {
 
 . ./test-lib.sh
 
-perl -MEncode -e 'decode_utf8("", Encode::FB_CROAK)' >/dev/null 2>&1 || {
+"$PERL_PATH" -MEncode -e 'decode_utf8("", Encode::FB_CROAK)' >/dev/null 2>&1 || {
     test_expect_success 'skipping gitweb tests, perl version is too old' :
     test_done
     exit

index 53f2a8a..c0558e5 100755
@@ -89,6 +89,16 @@ test_expect_success 'set up test repository' '
 	test_tick && git pull . b
 '
 
+# set up empty repository
+# create this as a subdirectory of trash directory; not pretty, but simple
+test_expect_success 'set up empty repository' '
+
+	mkdir empty.git &&
+	cd empty.git &&
+	git init --bare &&
+	cd ..
+'
+
 # set up gitweb configuration
 safe_pwd="$("$PERL_PATH" -MPOSIX=getcwd -e 'print quotemeta(getcwd)')"
 large_cache_root="../t9503/large_cache.tmp"

index 93108e7..fca81f7 100755
@@ -271,6 +271,11 @@ if (test_page '', 'project list (implicit)') {
 		"title contains $site_name");
 	$mech->content_contains('t9503-gitweb-Mechanize test repository',
 		'lists test repository (by description)');
+	if (follow_link( { text => 'empty.git' }, 'empty git repository')) {
+		# Just check that the empty.git summary page is linked
+		# and doesn't die.
+		$mech->back;
+	}
 }
 
 

@@ -578,18 +583,12 @@ for my $action qw( blame blob blob_plain ) {
 		  "$action: look up existent file");
 	test_page("?p=.git;a=$action;f=does_not_exist;hb=HEAD",
 		  "$action: look up non-existent file", 404);
-	TODO: {
-		local $TODO = 'wrong error code (but using Git::Repo will fix this)';
-		test_page("?p=.git;a=$action;f=$directories[0];hb=HEAD",
-			  "$action: look up directory", 400);
-	}
-}
-TODO: {
-	local $TODO = 'wrong error code (but using Git::Repo will fix this)';
-	test_page("?p=.git;a=tree;f=$files[0];hb=HEAD",
-		  'tree: look up existent file', 400);
+	test_page("?p=.git;a=$action;f=$directories[0];hb=HEAD",
+		  "$action: look up directory", 400);
 }
 # Pass valid and invalid paths to tree action
+test_page("?p=.git;a=tree;f=$files[0];hb=HEAD",
+	  'tree: look up existent file', 400);
 test_page("?p=.git;a=tree;f=does_not_exist;hb=HEAD",
 	  'tree: look up non-existent file', 404);
 test_page("?p=.git;a=tree;f=$directories[0];hb=HEAD",

1.6.0.90.gf3f7b

Since I didn't mention it: The patch series applies on next.