Re: [PATCH] Advertise per branch RSS/Atom feeds

From: Jakub Narebski <jnareb@gmail.com>
Date: Aug 11, 2007, 19:57 UTC
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:

Show 6 quoted lines

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

Show 6 quoted lines

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

Show 12 quoted lines

>  		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