From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 21 Jun 2011 23:48:50 GMT
Subject: Re: [PATCH] gitweb: Refactor git_header_html
Message-ID: <7v4o3i7nkt.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20110621183550.9315.21922.stgit@localhost.localdomain>

Jakub Narebski <jnareb@gmail.com> writes:

> P.S. I wonder if print_search_form() should also get passed %opts as
> one of its parameters...

I do not particularly care for this:

	sub foo {
        	my %opts = @_;

		if ($opts{-option} eq 'bar') {
                	... do this ...

style, which lets a caller pass any random typo to the callee, like so:

	foo(-optoin => 'value');

But maybe it is just me.

> +sub print_header_links {
> +	my %opts = @_;
> +
> +	# print out each stylesheet that exist, providing backwards capability
> +	# for those people who defined $stylesheet in a config file
> +	if (defined $stylesheet) {
> +		print '<link rel="stylesheet" type="text/css" href="'.esc_url($stylesheet).'"/>'."\n";
> +	} else {
> +		foreach my $stylesheet (@stylesheets) {
> +			next unless $stylesheet;
> +			print '<link rel="stylesheet" type="text/css" href="'.esc_url($stylesheet).'"/>'."\n";
> +		}
> +	}
> +	if ($opts{'-feed'}) {
> +		print_feed_meta();
> +	}

Here, we refrain from showing the <link> elements that are related to the
RSS/Atom feeds (if we know what $project we are talking about), or <link>
elements that are related to the project list (html top page and opml) if
we are not showing a normal return, as $opts{'-feed'} is true only when we
are returning '200 OK'.

The original implementation, even though it used statement modifiers after
this print_feed_meta(), was much easier to follow, at least for me. The
association between "feed-meta" and "200 OK" was close together and more
direct.

> ...
> -		}
> -	}
> -	print_feed_meta()
> -		if ($status eq '200 OK');

Other than that, this looks a correct no-op patch that makes the resulting
smaller functions easier to grok.