Versioncontrol browser: "export:" links formatted incorrectly

[David Avsajanishvili <avsd05@…>]

As a result of changeset:9125/trunk/trac/versioncontrol/web_ui/browser.py the "export:" browser link does not work correctly anymore, because following line has been changed:

source:trunk/trac/versioncontrol/web_ui/browser.py@9125#L727​ _format_export_link() method:

(a) old vs. (b) new

@@ -621 +725 @@
             path, rev = export.split('@', 1)
         else:
-            rev, path = self.env.get_repository().youngest_rev, export
+            rev, path = '', export
         return tag.a(label, class_='export',
                      href=formatter.href.export(rev, path) + fragment)

As a result, following kind of links are formed:

​http://trac.edgewall.org/export//trunk/README

I have fixed this issue in my environment by returning old line back, and it worked! No other problems appeared!

Just don't understand, why this line was changed…

[Remy Blank]

This looks weird indeed. Thanks for reporting the issue and fix.

[Remy Blank]

For reference, here's a non-working link: export:trunk/README

The links seem to work with non-default repositories. More investigation needed.

[Christian Boos]

The rev as an empty string '' is supposed to be an alias for youngest rev (see Repository.normalize_rev), and it's used that way in BrowserModule.process_request, so in theory, it should work.

In my test environment served by tracd, export links like export:trunk/README work.

Here on t.e.o, lighty sees the correct URL: "GET /export//trunk/README HTTP/1.1", but Trac not, it gets path_info: u'/export/trunk/README', hence path @ rev becomes u'trunk' @ u'README'.

So it looks like we can't rely on using '//' in URLs, as some web front ends seem to normalize them into a single '/'.

We could instead set rev to some special value indicative of "latest". Maybe standardize on HEAD? ("latest" itself could be a branch name in DVCS, so not a good idea).

[Christian Boos]

(and URL encoding // would be also be fragile, see ​http://httpd.apache.org/docs/2.2/mod/core.html#allowencodedslashes).

[David Avsajanishvili <avsd05@…>]

In my installation these "double-slash links" also work, but IT IS CACHED BY BROWSER! Once a revision of file is changed, the web-browser still shows old one from cache, without any notification. I couldn't find a way to get correct link on the last revision, that is not affected by caching, unless manually typing the revision number between the slashes.

So, the old approach, when revision number was automatically placed in URL explicitly, probably, is better. Or, at least, this behaviour should be configurable.

Replying to cboos:

The rev as an empty string '' is supposed to be an alias for youngest rev (see Repository.normalize_rev), and it's used that way in BrowserModule.process_request, so in theory, it should work.

In my test environment served by tracd, export links like export:trunk/README work.

Here on t.e.o, lighty sees the correct URL: "GET /export//trunk/README HTTP/1.1", but Trac not, it gets path_info: u'/export/trunk/README', hence path @ rev becomes u'trunk' @ u'README'.

So it looks like we can't rely on using '//' in URLs, as some web front ends seem to normalize them into a single '/'.

We could instead set rev to some special value indicative of "latest". Maybe standardize on HEAD? ("latest" itself could be a branch name in DVCS, so not a good idea).

[Christian Boos]

Replying to David Avsajanishvili <avsd05@…>:

I couldn't

find a way to get correct link on the last revision, that is not affected by caching

Correct, if we specify HEAD as the revision, then the browser shouldn't cache the page and should always try to get the content for the latest revision.

So, the old approach, when revision number was automatically placed in URL explicitly, probably, is better.

I don't see how… if we output the revision number, then it is appropriate to make the browser cache it, as you'll be trying to retrieve the file at that same revision again, hence with the same content.

[David Avsajanishvili <avsd05@…>]

In fact, the only problem is that browser always tries to cache the export link. So, once I have clicked "Download in other formats → Original format" link when browsing the source, the Web-browser downloads the file and CACHES IT! If I submit some changes of this in SVN, and return to source browsing page again and click on "Original format" link, the Web-browser shows me cached version of the file, rather than downloading fresh one from the server!

I use Trac to store PDF files and view them directly by the web-browser, by setting render_unsafe_content to True. So, in Trac-dev012 my users often get unexpected cached version of PDF file, instead of expecting fresh one. That's why I had to rollback this line of code in my Trac installation.

If you could help avoiding this caching - it would be nice!

[Christian Boos]

To sum up:

• normalize on 'HEAD' meaning youngest rev
• ensure we send the appropriate Cache-Control headers

[Remy Blank]

Implemented in [9588].

[Remy Blank]

OT: Weird, any changes to this ticket seem to trigger the message "The ticket validation has failed", without there being any warnings at the top of the page. Other tickets don't show this issue. I wonder where this comes from…

[Christian Boos]

Previewing a change, either as anonymous or when logged in, triggers a validation error.

Looking in the Trac log, I see:

[pid  1207519568] 2010-04-29 07:01:58,624 Trac[api] WARNING: HTML preview using <trac.mimeview.patch.PatchRenderer object at 0x7fc5fc09f6d0> failed (
Traceback (most recent call last):
  File "build/bdist.linux-x86_64/egg/trac/mimeview/api.py", line 761, in render
    rendered_content, filename, url)
  File "build/bdist.linux-x86_64/egg/trac/mimeview/patch.py", line 54, in render
    raise TracError(_('Invalid unified diff content'))
TracError: Le contenu n'est pas conforme au format diff unifié)

[Christian Boos]

Replying to cboos:

Previewing a change, …

Saving obviously worked ;-)

