Re: [PATCH] gitweb: return correct HTTP status codes

From: Jakub Narebski <jnareb@gmail.com>
Date: Jun 18, 2008, 00:12 UTC
Message-ID: <200806180212.15392.jnareb@gmail.com>
In-Reply-To: <48584D1F.5070000@gmail.com>

Lea Wiemann wrote:

Show 13 quoted lines

> Jakub Narebski wrote:
> >
> > But what are arguments for "check params; run command" vs "run command;
> > check params if error" proposed by Junio?  Why do you want to check
> > parameters upfront?
> 
> It's actually not checking, it's resolving.  Instead of ...
> 
> get_commit($symbol)
> 
> ... (with $symbol = 'HEAD' for instance), you do (pseudocode):
> 
> $hash = get_hash($symbol, 'commit'); # 'commit' to resolve tags

Errr... is there equivalent to ^{}, i.e. resolve to non-tag?

> check that $hash is defined
> get_commit($hash)

Note that you would have to examine gitweb sources to check if it uses href(..., -replay=>1) when it should, so the links from "volatile" link, e.g. HEAD version URL, also lead to appropriate version, e.g. "tree" from current / HEAD version.

Show 7 quoted lines

> (And get_commit won't even accept anything but 40-byte hashes.)  This is 
> for two reasons:
> 
> 1. Caching: Resolving symbols first gives you some (very few) cache 
> entries that need to be expired (namely, get_hash results for symbols 
> that are not SHA1 hashes already), but most cache entries (like the 
> get_commit) are infinitely valid.

BTW. one of earliest idea was to fully resolve hashes, add missing parameters if possible (like 'h', 'hp', 'f') and convert hashes to sha-1. One of intended uses was (weak) ETag for simple HTTP caching.

But it was before git-cat-file --batch-check, and before href(...,-replay=>1).

[...]

Thanks for explanation. It makes clear why you have chosen this solution (I think that (1) is deciding factor here).

> > By the way, would you be sending your current WIP for review?
> 
> Will do in the next 2 days after some cleanup.

Thanks.

I have read your current code, but I'd rather not comment on it (like no_plan and skipping tests, or elaborate wrapping fields vs. perltie) until it is ready, i.e. sent for peer review.

Show 6 quoted lines

> > [is] adding object oriented interface (wrapper) to git repositories
> > really  needed for implementing gitweb caching?
> 
> It makes things better to read for sure, and it means that you can have 
> a plain API without caching, and implement the caching layer as a 
> subclass (which overrides the methods with cacheable results).

All the time I think that caching _everything_ is a bad solution. Besides sometimes you need to cache information specific for gitweb, or information _aggregated_ by gitweb, which doesn't have place as single entity in Git::Repo.

So while I think that Git::Repo and making gitweb use it is a worthwhile goal, I think caching should be explicitely in gitweb. See CGI::Cache for good solution of inobtrusive output caching, and CHI (or other in recommended thread) for inobtrusive data caching using callbacks (and Cache::None engine). That not said that we should use such non-standard Perl modules!

<probably should not send email this late>

-- 
Jakub Narebski
Poland