Re: What's cooking in git.git (Aug 2009, #03; Thu, 20)

[Jakub Narebski <jnareb@gmail.com>]

On Fri, 21 Aug 2009, Mark A Rada wrote:
> On 20-Aug-09, at 10:48 PM, Junio C Hamano wrote:
> 
> > Should graduate to 'master' soon.
> >
> > * mr/gitweb-xz (2009-08-06) 3 commits
> >  (merged to 'next' on 2009-08-14 at b63b8e6)
> > + gitweb: add support for XZ compressed snapshots
> > + gitweb: update INSTALL regarding specific snapshot settings
> > + gitweb: support to globally disable a snapshot format
> >
> 
> 
> I never submitted any tests for the patch that adds global snapshot  
> disabling functionality to gitweb.
> 
> Jakub, I changed the gitweb_run routine to capture STDOUT as well, 
> is that ok?

I think it is a good addition to the gitweb tests in git testsuite,
as it might make finding cause of an error easier.

> Unless I missed a case, the tests show that the extra condition check  
> that was added in the git_snapshot() routine is never actually
> executed, because a disabled snapshot format is not added to
> @snapshot_fmts, which is checked first.
> 
> snippet:
> 5178     } elsif (!grep($_ eq $format, @snapshot_fmts)) {
> 5179         die_error(403, "Unsupported snapshot format");
> 5180     } elsif ($known_snapshot_formats{$format}{'disabled'}) {
> 5181         die_error(403, "Snapshot format not allowed");
> 5182     }
> 5183

So we did check if format is disable twice, once when creating
(generating @snapshot_fmts list, and once (I guess unnecessary;
@snapshot_fmts is generated after reading GITWEB_CONFIG file(s)) 
when checking if snapshot $format is on the list of possible 
(allowed) formats in the snipped above.

Am I right?  If it is so, the last part of above snipped wouldn't
be ever exercised, and is not necessary.

> ---
>   t/t9500-gitweb-standalone-no-errors.sh |   67 +++++++++++++++++++++++ 
> ++++++++-

Word-wrapped.

>   1 files changed, 66 insertions(+), 1 deletions(-)
> 
> diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb- 
> standalone-no-errors.sh
> index 6275181..9ce9667 100755
> --- a/t/t9500-gitweb-standalone-no-errors.sh
> +++ b/t/t9500-gitweb-standalone-no-errors.sh
> @@ -57,10 +57,11 @@ gitweb_run () {
>   	# we are interested only in properly formatted errors/warnings
>   	rm -f gitweb.log &&
>   	perl -- "$SCRIPT_NAME" \
> -		>/dev/null 2>gitweb.log &&
> +		>gitweb.output 2>gitweb.log &&
>   	if grep "^[[]" gitweb.log >/dev/null 2>&1; then false; else true; fi
> 
>   	# gitweb.log is left for debugging
> +	# gitweb.output is used to parse output
>   }

This is a good change.

> 
>   . ./test-lib.sh
> @@ -704,4 +705,68 @@ test_expect_success \
>   	 gitweb_run "p=.git;a=summary"'
>   test_debug 'cat gitweb.log'
> 
> +
> +#  
> ----------------------------------------------------------------------
> +# snapshot settings
> +
> +cat >>gitweb_config.perl <<EOF
> +
> +\$feature{'snapshot'}{'override'} = 0;
> +EOF

A trick: use '\EOF' and you don't need to escape $ against variable
expansion by shell.

  +cat >>gitweb_config.perl <<\EOF
  +
  +$feature{'snapshot'}{'override'} = 0;
  +EOF

> +
> +test_expect_success \
> +	'snapshots: tgz only default format enabled' \
> +	'gitweb_run "p=.git;a=snapshot;h=HEAD;sf=tgz" &&
> +	grep "Status: 200 OK" gitweb.output &&
> +	gitweb_run "p=.git;a=snapshot;h=HEAD;sf=tbz2" &&
> +	grep "403 - Unsupported snapshot format" gitweb.output &&
> +	gitweb_run "p=.git;a=snapshot;h=HEAD;sf=txz" &&
> +	grep "403 - Unsupported snapshot format" gitweb.output &&
> +	gitweb_run "p=.git;a=snapshot;h=HEAD;sf=zip" &&
> +	grep "403 - Unsupported snapshot format" gitweb.output'
> +test_debug 'cat gitweb.output'

I would prefer (but I do not require) that such test were placed in
separate file, named e.g. t9501-gitweb-check-http-status.sh, and leave
t9500-gitweb-standalone-no-errors.sh to be about checking for Perl 
errors and warnings only.  This would require to extract common 
infrastructure (gitweb_init, gitweb_run, checking prerequisites)
into t/lib-gitweb.sh (or t/gitweb-lib.sh ;-)).

-- 
Jakub Narebski
Poland