Edgewall Software
Modify

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#9264 closed defect (fixed)

Versioncontrol browser: "export:" links formatted incorrectly

Reported by: David Avsajanishvili <avsd05@…> Owned by: Remy Blank
Priority: high Milestone: 0.12
Component: version control/browser Version: 0.12dev
Severity: critical Keywords: export file versioncontrol
Cc: avsd05@… Branch:
Release Notes:
API Changes:

Description (last modified by Christian Boos)

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

    a b  
    621725            path, rev = export.split('@', 1)
    622726        else:
    623             rev, path = self.env.get_repository().youngest_rev, export
     727            rev, path = '', export
    624728        return tag.a(label, class_='export',
    625729                     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…

Attachments (0)

Change History (21)

comment:1 by Remy Blank, 9 years ago

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

comment:2 by Remy Blank, 9 years ago

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

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

comment:3 by Christian Boos, 9 years ago

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

comment:4 by Christian Boos, 9 years ago

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

in reply to:  3 ; comment:5 by David Avsajanishvili <avsd05@…>, 9 years ago

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

in reply to:  5 comment:6 by Christian Boos, 9 years ago

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.

comment:7 by David Avsajanishvili <avsd05@…>, 9 years ago

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!

comment:8 by Christian Boos, 9 years ago

To sum up:

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

comment:9 by Remy Blank, 9 years ago

Resolution: fixed
Status: newclosed

Implemented in [9588].

comment:10 by Remy Blank, 9 years ago

Owner: set to Remy Blank

comment:11 by Remy Blank, 9 years ago

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…

comment:12 by Christian Boos, 9 years ago

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é)

in reply to:  12 comment:13 by Christian Boos, 9 years ago

Description: modified (diff)

Replying to cboos:

Previewing a change, …

Saving obviously worked ;-)

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

comment:14 by Remy Blank, 9 years ago

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

comment:15 by Remy Blank, 9 years ago

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

    diff --git a/trac/mimeview/api.py b/trac/mimeview/api.py
    a b  
    785785                    return tag.div(class_='code')(tag.pre(result)).generate()
    786786
    787787            except Exception, e:
    788                 if context.req:
    789                     from trac.web.chrome import add_warning
    790                     add_warning(context.req,
    791                         _("HTML preview using %(renderer)s failed (%(err)s)",
    792                           renderer=renderer.__class__.__name__,
    793                           err=exception_to_unicode(e)))
    794                 self.log.warning('HTML preview using %s failed (%s)',
    795                         renderer, exception_to_unicode(e,traceback=True))
     788                self.log.warning('HTML preview using %s failed: %s',
     789                                 renderer.__class__.__name__,
     790                                 exception_to_unicode(e, traceback=True))
    796791
    797792    def _render_source(self, context, stream, annotations, marks=None):
    798793        from trac.web.chrome import add_warning

Thoughts?

comment:16 by Remy Blank, 9 years ago

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

Last edited 9 years ago by Remy Blank (previous) (diff)

comment:17 by Christian Boos, 9 years ago

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'):
    ...

comment:18 by Remy Blank, 9 years ago

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.

comment:19 by Remy Blank, 9 years ago

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

    diff --git a/trac/mimeview/api.py b/trac/mimeview/api.py
    a b  
    785785                    return tag.div(class_='code')(tag.pre(result)).generate()
    786786
    787787            except Exception, e:
    788                 if context.req:
     788                self.log.warning('HTML preview using %s failed: %s',
     789                                 renderer.__class__.__name__,
     790                                 exception_to_unicode(e, traceback=True))
     791                if context.req and not context.get_hint('disable_warnings'):
    789792                    from trac.web.chrome import add_warning
    790793                    add_warning(context.req,
    791794                        _("HTML preview using %(renderer)s failed (%(err)s)",
    792795                          renderer=renderer.__class__.__name__,
    793796                          err=exception_to_unicode(e)))
    794                 self.log.warning('HTML preview using %s failed (%s)',
    795                         renderer, exception_to_unicode(e,traceback=True))
    796797
    797798    def _render_source(self, context, stream, annotations, marks=None):
    798799        from trac.web.chrome import add_warning
  • trac/wiki/formatter.py

    diff --git a/trac/wiki/formatter.py b/trac/wiki/formatter.py
    a b  
    351351    def __init__(self, env, context):
    352352        """Note: `req` is still temporarily used."""
    353353        self.env = env
    354         self.context = context
     354        self.context = context()
     355        self.context.set_hints(disable_warnings=True)
    355356        self.req = context.req
    356357        self.href = context.href
    357358        self.resource = context.resource

Is this what you had in mind?

comment:20 by Christian Boos, 9 years ago

Yes!

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

comment:21 by Remy Blank, 9 years ago

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Remy Blank to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.