Redmine

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;

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

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

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.

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.

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

Any chance of a back-ported fix?

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();

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?

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.

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

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.

Opened #14116.