Overlay diff in annotate view shows wrong file

[Remy Blank]

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.

[Christian Boos]

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?

[Remy Blank]

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

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

[Carsten Klein <carsten.klein@…>]

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.

[Carsten Klein <carsten.klein@…>]

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?

[Christian Boos]

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

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

See comment:1.

[Carsten Klein <carsten.klein@…>]

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"

[Christian Boos]

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

@@ -17 +17 @@
       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 )
+        if ( sep > 0 ) {
           path = href.slice(sep+1);
-        else
+          if (reponame)
+            path = path.substr(reponame.length);
+        } else
           path = original_path;
       }

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.

[Carsten Klein <carsten.klein@…>]

I thought so, hence the comment ;)

[Christian Boos]

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.

[Christian Boos]

Well, it still doesn't work in all situations: source:sandbox/multirepos/trac/versioncontrol/web_ui/browser.py?rev=9125&annotate=blame#L727

[Christian Boos]

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.

[Christian Boos]

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…

[Christian Boos]

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