Edgewall Software
Modify

Opened 10 years ago

Closed 10 years ago

#9236 closed defect (fixed)

Overlay diff in annotate view shows wrong file

Reported by: Remy Blank Owned by: Christian Boos
Priority: high Milestone: 0.12
Component: version control/browser Version: 0.12dev
Severity: minor Keywords: blame
Cc: Branch:
Release Notes:
API Changes:

Description

Go to the following annotated file:

http://trac.edgewall.org/browser/trunk/trac/ticket/default_workflow.py?annotate=blame&rev=9268

Then click on the revision of the first line in the "Rev" column ([3025]). The overlaid changeset shows the diff for the file trunk/trac/ticket/api.py instead of trunk/trac/ticket/default_workflow.py. Other lines don't show a diff at all.

Attachments (0)

Change History (14)

comment:1 by Christian Boos, 10 years ago

I believe it's working fine for the first situation, as trac/ticket/default_workflow.py was copied from trac/ticket/api.py, so at [3025], api.py should be shown (log:trunk/trac/ticket/default_workflow.py).

For the lines that don't show a diff, there might indeed be a problem, as sometimes I also get this impression, but that could also be a changeset where there was a propchange… So for which lines/revisions does it happen?

in reply to:  1 comment:2 by Remy Blank, 10 years ago

Replying to cboos:

I believe it's working fine for the first situation, as trac/ticket/default_workflow.py was copied from trac/ticket/api.py, so at [3025], api.py should be shown (log:trunk/trac/ticket/default_workflow.py).

Oh, right, I didn't think of this situation.

For the lines that don't show a diff, there might indeed be a problem, as sometimes I also get this impression, but that could also be a changeset where there was a propchange… So for which lines/revisions does it happen?

It seems that this happens for all lines with revisions after the rename, for example, line 25 ([5731]), line 28 ([8638]), line 67 ([6873]). The copy was done in [5441].

comment:3 by Christian Boos, 10 years ago

Owner: set to Christian Boos

Ah, yes, there is a problem indeed. Didn't look as closely as needed on the second line already, r7890: it does show a diff, but with api.py and that's the wrong file at that time.

comment:4 by Carsten Klein <carsten.klein@…>, 10 years ago

Seems like the algorithm used just picks the first file in the changeset and never matches the file currently viewed?

I.e. if there are more than one file listed in the changeset, it will presumably take the first that was retrieved from the database cache (natural ordering?).

If there happens to be only one file in the changeset, and this is the file that is currently viewed, then, naturally, the correct file's diff will be displayed.

comment:5 by Carsten Klein <carsten.klein@…>, 10 years ago

In blame.js:

4 window.enableBlame = function(url, reponame, original_path) {
5 var message = null;
6 var message_rev = null;
7
8 /* for each blame cell... */
9 var path = null;
10 $("table.code th.blame").each(function() {
11 // determine path from the changeset link
12 var a = $(this).find("a");
13 var href = a.attr("href");
14 if ( ! (href || path) )
15 return; // was "Rev" column title
16
17 if ( href ) {
18 a.removeAttr("href");
19 href = href.slice(href.indexOf("changeset/") + 10);
20 if (reponame)
21 href = href.substr(reponame.length);
22 var sep = href.indexOf("/");
23 if ( sep > 0 )
24 path = href.slice(sep+1);
25 else
26 path = original_path;
27 } 

Takes the path associated with the changeset, which happens to be the api.py module.

Perhaps changing this so that it will always use the original_path instead would suffice to make it work?

Last edited 10 years ago by Christian Boos (previous) (diff)

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

Replying to Carsten Klein <carsten.klein@…>:

Takes the path associated with the changeset, which happens to be the api.py module.

See comment:1.

comment:7 by Carsten Klein <carsten.klein@…>, 10 years ago

This would represent an early attempt to solve the problem, however, I am sure that it would break with something else, would it not?

Index: htdocs/js/blame.js
===================================================================
--- htdocs/js/blame.js	(Revision 54)
+++ htdocs/js/blame.js	(Arbeitskopie)
@@ -16,14 +16,7 @@
       
       if ( href ) {
         a.removeAttr("href");
-        href = href.slice(href.indexOf("changeset/") + 10);
-        if (reponame)
-            href = href.substr(reponame.length);
-        var sep = href.indexOf("/");
-        if ( sep > 0 )
-          path = href.slice(sep+1);
-        else
-          path = original_path;
+        path = original_path;
       }
 
       // determine rev from th class, which is of the form "blame r123"

comment:8 by Christian Boos, 10 years ago

The problem was indeed located in this part of the code, but getting away with the problem using a simple path = original_path replacement would have been too easy, don't you think? ;-)

  • trac/htdocs/js/blame.js

     
    1717      if ( href ) {
    1818        a.removeAttr("href");
    1919        href = href.slice(href.indexOf("changeset/") + 10);
    20         if (reponame)
    21             href = href.substr(reponame.length);
    2220        var sep = href.indexOf("/");
    23         if ( sep > 0 )
     21        if ( sep > 0 ) {
    2422          path = href.slice(sep+1);
    25         else
     23          if (reponame)
     24            path = path.substr(reponame.length);
     25        } else
    2626          path = original_path;
    2727      }

As some of the revisions might correspond to changes made under a different name, we need to retrieve that path from the href to the restricted changeset. Only, the fix I did for handling multiple repositories was completely buggy (well, the breakage possibly happened during a merge, r7680).

More testing needed, along with some additional unicode related fixes.

comment:9 by Carsten Klein <carsten.klein@…>, 10 years ago

I thought so, hence the comment ;)

comment:10 by Christian Boos, 10 years ago

Keywords: blame added
Resolution: fixed
Status: newclosed

Should be fixed by r9491.

Finally, I detected that the annotation for Mercurial files is also broken in case the file has been renamed, but this will hopefully be fixed indirectly by #9230.

comment:11 by Christian Boos, 10 years ago

Milestone: 0.120.12.1
Resolution: fixed
Status: closedreopened

comment:12 by Christian Boos, 10 years ago

It looks like it's in case of a single repository, the XHR's URL is <cset number>/changeset, i.e. "changeset" is mistaken for the reponame.

comment:13 by Christian Boos, 10 years ago

Scratch that. It was simpler.

The path was correctly computed but the scoping was wrong, the same path variable was used for all callbacks, hence blame was only working for the revisions having the same path as the last row…

comment:14 by Christian Boos, 10 years ago

Milestone: 0.12.10.12
Resolution: fixed
Severity: normalminor
Status: reopenedclosed

Yay, it seems it works now. Fixed in r9664.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Christian Boos.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Christian Boos 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.