Re: [PATCH] gitweb: Refactor git_header_html

From: Jakub Narebski <jnareb@gmail.com>
Date: Jun 22, 2011, 06:44 UTC
Message-ID: <201106220844.35431.jnareb@gmail.com>
In-Reply-To: <7v4o3i7nkt.fsf@alter.siamese.dyndns.org>

On Wed, 22 Jun 2011, Junio C Hamano wrote:

Show 18 quoted lines

> 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.

Well, originally this style was used mainly to pass some extra rarely set boolean flags; isn't

        foo($bar, "baz", -quux => 1, -corge => 1);

better than

        foo($bar, "baz", 1, 0, 1);

when it is ordinarily (most times) called as

        foo($bar, "baz");

When writing in C, one would probably use #defines for this:

        foo(bar, "baz", FOO_QUUX | FOO_CORGE);

but it is not very Perl-ish, I would think.

Show 27 quoted lines

> > +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.

Hmmm... perhaps then I should have gone with my first thought, namely passing $status to print_header_links().

Will resend.

Note that in case of print_nav_breadcrumbs() the code used %opts originally, so it is only passed from git_header_html...

Show 8 quoted lines

> > ...
> > -		}
> > -	}
> > -	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.

Thanks.

-- 
Jakub Narebski
Poland