Defect #11612

Revision graph sometimes broken due to raphael.js error

Added by Daniel Ritz over 5 years ago. Updated over 4 years ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Jean-Philippe Lang% Done:

0%

Category:SCM
Target version:2.1.0
Resolution:Fixed Affected version:2.0.3

Description

In most Git repositories here, the revision graph is sometimes not completely rendered, some (or even most) lines and dots are missing. I tried it in FF 14 and Safari 6 with the same result.

Firefox error console shows this:

Timestamp: 2012-08-09 11:03:52 
Error: TypeError: b[0] is undefined
Source File: http://naft02.ch.alcatel-lucent.com/redmine/javascripts/raphael.js?1342787109
Line: 7

Since revision_graph.js does not use any of the deprecated API of Raphaël 2.1, I tried updating Raphaël:

cd public/javascripts
mv raphael.js{,.old}
wget -O raphael.js https://raw.github.com/DmitryBaranovskiy/raphael/master/raphael-min.js

This completely fixes the problem.


Related issues

Related to Redmine - Defect #14116: Don't render dot for commit which have no space property ... New

Associated revisions

Revision 10369
Added by Jean-Philippe Lang about 5 years ago

Revision graph sometimes broken (#11612).

History

#1 Updated by Daniel Ritz over 5 years ago

Spoke too soon. Found another case where things are not drawn. Turns out the problem is the missing "space" property of some of the commits. I'll check the root case later. Meanwhile the quick fix in revision_graph.js is this:

diff --git a/public/javascripts/revision_graph.js b/public/javascripts/revision_graph.js
index 31aacd8..92fb7ab 100644
--- a/public/javascripts/revision_graph.js
+++ b/public/javascripts/revision_graph.js
@@ -41,6 +41,9 @@ function drawRevisionGraph(holder, commits_hash, graph_space) {

     commits.each(function(commit) {

+        if (!commit.hasOwnProperty("space"))
+            commit.space = 0;
+
         y = commit_table_rows[max_rdmid - commit.rdmid].getLayout().get('top') - graph_y_offset + CIRCLE_INROW_OFFSET;
         x = graph_x_offset + XSTEP / 2 + XSTEP * commit.space;

@@ -55,6 +58,9 @@ function drawRevisionGraph(holder, commits_hash, graph_space) {
             parent_commit = commits_by_scmid.get(parent_scmid);

             if (parent_commit) {
+                if (!parent_commit.hasOwnProperty("space"))
+                    parent_commit.space = 0;
+
                 parent_y = commit_table_rows[max_rdmid - parent_commit.rdmid].getLayout().get('top') - graph_y_offset + CIRCLE_INROW_OFFSET;
                 parent_x = graph_x_offset + XSTEP / 2 + XSTEP * parent_commit.space;

#2 Updated by Jean-Philippe Lang about 5 years ago

  • Status changed from New to Resolved
  • Assignee set to Jean-Philippe Lang
  • Target version set to 2.1.0
  • Resolution set to Fixed

Patch applied in r10369. Thanks.

#3 Updated by Etienne Massip about 5 years ago

Not sure forcing the space to 0 has no side-effect, the problem might also be in the commit data.

#4 Updated by Jean-Philippe Lang about 5 years ago

The space property is supposed to be numeric so setting it to 0 if it's undefined can't be bad.

#5 Updated by Jean-Philippe Lang about 5 years ago

  • Status changed from Resolved to Closed

Merged.

#6 Updated by Etienne Massip about 5 years ago

Jean-Philippe Lang wrote:

The space property is supposed to be numeric so setting it to 0 if it's undefined can't be bad.

IIRC it means forcing the position of the commit on the first displayed branch which is not necessarily correct, that's all my concern.

As Daniel says, having this property unset probably hides some deeper cause.

#7 Updated by Jean-Philippe Lang about 5 years ago

As Daniel says, having this property unset probably hides some deeper cause.

Sure, but his patch fixes his problem. Just some kind of workaround until someone actually fixes the root cause.

#8 Updated by John Kubiatowicz about 5 years ago

I am seeing this problem on 1.4 stable branch as well.

Any chance of a back-ported fix?

#9 Updated by Jens Krämer over 4 years ago

We recently had the same problem - graph rendering broke completely for a repository due to JS errors because some commits were missing the space property.

Turns out that on a fresh install, where the repository in question was re-imported, the problem didn't occur, because the problematic commits didnt show up at all. Further investigation showed that those commits were basically abandoned and not connected to any branch.

I think not rendering these commits in the graph at all is much better than putting them randomly on the first branch as the patch above does.

Here's the patch for 1.4:

--- a/public/javascripts/revision_graph.js
+++ b/public/javascripts/revision_graph.js
@@ -41,6 +41,10 @@ function drawRevisionGraph(holder, commits_hash, graph_space) {

     commits.each(function(commit) {

+      if (typeof commit.space != 'undefined') {
         y = commit_table_rows[max_rdmid - commit.rdmid].getLayout().get('top') - graph_y_offset + CIRCLE_INROW_OFFSET;
         x = graph_x_offset + XSTEP / 2 + XSTEP * commit.space;

@@ -95,6 +99,7 @@ function drawRevisionGraph(holder, commits_hash, graph_space) {
         }

         top.push(revision_dot_overlay);
+      }
     });

     top.toFront();

#10 Updated by Etienne Massip over 4 years ago

Jens Krämer wrote:

We recently had the same problem - graph rendering broke completely for a repository due to JS errors because some commits were missing the space property.

Could you please open a new issue for this?

#11 Updated by Toshi MARUYAMA over 4 years ago

Etienne Massip wrote:

Jens Krämer wrote:

We recently had the same problem - graph rendering broke completely for a repository due to JS errors because some commits were missing the space property.

Could you please open a new issue for this?

No need.
r10369 is not in version 1.4.x.

#12 Updated by Toshi MARUYAMA over 4 years ago

Version 1.4.x Prototype and version >= 2.1 jQuery are incompatible.

#13 Updated by Etienne Massip over 4 years ago

Toshi MARUYAMA wrote:

No need.
r10369 is not in version 1.4.x.

?

As I stated in #11612#note-6, r10369 is not a good fix.

It seems that Jens found the true reason of #11612 so he should open an issue for trunk to deal with undefined space property correctly, that is probably more like the way Jens suggested rather than erroneously attaching suspicious commits to first branch.

I didn't say the patch is ready to be applied to trunk, I just say we need to have a correct fix for this issue which is closed and fixed with a released version and, as such, can't be reopen.

#14 Updated by Etienne Massip over 4 years ago

Opened #14116.

Also available in: Atom PDF