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

Angel Ezquerra angel.ezquerra at gmail.com
Wed Dec 5 01:07:30 CST 2012


On Wed, Dec 5, 2012 at 12:24 AM, Matt Mackall <mpm at selenic.com> wrote:
> 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.
>

Cool, I guess that the same happens for branches then, since it also
uses a generator, right? If so, will you fix that too?

If you do, I can resend my "hgweb: add branches RSS and Atom feeds"
patch adding the "{latestentry%feedupdated}" line, and updating the
commit message accordingly.

Cheers,

Angel


More information about the Mercurial-devel mailing list