Edgewall Software
Modify

Opened 15 years ago

Last modified 13 months ago

#8639 new defect

"View latest revision" links to non-existent (404) page

Reported by: Mitar Owned by:
Priority: normal Milestone: next-stable-1.6.x
Component: version control/browser Version: 0.11.4
Severity: minor Keywords: newcache performance
Cc: mmitar@…, Ryan J Ollos Branch:
Release Notes:
API Changes:
Internal Changes:

Description

"View latest revision" links to non-existent (404) page in repository browser for files which do not exist anymore in current/latest revision. This makes a lot of 404 errors when robots are crawling Trac and is otherwise not nice to have links to non-existent pages.

I would propose replacing this link with a static message "Non-existent in latest revision".

Attachments (0)

Change History (16)

comment:1 by Ryan Ollos <ryano@…>, 15 years ago

Cc: ryano@… added

comment:2 by Remy Blank, 15 years ago

Milestone: 0.12.1
Owner: set to Remy Blank

The usual way of displaying this is to have the link grayed out and disabled, but still with the same text, in this case "View latest revision". The hint for the link could be "Doesn't exist in the latest revision".

Assigning to 0.12.1, as it's not critical. If I find time, I'll put it into 0.12.

comment:3 by Remy Blank, 14 years ago

The same issue exists with the "Next Revision" link. The patch below fixes the issue, by removing the "Latest Revision" link if the file doesn't exist in the youngest revision, and by graying the "Next Revision" link if the file was removed in the next revision.

  • trac/versioncontrol/web_ui/browser.py

    diff --git a/trac/versioncontrol/web_ui/browser.py b/trac/versioncontrol/web_ui/browser.py
    a b  
    427427                prev_rev = repos.previous_rev(rev=node.rev,
    428428                                              path=node.created_path)
    429429                if prev_rev:
    430                     href = req.href.browser(reponame,
    431                                             node.created_path, rev=prev_rev)
     430                    href = req.href.browser(reponame, node.created_path,
     431                                            rev=prev_rev)
    432432                    add_link(req, 'prev', href,
    433433                             _('Revision %(num)s', num=display_rev(prev_rev)))
    434                 if rev is not None:
     434                if rev is not None and repos.has_node(node.created_path):
    435435                    add_link(req, 'up', req.href.browser(reponame,
    436436                                                         node.created_path))
    437437                next_rev = repos.next_rev(rev=node.rev,
    438438                                          path=node.created_path)
    439                 if next_rev:
     439                if next_rev and repos.has_node(node.created_path, next_rev):
    440440                    href = req.href.browser(reponame, node.created_path,
    441441                                            rev=next_rev)
    442442                    add_link(req, 'next', href,

Unfortunately, the current implementation of prevnext_nav() doesn't allow specifying a tooltip for disabled items, and neither does it allow having a grayed "up" link. Is this good enough, or should I extend prevnext_nav()?

comment:4 by Christian Boos, 14 years ago

I'm not sure about the fix for "View Latest Revision", as ideally you should go to the … latest revision, i.e. the one just before the delete. By going to the "not existing" page, at least you can find from there the latest existing following the search for previous… link (ignoring for a moment that this operation is slow …).

comment:5 by Remy Blank, 14 years ago

So the idea would be to find the last revision before the delete, and use that for the "Latest Revision" link? This would indeed be a logical functionality.

Isn't that a very inefficient operation? If that's the case, we should probably use a kind of "tag" as a revision, and only compute it when actually using the link.

in reply to:  5 comment:6 by Mitar, 14 years ago

Replying to rblank:

So the idea would be to find the last revision before the delete, and use that for the "Latest Revision" link? This would indeed be a logical functionality.

Yes. It is also one thing which bothers me all the time. Especially when people move code around, refactor it and similar. It is really hard to find out where the file has moved. I would be for such behavior. Maybe we could add a warning on top "In repository's latest revision this file does not exist anymore. This is last existing revision."

in reply to:  5 ; comment:7 by Christian Boos, 14 years ago

Replying to rblank:

Isn't that a very inefficient operation?

Well, in svn_fs.py yes, but that would be very easy to implement in cache.py (can't believe I was so lazy to do it, given the number of times it annoyed me over the years. This and get_path_history.

If that's the case, we should probably use a kind of "tag" as a revision, and only compute it when actually using the link.

That's a very good idea as well. We should have several of those actually: @123+latest for latest revision, path@123+next for next revision on path (we had a discussion about that in a ticket which I can't find anymore), and even for the eligible stuff (#8459), perhaps also for a "next revisions" link in the log (#4760).

in reply to:  7 comment:8 by Remy Blank, 14 years ago

Replying to cboos:

We should have several of those actually: @123+latest for latest revision, path@123+next for next revision on path (we had a discussion about that in a ticket which I can't find anymore), and even for the eligible stuff (#8459), perhaps also for a "next revisions" link in the log (#4760).

The only drawback is that it doesn't give us the information whether the referenced changeset actually exists or not.

comment:9 by Christian Boos, 13 years ago

Keywords: newcache added
Milestone: next-minor-0.12.x0.12.4

While having a very efficient way to compute the next/prev link could probably only be done with a reworked cache, using relative links in the meantime would be a good idea (e.g. ?version=123&rel=next), the downside of not having the grayed out link if the next or prev revision doesn't exist would be largely compensated by the speed-up, which can be very significative.

comment:10 by Christian Boos, 12 years ago

Keywords: performance added

comment:11 by Christian Boos, 12 years ago

Milestone: 0.12.4next-stable-1.0.x

Ok, not for 0.12.x anymore…

comment:13 by Ryan J Ollos, 10 years ago

Cc: Ryan J Ollos added; ryano@… removed

comment:14 by Ryan J Ollos, 9 years ago

Owner: Remy Blank removed

comment:15 by Ryan J Ollos, 8 years ago

Milestone: next-stable-1.0.xnext-stable-1.2.x

Moved ticket assigned to next-stable-1.0.x since maintenance of 1.0.x is coming to a close. Please move the ticket back if it's critical to fix on 1.0.x.

comment:16 by Ryan J Ollos, 4 years ago

Milestone: next-stable-1.2.xnext-stable-1.4.x

comment:17 by Ryan J Ollos, 13 months ago

Milestone: next-stable-1.4.xnext-stable-1.6.x

Milestone renamed

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


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