Re: [PATCH] gitweb: Support for snapshot

From: Junio C Hamano <junkio@cox.net>
Date: Aug 19, 2006, 21:49 UTC
Message-ID: <7vwt941jlp.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <ec76rd$8qf$1@sea.gmane.org>

Jakub Narebski <jnareb@gmail.com> writes:

Show 8 quoted lines

> Below comments to the patch.
>...
> Wouldn't it make it easier to understand code to put %feature hash 
> and gitweb_check_feature subroutine _before_ subroutines for specific
> features?
>
> It would be enough to put:
> $feature{'snapshot'} =  [\&feature_snapshot, 0, undef];

Yes; although actually even 'undef' is not needed, I think it makes sense to have at least one there ;-).

Show 11 quoted lines

> By the way, wouldn't it be better to use _hash_ for mixed meaning
> than _array_? I.e.
>
> our %feature =
> (
>        # feature       => {'sub' => feature-sub, 'override' => allow-override, 'default' => default options...]
>
>        'blame'         => {'sub' => \&feature_blame, 'override' => 0, 'default' => 0},
>    #or 'blame'         => {'sub' => \&feature_blame, 'override' => 0, 'default' => [ 0 ]},
>        'snapshot'      => {'sub' => \&feature_snapshot, 'override' => 0, 'default => [ 'x-gzip', 'gz', 'gzip' ]},
> );

I like it better except that you made it wider than my terminal again making it a lot harder to read.