Re: [PATCHv2 2/2] gitweb: support to globally enable/disable a snapshot format

From: Mark A Rada <marada@uwaterloo.ca>
Date: Aug 1, 2009, 23:50 UTC
Message-ID: <D33D9C93-300E-4565-8040-BED425A6F2FE@uwaterloo.ca>
In-Reply-To: <m3hbwrqly9.fsf@localhost.localdomain>

On 1-Aug-09, at 5:11 PM, Jakub Narebski wrote:

Show 22 quoted lines

> Mark A Rada <marada@uwaterloo.ca> writes:
>
>> ------->8-------------
>> From: Mark Rada <marada@uwaterloo.ca>
>> Date: Sat, 1 Aug 2009 13:24:03 -0400
>> Subject: [PATCH 2/2] gitweb: support to globally enable/disable a  
>> snapshot format
>>
>> I added a 'disabled' variable to to the $known_snapshot_formats  
>> keys so
>> that a Gitweb administrator can globally enable or disable a specific
>> format for snapshots.
>>
>> All formats are enabled by default because project specific  
>> overriding
>> is disabled by default.
>>
>> Signed-off-by: Mark Rada <marada@uwaterloo.ca>
>
> O.K.  I think this patch now covers everything needed.  Well, except
> documentation.
>

Documentation documentation or code comments documentation?

Show 68 quoted lines

>> ---
>>  gitweb/gitweb.perl |   17 ++++++++++++-----
>>  1 files changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 3398163..a59569f 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -167,27 +167,31 @@ our %known_snapshot_formats = (
>
> At the beginning of %known_snapshot_formats definition there is a
> comment with description of structure of this hash.  You should have
> updated it, for example in the following way:
>
> @@ -168,7 +168,8 @@ our %known_snapshot_formats = (
> 	# 	'suffix' => filename suffix,
> 	# 	'format' => --format for git-archive,
> 	# 	'compressor' => [compressor command and arguments]
> -	# 	                (array reference, optional)}
> +	# 	                (array reference, optional),
> +	# 	'disabled' => boolean (optional)}
> 	#
> 	'tgz' => {
> 		'display' => 'tar.gz',
>
> The above is, of course, not the only possible way.
>
>>  		'type' => 'application/x-gzip',
>>  		'suffix' => '.tar.gz',
>>  		'format' => 'tar',
>> -		'compressor' => ['gzip']},
>> +		'compressor' => ['gzip'],
>> +		'disabled' => 0},
>>
>>  	'tbz2' => {
>>  		'display' => 'tar.bz2',
>>  		'type' => 'application/x-bzip2',
>>  		'suffix' => '.tar.bz2',
>>  		'format' => 'tar',
>> -		'compressor' => ['bzip2']},
>> +		'compressor' => ['bzip2'],
>> +		'disabled' => 0},
>>
>>  	'txz' => {
>>  		'display' => 'tar.xz',
>>  		'type' => 'application/x-xz',
>>  		'suffix' => '.tar.xz',
>>  		'format' => 'tar',
>> -		'compressor' => ['xz']},
>> +		'compressor' => ['xz'],
>> +		'disabled' => 0},
>>
>>  	'zip' => {
>>  		'display' => 'zip',
>>  		'type' => 'application/x-zip',
>>  		'suffix' => '.zip',
>> -		'format' => 'zip'},
>> +		'format' => 'zip',
>> +		'disabled' => 0},
>>  );
>
> Although I though that we don't need to put "'disabled' => 0", as
> the fact that 'disabled' key does not exist means that it is enabled.
>
> But if you chose to have all known formats not disabled, then I think
> it is a correct solution.  (And then choosing between 'enabled' and
> 'disabled' is mainly a matter of taste.)
>

I think spelling it out explicitly in each case makes things more clear, but would it not increase the memory footprint ever so slightly (I'm not familiar with PERL internals and whether having a field in one hash entry means it will have memory allocated in all the rest). I'm also not convinced that worrying about this micro-optimization is worth it.

Show 31 quoted lines

>>
>>  # Aliases so we understand old gitweb.snapshot values in repository
>> @@ -502,7 +506,8 @@ sub filter_snapshot_fmts {
>>  		exists $known_snapshot_format_aliases{$_} ?
>>  		       $known_snapshot_format_aliases{$_} : $_} @fmts;
>>  	@fmts = grep {
>> -		exists $known_snapshot_formats{$_} } @fmts;
>> +		exists $known_snapshot_formats{$_} &&
>> +		!$known_snapshot_formats{$_}{'disabled'}} @fmts;
>>  }
>>
>>  our $GITWEB_CONFIG = $ENV{'GITWEB_CONFIG'} || "++GITWEB_CONFIG++";
>> @@ -5171,6 +5176,8 @@ sub git_snapshot {
>>  		die_error(400, "Unknown snapshot format");
>>  	} elsif (!grep($_ eq $format, @snapshot_fmts)) {
>>  		die_error(403, "Unsupported snapshot format");
>> +	} elsif ($known_snapshot_formats{$format}{'disabled'}) {
>> +		die_error(403, "Snapshot format not allowed");
>>  	}
>>
>>  	if (!defined $hash) {
>
> Now I think that covers everything: preventing displaying known but
> disabled snapshot formats in snapshots links in gitweb, and preventing
> using known but disabled snapshot format.
>
> -- 
> Jakub Narebski
>
> Git User's Survey 2009
> http://tinyurl.com/GitSurvey2009

-- Mark A Rada (ferrous26) marada@uwaterloo.ca