From: Jakub Narebski <jnareb@gmail.com>
Date: Sat, 11 Aug 2007 19:57:31 GMT
Subject: Re: [PATCH] Advertise per branch RSS/Atom feeds
Message-ID: <200708112157.31764.jnareb@gmail.com>
In-Reply-To: <7vps1ummqy.fsf@assigned-by-dhcp.cox.net>

On Sat, 11 August 2007, Junio C Hamano wrote:

> The feeder code had provisions to accept 'h' parameter to
> specify which branch to send the feed data from, but there was
> no link that advertised this capability on the pages.
> 
> This adds h parameter to the footer of shortlog/log page for the
> branch.

And also of 'history' and 'tree' views; see comments below.

Besides, that is only half of the places where we put links to
RSS/Atom feeds. There are RSS/Atom feeds in the page header,
in the form of "<link .../>" elements; but that is perhaps for
separate patch. I think for example that we should add more
specialized feeds to HTML header instead of replacing generic
feeds by specialized ones like this patch does in footer.

>  	if (defined $project) {
>  		my $descr = git_get_project_description($project);
> +		my @tips = ();
> +		if ($hash !~ /^[0-9a-fA-F]{40}$/) {
> +			@tips = (hash => $hash);
> +		}

I'd use hash instead of array (list) here, c.f. %hash_base trick,
i.e. "my %tips" and "%tips = (hash => $hash);". Just for code
consistency and slightly better readibility.

By the way, the above would work also for 'history' and 'tree'
views if h parameter is not literal hash (perhaps we should exclude
"HEAD" as well, by the way), and we can have path limited feeds,
then perhaps it would be even better to use

			%tips = (hash=>$hash, file_name=>$file_name);

I'm also not sure if %tips (or @tips) is best name for this variable,
but I don't have better proposal. %narrow perhaps or something like
that.

>  		if (defined $descr) {
>  			print "<div class=\"page_footer_text\">" . esc_html($descr) . "</div>\n";
>  		}
> -		print $cgi->a({-href => href(action=>"rss"),
> +		print $cgi->a({-href => href(action=>"rss", @tips),
>  		              -class => "rss_logo"}, "RSS") . " ";
> -		print $cgi->a({-href => href(action=>"atom"),
> +		print $cgi->a({-href => href(action=>"atom", @tips),
>  		              -class => "rss_logo"}, "Atom") . "\n";
>  	} else {
>  		print $cgi->a({-href => href(project=>undef, action=>"opml"),
> 

And perhaps we should add title attribute explaining that feed is for
given branch... or perhaps not.

-- 
Jakub Narebski
Poland