After "fixing" the patch in the description, previewing works as well.

[Remy Blank]

Right, I can now reproduce the issue here. Thanks!

[Remy Blank]

Ok, I know what happens. The patch renderer fails to render non-unified diffs, and raises an exception. That exception is caught​, and a warning is added with add_warning(). However, at the point where this happens, the warnings at the top of the page have already been rendered, so it doesn't appear there. OTOH, the warning above the ticket comment box hasn't been rendered yet, and it appears due to the presence of warnings in chrome.warnings.

I suggest removing the call to add_warning(), as the warning really isn't that helpful to the user, and it is logged for the admin anyway. Something along the lines of:

trac/mimeview/api.py

@@ -785 +785 @@
                     return tag.div(class_='code')(tag.pre(result)).generate()
 
             except Exception, e:
-                if context.req:
-                    from trac.web.chrome import add_warning
-                    add_warning(context.req,
-                        _("HTML preview using %(renderer)s failed (%(err)s)",
-                          renderer=renderer.__class__.__name__,
-                          err=exception_to_unicode(e)))
-                self.log.warning('HTML preview using %s failed (%s)',
-                        renderer, exception_to_unicode(e,traceback=True))
+                self.log.warning('HTML preview using %s failed: %s',
+                                 renderer.__class__.__name__,
+                                 exception_to_unicode(e, traceback=True))
 
     def _render_source(self, context, stream, annotations, marks=None):
         from trac.web.chrome import add_warning

Thoughts?

[Remy Blank]

Mmh… This was added on purpose in [9303]. I understand the motivation, but OTOH mimeview rendering can happen after the warnings are already rendered, so they wouldn't appear anyway.

[Christian Boos]

We could use a context hint, set it in the wiki Formatter:

self.context.set_hints(disable_warnings=True)

and the test would become:

if context.req and not context.get_hint('disable_warnings'):
    ...

[Remy Blank]

Where would you set the context hint? Rendering using mimeview can happen anywhere wiki formatting is rendered, so that would be many places. Shouldn't we rather disable the warnings by default, and enable them only when rendering e.g. an attachment or a source file?

Oh, you meant disable the warnings any time we render through a Formatter? So this essentially has the same effect. I'll try that and report back.

[Remy Blank]

The following patch seems to work. I create a nested context in Formatter.__init__() to avoid changing the context passed as an argument.

trac/mimeview/api.py

@@ -785 +785 @@
                     return tag.div(class_='code')(tag.pre(result)).generate()
 
             except Exception, e:
-                if context.req:
+                self.log.warning('HTML preview using %s failed: %s',
+                                 renderer.__class__.__name__,
+                                 exception_to_unicode(e, traceback=True))
+                if context.req and not context.get_hint('disable_warnings'):
                     from trac.web.chrome import add_warning
                     add_warning(context.req,
                         _("HTML preview using %(renderer)s failed (%(err)s)",
                           renderer=renderer.__class__.__name__,
                           err=exception_to_unicode(e)))
-                self.log.warning('HTML preview using %s failed (%s)',
-                        renderer, exception_to_unicode(e,traceback=True))
 
     def _render_source(self, context, stream, annotations, marks=None):
         from trac.web.chrome import add_warning

trac/wiki/formatter.py

@@ -351 +351 @@
     def __init__(self, env, context):
         """Note: `req` is still temporarily used."""
         self.env = env
-        self.context = context
+        self.context = context()
+        self.context.set_hints(disable_warnings=True)
         self.req = context.req
         self.href = context.href
         self.resource = context.resource

Is this what you had in mind?

[Christian Boos]

Yes!

Please also add a note about the hint in render's doc.

[Remy Blank]

Patch applied in [9642], with the requested documentation.