Re: [PATCHv3 3/4 (resent)] gitweb: Optionally add "git" links in project list page
On Sat, 9 Jan 2010, J.H. wrote:
Show 30 quoted lines
> On 01/03/2010 08:07 AM, Jakub Narebski wrote:
> > From: John 'Warthog9' Hawley <warthog9@kernel.org>
> >
> > This adds a "git" link for each project in the project list page,
> > should a common $gitlinkurl_base be defined and not empty. The full
> > URL of each link is composed of $gitlinkurl_base and project name.
> > It is intended for git:// links, and in fact GITWEB_BASE_URL build
> > variable is used as its default value only if it starts with git://
> >
> > This does make the assumption that the git repositories share a common
> > path. Nothing to date is known to actually make use of introduced
> > link.
> >
> > Created "git" link follows rel=vcs-* microformat specification:
> > http://kitenet.net/~joey/rfc/rel-vcs/
> >
> > Signed-off-by: John 'Warthog9' Hawley <warthog9@kernel.org>
> > Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> > ---
> > I think it might be good idea... but for the fact "Nothing to date is
> > known to actually make use of introduced link". What's its intended
> > use?
> >
> > Differences to original version by John 'Warthog9' Hawley (J.H.):
> > * It doesn't cause syntax error ;-)
> > * Escaping of attribute value is left to CGI.pm (avoid double escaping)
> > * $gitlinkurl got renamed to $gitlinkurl_base, now includes git://
> > prefix, and defaults to GITWEB_BASE_URL if it begins with git://
> > * Added description of $gitlinkurl_base to gitweb/README
> > * Uses rel=vcs-* microformat by Joey Hess
> > gitweb/README | 4 ++++
> > gitweb/gitweb.perl | 8 ++++++++
> > 2 files changed, 12 insertions(+), 0 deletions(-)
A reminder - this patch series consists of the following patches:
[PATCHv2 1/4 (resent)] gitweb: Load checking
[RFC/PATCHv2 2/4 (resent)] gitweb: Add option to force version match
[PATCHv3 3/4 (resent)] gitweb: Optionally add "git" links in project list page
[PATCHv2/RFC 4/4 (resent)] gitweb: Makefile improvements
Show 5 quoted lines
> Ok I've been debating this as I've been going through the patches, I've
> got small modifications on top of your patches Jakub for 1 and 2,
> haven't pushed them yet but they are relatively trivial. The changes to
> the first patch sets things up for additional load checkers to be added
> later on.
Good idea, although I think that such addition can be left for a separate patch.
By the way, are you doing if-elsif fallback chain, trying different mechanisms (like '/proc/loadavg', BSD::getloadavg, etc.), or did you made get_loadavg() into code reference, i.e. run it with $get_loadavg->(), which has the advantage that the gitweb admin can override it in gitweb config file (including such thing like simply using load average over last 5 minutes, and not over last minute)?
> The second changes the error message to use/abuse die_error()
> vs. doing it's own thing (though I still think this should be on by
> default).
True, the error message could use improvement (and not only using its own class instead of abusing 'readme' class, or renaming 'readme' class to something more generic). The problem with error message for this is who is the target of this message: is it gitweb administrator (who can change gitweb configuration), or is it gitweb user (who need to contact web admin).
The problem with this patch is that for it to be useful for protecting against silent errors it should be on by default, but OTOH having it on by default is quite inconvenient.
Best solution would be to treat core of this issue, namely eliminate silent errors and always provide some message in case of error.
Show 13 quoted lines
> Patch 4 I don't have anything to add or change at this point.
>
> This patch has me pondering and I'm unsure of what I'd suggest, mainly
> because of the addition of the smart http support meaning that git://
> and http:// are legitimate and useful links for supporting full git
> transactions.
>
> I may withdraw the patch entirely since the link on kernel.org has been
> around for years, and I'm unsure if anything actually uses it (though I
> can see it being useful still). If it stays I think there's got to be a
> way to specifically mark a url as being the one to link to vs.
> defaulting to git:// (or allow for a marking to override the git://) and
> I need to ponder that.
Also, it has to be _fast_, I think, i.e. no reading cloneurl and repo config (for gitweb.url) for each repository.
You can always remove the check for "git://" prefix, and/or take first base in @git_base_url_list.
>
> I have given some initial thought to converting the $output options I'm
> currently using to a print <FH> that Jakub is suggesting & exploring.
It's 'print {$fh}', i.e. use indirect filehandle, not global filehandle.> I think all told it's going to be a similarly sized patch, since all
> output still has to get adjusted (including the things that directly
> output but don't print).
print -> print {$fh} can be separate patch, and it can be checked that it produces the same results. Well print -> $output .= could also be separate patch...
Show 5 quoted lines
> I'm unsure if there's a real advantage to
> either way, other than design preference. My patch (forcing the output
> to get passed around) moves towards more of a modal style design
> separating data & layout vs. it's combined nature now, well it's a step
> in that direction anyway.
Errr... what? Forcing buffering (you need to read whole output into memory, including for snapshots (uncompressed in case of .tar.gz)) is IMVHO orthogonal to the issue of separating data & layout. BTW. Modern web server interfaces like Rack, PSGI/Plack etc. explicitly include streaming support.
The advantage of doing 'print {$fh}' is that $fh can be \*STDOUT, can be \$buffer, but can be filehandle to (temporary) file on disk, and you can even "tee" it, i.e. both write to memory/file, and to STDOUT. The number of possible choices / possible improvements is much larger.
And what is also important it means that people who do not use caching do not suffer latency penalty and memory pressure from caching infrastructure they do not use.
P.S.
Subject: [Virus] Exploit.PDF-9669
X-Virus-Scanned: ClamAV 0.88.7/10275/Fri Jan 8 17:06:46 2010 on shards.monkeyblade.net
X-Virus-Status: Infected with Exploit.PDF-9669
X-Original-Subject: Re: [PATCHv3 3/4 (resent)] gitweb: Optionally add "git"
links in project list page
A message sent from <warthog9@eaglescrag.net> to
<jnareb@gmail.com>
<git@vger.kernel.org>
<warthog9@kernel.org>
contained Exploit.PDF-9669 and has not been delivered.--
Jakub Narebski
Poland
Jakub Narebski