Merge lp:~mwhudson/loggerhead/bug-383631 into lp:loggerhead

[Michael Hudson-Doyle]

This branch fixes the linked bug, twice over:

1) It changes the URLs of the in-branch breadcrumbs to be based on paths, like everything else in Loggerhead has been for years.

2) It removes 'file_id' from the set of query arguments that the (disgusting, I'd forgotten how horrible this bit was) machinery used by context_url preserves, now that we never ever generate a url with a file_id argument.

Cheers,
mwh

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 1/8/2012 10:54 PM, Michael Hudson-Doyle wrote:
> Michael Hudson-Doyle has proposed merging
> lp:~mwhudson/loggerhead/bug-383631 into lp:loggerhead.
>
> Requested reviews: Loggerhead Reviewers (loggerhead-reviewers)
> Related bugs: Bug #383631 in loggerhead: "Breadcrumbs while
> annotating a file in head shows the most recent revision in the url
> instead of head" https://bugs.launchpad.net/loggerhead/+bug/383631
>
> For more details, see:
> https://code.launchpad.net/~mwhudson/loggerhead/bug-383631/+merge/87877
>
> This branch fixes the linked bug, twice over:
>
> 1) It changes the URLs of the in-branch breadcrumbs to be based on
> paths, like everything else in Loggerhead has been for years.
>
> 2) It removes 'file_id' from the set of query arguments that the
> (disgusting, I'd forgotten how horrible this bit was) machinery
> used by context_url preserves, now that we never ever generate a
> url with a file_id argument.
>
> Cheers, mwh

I love getting rid of file_id in the URLs, but will this break
existing URLs if people are saving them somewhere? ISTR some people
wanted a good way to point people at :head of a file like README, etc.

I especially like this:
- - 'file_id': inv.path2id('/'.join(dir_parts[:index + 1])),
+ 'path': '/'.join(dir_parts[:index + 1]),

Internally it was already doing paths, and forcing a translation to
file_id at this part.

Anyway, if it is incompatible, is there a way we could try to migrate
gracefully?

 review: needsinfo

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk8W3/UACgkQJdeBCYSNAAOPCQCfX+WJIFLOagJvmKJnLy+/tKXJ
snsAn0jlmnE6b3DHidXeMLPZLnOIUaP0
=UHk1
-----END PGP SIGNATURE-----

review: Needs Information

[Michael Hudson-Doyle (mwhudson)]

On Wed, 18 Jan 2012 16:06:29 +0100, John Arbash Meinel <email address hidden> wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 1/8/2012 10:54 PM, Michael Hudson-Doyle wrote:
> > Michael Hudson-Doyle has proposed merging
> > lp:~mwhudson/loggerhead/bug-383631 into lp:loggerhead.
> >
> > Requested reviews: Loggerhead Reviewers (loggerhead-reviewers)
> > Related bugs: Bug #383631 in loggerhead: "Breadcrumbs while
> > annotating a file in head shows the most recent revision in the url
> > instead of head" https://bugs.launchpad.net/loggerhead/+bug/383631
> >
> > For more details, see:
> > https://code.launchpad.net/~mwhudson/loggerhead/bug-383631/+merge/87877
> >
> > This branch fixes the linked bug, twice over:
> >
> > 1) It changes the URLs of the in-branch breadcrumbs to be based on
> > paths, like everything else in Loggerhead has been for years.
> >
> > 2) It removes 'file_id' from the set of query arguments that the
> > (disgusting, I'd forgotten how horrible this bit was) machinery
> > used by context_url preserves, now that we never ever generate a
> > url with a file_id argument.
> >
> > Cheers, mwh
>
> I love getting rid of file_id in the URLs, but will this break
> existing URLs if people are saving them somewhere? ISTR some people
> wanted a good way to point people at :head of a file like README, etc.

This branch doesn't change how any URLs are processed, it just changes
the ones which are generated on the page. It will change where the
links on pages accessed via ?file_id=xxx go to, but from ones that are
broken to ones that work, so I think that's probably a good thing :-)

Cheers,
mwh

=== modified file 'loggerhead/templates/breadcrumbs.pt'
--- loggerhead/templates/breadcrumbs.pt	2008-09-30 02:04:14 +0000
+++ loggerhead/templates/breadcrumbs.pt	2012-01-08 21:53:23 +0000
@@ -10,7 +10,7 @@
 
 <!-- Branch breadcrumbs (for the path within the branch) -->
 <span metal:define-macro="branch" class="breadcrumb">
-    <tal:block repeat="crumb branch_breadcrumbs">/<a tal:attributes="href python:url([crumb['suffix'], change.revno], file_id=crumb['file_id'])" tal:content="crumb/dir_name" tal:condition="not:repeat/crumb/end" /><span tal:replace="crumb/dir_name" tal:condition="repeat/crumb/end" /></tal:block>
+    <tal:block repeat="crumb branch_breadcrumbs">/<a tal:attributes="href python:url([crumb['suffix'], change.revno, crumb['path']])" tal:content="crumb/dir_name" tal:condition="not:repeat/crumb/end" /><span tal:replace="crumb/dir_name" tal:condition="repeat/crumb/end" /></tal:block>
 </span>
 
 <!-- Directory breadcrumbs (for the path leading up to the branch) -->
 
=== modified file 'loggerhead/util.py'
--- loggerhead/util.py	2011-09-08 00:33:28 +0000
+++ loggerhead/util.py	2012-01-08 21:53:23 +0000
@@ -226,7 +226,7 @@
 def html_escape(s):
     """Transform dangerous (X)HTML characters into entities.
 
-    Like cgi.escape, except also escaping " and '. This makes it safe to use
+    Like cgi.escape, except also escaping \" and '. This makes it safe to use
     in both attribute and element content.
 
     If you want to safely fill a format string with escaped values, use
@@ -484,7 +484,7 @@
     for index, dir_name in enumerate(dir_parts):
         inner_breadcrumbs.append({
             'dir_name': dir_name,
-            'file_id': inv.path2id('/'.join(dir_parts[:index + 1])),
+            'path': '/'.join(dir_parts[:index + 1]),
             'suffix': '/' + view,
         })
     return inner_breadcrumbs
@@ -555,8 +555,8 @@
 #         for re-ordering an existing page by different sort
 
 t_context = threading.local()
-_valid = ('start_revid', 'file_id', 'filter_file_id', 'q', 'remember',
-          'compare_revid', 'sort')
+_valid = (
+    'start_revid', 'filter_file_id', 'q', 'remember', 'compare_revid', 'sort')
 
 
 def set_context(map):