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

[Jakub Narebski <jnareb@gmail.com>]

Lea Wiemann wrote:
> 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.

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

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