[PATCH] hgweb: add RSS and Atom subscribe buttons to the main paper template pages

[Matt Mackall]

On Tue, 2012-12-04 at 23:44 +0100, Angel Ezquerra wrote:
> On Tue, Dec 4, 2012 at 9:47 PM, Matt Mackall <mpm at selenic.com> wrote:
> > On Tue, 2012-12-04 at 09:45 +0100, Angel Ezquerra wrote:
> >> On Tue, Dec 4, 2012 at 9:32 AM, Matt Mackall <mpm at selenic.com> wrote:
> >> > On Tue, 2012-12-04 at 00:41 +0100, Angel Ezquerra wrote:
> >> >> # HG changeset patch
> >> >> # User Angel Ezquerra <angel.ezquerra at gmail.com>
> >> >> # Date 1354578089 -3600
> >> >> # Node ID 62f1adcd73969f967098c08000ca50f129bcfd59
> >> >> # Parent  41d3a5a1c8495cd599e5b422fddcdf93609b8101
> >> >> hgweb: add RSS and Atom subscribe buttons to the main paper template pages
> >> >>
> >> >> The buttons are found at the bottom of each page. They are similar to the
> >> >> monoblue template buttons, but their colors have been changed to better match
> >> >> the patper black, grey and white color palette of the paper style.
> >> >
> >> > Three things I would do differently:
> >> >
> >> > a) use the standard icon (ie from feedicons.com)
> >> > b) link to only the Atom feed
> >> > c) put the icon under the menu on the left
> >> >
> >> > Once upon a time it made sense to offer two types of feeds: when clients
> >> > might only have RSS support, but you had content that would actually
> >> > work better with Atom.
> >> >
> >> > But every client has supported atom feeds for years now (anyone have a
> >> > counterexample?), so we should simply use it. We're syndicating HTML, so
> >> > Atom is technically more suited. Also, Atom is an IETF standard rather
> >> > than.. well, go read about RSS versions. Adding two links to paper that
> >> > basically do the same thing is just clutter. (Alternately, if people
> >> > REALLY think there's a problem with Atom, then we should just advertise
> >> > the RSS feeds.)
> >> >
> >> > If at some point we switch the other styles over to use the icon, we
> >> > should just use one icon there as well. The old RSS subscriptions should
> >> > continue to work, of course.
> >>
> >> I think this is a good idea. Actually I also thought that having 2
> >> buttons is a little uncommon, since most pages only show a single
> >> "feed button". However  I decided to follow the example of the
> >> existing buttons on the monoblue theme since I did not know which feed
> >> is the best of the two.
> >>
> >> Would it make sense to add a config option that let you choose which
> >> type of feed you want to advertise on this new, single button (the
> >> default would be atom as you suggest)?
> >
> > Do you have some reason to think that someone will ever have a good
> > reason to care? I do not. Let's wait for that person to show up and
> > complain instead.
> >
> >> The only issue I see with using the atom feed by default is that I
> >> could not get the atom-branches feed (which I added on the other patch
> >> I sent to the list yesterday) to work. If anyone can help me with that
> >> it would be awesome.
> >
> > Make sure you:
> >
> > a) have an appropriate entry in the map
> > b) restart your server
> > c) use force-reload to bypass HTTP caching
> 
> Actually I believe that the existing tags atom feed does not work
> either, or at least it does not work for me. The feed is empty.
> 
> If I modify the existing atom/tags.tmpl file and I remove the
> "{latestentry%feedupdated}" line then it works fine. The same thing
> happens for the new atom/branches.tmpl file that I made.
> 
> I don't know why this can be, since other atom templates have that
> line as well and they do work fine...

There is indeed something broken here, at some point after 2.2. Bisect
points to:

The first bad revision is:
changeset:   17261:c0068b058fcd
branch:      stable
parent:      17255:3e856d8abe9c
user:        Patrick Mezard <patrick at mezard.eu>
date:        Fri Jul 27 17:48:49 2012 +0200
summary:     webcommands: do not modify repo.tagslist()

Removing this line (not present in the RSS version of these feeds):

 {latestentry%feedupdated}

..makes it work again.

The problem is here:

http://www.selenic.com/hg/file/c0068b058fcd/mercurial/hgweb/webcommands.py#l396

On line 397, we declare an iterator i over the list of tags. This used
in the entries() local function.  And on lines 415, 416, and 417 we pass
three different ways to look at the entries on the template. 

Here's the trouble, then: when we use latestentry, we exhaust the
iterator, so it's empty by the time we use entriesnotip. This didn't
happen before the above patch because it wasn't an iterator. 

To fix it, we need to go back to having an actual list, but this time,
it needs to be a private copy to avoid the issue the patch tried to fix:

diff -r e7a35c4b6c21 mercurial/hgweb/webcommands.py
--- a/mercurial/hgweb/webcommands.py	Tue Dec 04 12:54:18 2012 -0800
+++ b/mercurial/hgweb/webcommands.py	Tue Dec 04 17:23:37 2012 -0600
@@ -399,7 +399,7 @@
                 branches=webutil.nodebranchdict(web.repo, ctx))
 
 def tags(web, req, tmpl):
-    i = reversed(web.repo.tagslist())
+    i = list(reversed(web.repo.tagslist()))
     parity = paritygen(web.stripecount)
 
     def entries(notip=False, limit=0, **map):

I'll queue up a fix for this and some related iterator-abuse.

-- 
Mathematics is the supreme nostalgia of our time.