Damaged graph

[a-n-dokuchaev]

• Can you reproduce the bug at https://try.gitea.io:
  • Yes (URL)
  • No
  • Not relevant

Description

We have damaged graph on some projects. This is very disturbing. Thanks.

Screenshots

[lunny]

Could you give more details?

[a-n-dokuchaev]

The graph is disappearing after doing some merges or pushing local branches. It's the only thing we know. We found 5/100+ of our repos with the same state as on the try.gitea.io.

[zeripath]

Looking at the browser inspector console I see:

jquery.js?v=798fb62373ca0ec978de1f0f5e229f0f:1 jQuery.Deferred exception: Cannot read property 'color' of undefined TypeError: Cannot read property 'color' of undefined
    at https://try.gitea.io/js/gitgraph.js:1:4466
    at n (https://try.gitea.io/js/gitgraph.js:1:4703)
    at https://try.gitea.io/js/index.js?v=798fb62373ca0ec978de1f0f5e229f0f:1:67748
    at l (https://try.gitea.io/js/index.js?v=798fb62373ca0ec978de1f0f5e229f0f:1:255165)
    at Generator._invoke (https://try.gitea.io/js/index.js?v=798fb62373ca0ec978de1f0f5e229f0f:1:254918)
    at Generator.forEach.e.<computed> [as next] (https://try.gitea.io/js/index.js?v=798fb62373ca0ec978de1f0f5e229f0f:1:255522)
    at n (https://try.gitea.io/js/index.js?v=798fb62373ca0ec978de1f0f5e229f0f:1:553357)
    at s (https://try.gitea.io/js/index.js?v=798fb62373ca0ec978de1f0f5e229f0f:1:553568) undefined
C.Deferred.exceptionHook @ jquery.js?v=798fb62373ca0ec978de1f0f5e229f0f:1
c @ jquery.js?v=798fb62373ca0ec978de1f0f5e229f0f:1
(anonymous) @ jquery.js?v=798fb62373ca0ec978de1f0f5e229f0f:1
Promise.then (async)
l @ jquery.js?v=798fb62373ca0ec978de1f0f5e229f0f:1
c @ jquery.js?v=798fb62373ca0ec978de1f0f5e229f0f:1
setTimeout (async)
(anonymous) @ jquery.js?v=798fb62373ca0ec978de1f0f5e229f0f:1
u @ jquery.js?v=798fb62373ca0ec978de1f0f5e229f0f:1
fireWith @ jquery.js?v=798fb62373ca0ec978de1f0f5e229f0f:1
fire @ jquery.js?v=798fb62373ca0ec978de1f0f5e229f0f:1
u @ jquery.js?v=798fb62373ca0ec978de1f0f5e229f0f:1
fireWith @ jquery.js?v=798fb62373ca0ec978de1f0f5e229f0f:1
ready @ jquery.js?v=798fb62373ca0ec978de1f0f5e229f0f:1
_ @ jquery.js?v=798fb62373ca0ec978de1f0f5e229f0f:1
jquery.js?v=798fb62373ca0ec978de1f0f5e229f0f:1 Uncaught TypeError: Cannot read property 'color' of undefined
    at gitgraph.js:1
    at n (gitgraph.js:1)
    at gitgraph.js:12
    at l (runtime.js:45)
    at Generator._invoke (runtime.js:274)
    at Generator.forEach.e.<computed> [as next] (runtime.js:97)
    at n (asyncToGenerator.js:3)
    at s (asyncToGenerator.js:25)

[zeripath]

running with: make watch-frontend in the console I see:

jquery.js?v=d6ca3c6360663dfd3a697d1a6e4ed792:4150 Uncaught TypeError: Cannot read property 'color' of undefined
    at draw (gitgraph.js:519)
    at gitGraph (gitgraph.js:557)
    at _callee$ (to-absolute-index.js:11)
    at tryCatch (to-absolute-index.js:11)
    at Generator.invoke [as _invoke] (to-absolute-index.js:11)
    at Generator.prototype.<computed> [as next] (to-absolute-index.js:11)
    at asyncGeneratorStep (to-absolute-index.js:11)
    at _next (to-absolute-index.js:11)

Line 519 (to the browser) appears to match up with:

gitea/web_src/js/vendor/gitgraph.js

Line 380 in d31a9c5

color = flows[colomnIndex].color;

[zeripath]

It appears the problem is that the git graph is not in the format that this js function seems to expect.

The js seems to expect the use of underscores and not hyphens...

If I bodge in code to handle hyphens the problem goes away.

Now I'm not sure why this is different but my build of git 1.7.2 appears to use hyphens so perhaps this has always been broken?!

---

Edit:

I suspect it has

bluef/gitgraph.js#6

[ahttvy]

Another damaged graph: https://try.gitea.io/ahaha/gitgraph/graph

[a-n-dokuchaev]

Hello! 2/5 of our repos still damaged. @ahttvy may be right

Another damaged graph: https://try.gitea.io/ahaha/gitgraph/graph

[zeripath]

sigh...

[zeripath]

OK so that's a different but related issue - here's another bugfix... I suspect this code needs to be completely rewritten.

[a-n-dokuchaev]

The last one - https://try.gitea.io/ahaha/qtconnectivity/graph

[zeripath]

Ugh this one is a bit more difficult. I'll have to take a deeper look to understand what the code is doing.

[zeripath]

The below patch will prevent the graph from erroring out but doesn't fix the underlying problem:

diff --git a/web_src/js/vendor/gitgraph.js b/web_src/js/vendor/gitgraph.js
index 0cf5d0f75..7a5777598 100644
--- a/web_src/js/vendor/gitgraph.js
+++ b/web_src/js/vendor/gitgraph.js
@@ -376,7 +376,18 @@ export default function gitGraph(canvas, rawGraphList, config) {
           flows.splice(colomnIndex, 0, genNewFlow());
         }
 
-        color = flows[colomnIndex].color;
+        if (colomnIndex < flows.length) {
+          color = flows[colomnIndex].color;
+        } else {
+          console.error({
+            message: "Missing flow!",
+            colomnIndex,
+            colomn,
+            prevColomn,
+            flowSwapPos,
+          });
+          color = "#000";
+        }
 
         switch (colomn) {
           case '-':

[zeripath]

OK I think this 3rd problem is sufficiently difficult to solve that it might be that I just have to rewrite this code.

[zeripath]

So I think I will have to rewrite the gitgraph.js to look something like this to fix this final problem:

Broken JS

/* This is a customized version of https://github.com/bluef/gitgraph.js/blob/master/gitgraph.js
   Changes include conversion to ES6 and linting fixes */

/*
 * @license magnet:?xt=urn:btih:c80d50af7d3db9be66a4d0a86db0286e4fd33292&dn=bsd-3-clause.txt BSD 3-Clause
 * Copyright (c) 2011, Terrence Lee <kill889@gmail.com>
 * All rights reserved.
 *
 * Redistribution and use in source and binary forms, with or without
 * modification, are permitted provided that the following conditions are met:
 *     * Redistributions of source code must retain the above copyright
 *       notice, this list of conditions and the following disclaimer.
 *     * Redistributions in binary form must reproduce the above copyright
 *       notice, this list of conditions and the following disclaimer in the
 *       documentation and/or other materials provided with the distribution.
 *     * Neither the name of the <organization> nor the
 *       names of its contributors may be used to endorse or promote products
 *       derived from this software without specific prior written permission.
 *
 * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
 * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
 * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
 * DISCLAIMED. IN NO EVENT SHALL <COPYRIGHT HOLDER> BE LIABLE FOR ANY
 * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
 * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
 * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
 * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
 * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
 * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 */

export default function gitGraph(canvas, rawGraphList, config) {
  if (!canvas.getContext) {
    return;
  }

  if (typeof config === 'undefined') {
    config = {
      unitSize: 20,
      lineWidth: 3,
      nodeRadius: 4
    };
  }

  const nextFlows = [];
  const graphList = [];

  const ctx = canvas.getContext('2d');

  const devicePixelRatio = window.devicePixelRatio || 1;
  const backingStoreRatio = ctx.webkitBackingStorePixelRatio
                          || ctx.mozBackingStorePixelRatio
                          || ctx.msBackingStorePixelRatio
                          || ctx.oBackingStorePixelRatio
                          || ctx.backingStorePixelRatio || 1;

  const ratio = devicePixelRatio / backingStoreRatio;

  const init = function () {
    let maxWidth = 0;
    let i;
    const l = rawGraphList.length;
    let row;
    let midStr;

    for (i = 0; i < l; i++) {
      midStr = rawGraphList[i].replace(/\s+/g, ' ').replace(/^\s+|\s+$/g, '');
      midStr = midStr.replace(/(--)|(-\.)/g,'-')
      maxWidth = Math.max(midStr.replace(/(_|\s)/g, '').length, maxWidth);

      row = midStr.split('');

      graphList.unshift(row);
    }

    const width = maxWidth * config.unitSize;
    const height = graphList.length * config.unitSize;

    canvas.width = width * ratio;
    canvas.height = height * ratio;

    canvas.style.width = `${width}px`;
    canvas.style.height = `${height}px`;

    ctx.lineWidth = config.lineWidth;
    ctx.lineJoin = 'round';
    ctx.lineCap = 'round';

    ctx.scale(ratio, ratio);
  };

  const genRandomStr = function () {
    const chars = '0123456789ABCDEF';
    const stringLength = 6;
    let randomString = '', rnum, i;
    for (i = 0; i < stringLength; i++) {
      rnum = Math.floor(Math.random() * chars.length);
      randomString += chars.substring(rnum, rnum + 1);
    }

    return randomString;
  };

  const findFlow = function (id) {
    let i = nextFlows.length;

    while (i-- && nextFlows[i].id !== id);

    return i;
  };

  const findColomn = function (symbol, row) {
    let i = row.length;

    while (i-- && row[i] !== symbol);

    return i;
  };

  const findBranchOut = function (row) {
    if (!row) {
      return -1;
    }

    let i = row.length;

    while (i--
      && !(row[i - 1] && row[i] === '/' && row[i - 1] === '|')
      && !(row[i - 2] && row[i] === '_' && row[i - 2] === '|'));

    return i;
  };

  const findLineBreak = function (row) {
    if (!row) {
      return -1;
    }

    let i = row.length;

    while (i--
    && !(row[i - 1] && row[i - 2] && row[i] === ' ' && row[i - 1] === '|' && row[i - 2] === '_'));

    return i;
  };

  const genNewFlow = function () {
    let newId;

    do {
      newId = genRandomStr();
    } while (findFlow(newId) !== -1);

    return { id: newId, color: `#${newId}` };
  };

  // Draw methods
  const drawLine = function (moveX, moveY, lineX, lineY, color) {
    ctx.strokeStyle = color;
    ctx.beginPath();
    ctx.moveTo(moveX, moveY);
    ctx.lineTo(lineX, lineY);
    ctx.stroke();
  };

  const drawLineRight = function (x, y, color) {
    drawLine(x, y + config.unitSize / 2, x + config.unitSize, y + config.unitSize / 2, color);
  };

  const drawLineUp = function (x, y, color) {
    drawLine(x, y + config.unitSize / 2, x, y - config.unitSize / 2, color);
  };

  const drawNode = function (x, y, color) {
    ctx.strokeStyle = color;

    drawLineUp(x, y, color);

    ctx.beginPath();
    ctx.arc(x, y, config.nodeRadius, 0, Math.PI * 2, true);
    ctx.fill();
  };

  const drawLineIn = function (x, y, color) {
    drawLine(x + config.unitSize, y + config.unitSize / 2, x, y - config.unitSize / 2, color);
  };

  const drawLineOut = function (x, y, color) {
    drawLine(x, y + config.unitSize / 2, x + config.unitSize, y - config.unitSize / 2, color);
  };

  const draw = function (graphList) {
    let x, y, i;

    y = (canvas.height / ratio) - config.unitSize * 0.5;

    let flows = new Array(graphList[0].length);

    // Generate nextFlows for the first row - this is guaranteed to be: "(\| )*(\*)( \|)*"
    for (i = 0; i < graphList[0].length; i++) {
      if (graphList[0][i] === ' ') {
        continue;
      }
      flows[i] = genNewFlow();
    }

    // Draw the first row
    for (let idx = 0; idx < graphList[0].length; idx++) {
      x = config.unitSize * 0.5 * (idx + 1);
      const column = graphList[0][idx];
      switch (column) {
        case '*':
          drawNode(x, y, flows[idx].color);
          break;
        case ' ':
          break;
        case '|':
          drawLineUp(x, y, flows[idx].color);
          break;
        default:
          console.error(`Unexpected symbol in first row[${idx}] = '${column}'`);
      }
    }

    for (i = 1; i < graphList.length; i++) {
      // Done previous row - step up the row
      y -= config.unitSize;
      const currentRow = graphList[i];
      const prevRow = graphList[i-1];

      let nextFlows = new Array(currentRow.length);
      
      for (let idx = 0; idx < currentRow.length; idx++) {
        switch (currentRow[idx]) {
          case '|':
          case '*':
            if (idx+1 < currentRow.length &&
                (currentRow[idx+1] === '/' || currentRow[idx+1] === '_') && 
                idx < prevRow.length && 
                (prevRow[idx] === '|' || prevRow[idx] === '*') && 
                prevRow[idx-1] === '/' ) {
              // If     ' |/' keep the '|' flow
              //        '/|'

              nextFlows[idx] = flows[idx];

            } else if (idx+1 < currentRow.length && 
                (currentRow[idx+1] === '/' || currentRow[idx+1] === '_') && 
                idx -1 < prevRow.length &&
                prevRow[idx-1] === '/') {
              // If     ' |/' take the '/' flow
              //        '/ ?'

              nextFlows[idx] = flows[idx-1];
              flows[idx-1] = genNewFlow();

            } else if (idx - 1 < prevRow.length &&
              prevRow[idx-1] === '/') {
              // if ' |' take the '/' flow
              //    '/ '
            
              nextFlows[idx] = flows[idx-1];
              flows[idx-1] = genNewFlow();
            
            } else if (idx < prevRow.length &&
                (prevRow[idx] === '|' || prevRow[idx] === '*')) {
              // If '|' OR '|' take the '|' flow
              //    '|'    '*'

              nextFlows[idx] = flows[idx];
              flows[idx] = genNewFlow();
          
            } else if (idx +1 < prevRow.length &&
                prevRow[idx+1] === '\\') {
              // If '| ' keep the '\' flow
              //    ' \'

              nextFlows[idx] = flows[idx + 1];

            } else {
              // else just create a new flow - probably this is an error... 
              nextFlows[idx] = genNewFlow();
            }
            break;

          case '/':
            if (idx > 0 &&
                currentRow[idx-1] === '_') {
              // if      '_/' keep the '_' flow

              nextFlows[idx] = nextFlows[idx-1];

            } else if (idx > 1 &&
                (currentRow[idx-1] === '|' || currentRow[idx-1] === '*') &&
                currentRow[idx-2] === '_') {
              // If '_|/' keep the '_' flow

              nextFlows[idx] = nextFlows[idx-2];

            } else if (i > 1 &&
                currentRow[idx-1] === '|' &&
                idx-2 < prevRow.length &&
                prevRow[idx-2] === '/') {
              // If '?|/'  keep the '/' flow - ('|' will have caused regeneration as necessary)
              //    '/??'

              nextFlows[idx] = flows[idx-2];

            } else if (idx > 0 &&
                currentRow[idx-1] === ' ' &&
                idx-1 < prevRow.length &&
                prevRow[idx-1] === '/') {
              // If ' /'  - keep the '/' flow, but transform to '|' due to our spacing
              //    '/ '

              nextFlows[idx] = flows[idx-1];           
              currentRow[idx] = '|';

            } else if (idx > 0 &&
                currentRow[idx-1] === ' ' &&
                idx-1 < prevRow.length &&
                prevRow[idx-1] === '|') {
              // If ' /'  - keep the '|' flow
              //    '| '

              nextFlows[idx] = flows[idx-1];           

            } else if (idx < prevRow.length &&
                prevRow[idx] === '\\') {
              // If '/' <- Not sure this ever happens... but keep the '\' flow
              //    '\'

              nextFlows[idx] = flows[idx];

            } else {
              // Otherwise just generate a new flow and wait for bug-reports...

              nextFlows[idx] = genNewFlow();
            }
            break;

          case '\\':
            if (prevRow[idx] === '/') {
              // If '\?' take the '/' flow
              //    '/?'
              
              nextFlows[idx] = flows[idx];
              flows[idx] = genNewFlow();

            } else if (idx+1 < prevRow.length &&
                prevRow[idx+1] === '\\') {
              // If '\?' take the '\' flow and reassign to |
              //    ' \'

              nextFlows[idx] = flows[idx+1];
              currentRow[idx] = '|';
              flows[idx+1] = genNewFlow(); // <- prevents us having to think too hard about '\|/' 

            } else if (idx+1 < prevRow.length &&
                (prevRow[idx+1] === '|' || prevRow[idx+1] === '*')) {
              // If '\?' take the '|' flow
              //    ' |'

              nextFlows[idx] = flows[idx+1];
              flows[idx+1] = genNewFlow(); // <- prevents us having to think too hard about '\|/' 

            } else {
              // Otherwise just generate a new flow and wait for bug-reports...

              nextFlows[idx] = genNewFlow();
            }
            break;

          case '_':
            if (idx > 0 &&
                currentRow[idx-1] === '_') {
              // if '__' keep the '_' flow

              nextFlows[idx] = nextFlows[idx-1];
  
            } else if (idx > 1 &&
                currentRow[idx-1] === '|' &&
                currentRow[idx-2] === '_') {
              // if '_|_' keep the '_' flow 

              nextFlows[idx] = nextFlows[idx-2];

            } else if (idx > 0 &&
                idx-1 < prevRow.length &&
                prevRow[idx-1] === '/') {
              // if ' _' - keep the '/' flow
              //    '/ '

              nextFlows[idx] = flows[idx-1];

            } else if (idx > 1 &&
                idx-2 < prevRow.length &&
                prevRow[idx-2] === '/') {
              // if ' |_' - keep the '/' flow
              //    '/? '
              
              nextFlows[idx] = flows[idx-2];

            } else {
              // There really shouldn't be another way of doing this - generate and wait for bug-reports...
 
              nextFlows[idx] = genNewFlow();
            }
            break;

          case '-': {
            // This is: ------.
            //               /|\

            // Find the end of the '-':
            let originalIdx = idx;
            for (;idx < currentRow.length && currentRow[idx] === '-'; idx++);

            if (currentRow[idx] !== '.') {
              // It really should end in a '.' but this one doesn't...
              // Step back
              idx--;
              // Generate a new flow and await bug-reports...
              nextFlows[idx] = genNewFlow();

              // Assign all of the '-' to the same flow.
              for (;originalIdx<idx;originalIdx++) {
                nextFlows[originalIdx] = nextFlows[idx];
              }
            } else {
              // We have a sensible ----.
              
              // Choose our flow either /, |, or \
              let flow;
              if (idx-1 < prevRow.length && prevRow[idx-1] == '/') {
                flow = flows[idx-1];
              } else if (idx < prevRow.length && prevRow[idx] == '|') {
                flow = flows[idx];
              } else if (idx+1 < prevRow.length && prevRow[idx] == '\\') {
                flow = flows[idx+1];
              } else {
                // Again unexpected so let's generate and wait the bug-report
                flow = genNewFlow();
              }
              
              // Assign all of the rest of the ----. to this flow.
              for (;originalIdx<idx+1;originalIdx++) {
                nextFlows[originalIdx] = flow;
              }
            }
          }
            break;
          
          case ' ':
            // spaces don't get no flows
            break;
          
          default:
            // Unexpected so let's generate a new flow and wait for bug-reports
            flows[idx] = genNewFlow();
        }
      }

      for (let idx = 0; idx < currentRow.length; idx++) {
        x = config.unitSize * 0.5 * (idx + 1);
        switch (currentRow[idx]) {
          case '-':
          case '.':
          case '_':
            drawLineRight(x - 0.5 *config.unitSize, y, nextFlows[idx].color);
            break;
          case '*':
            drawNode(x, y, nextFlows[idx].color);
            break;
          case '|':
            drawLineUp(x, y, nextFlows[idx].color);
            break;
          case '/':
            drawLineOut(x - 0.5 *config.unitSize, y, nextFlows[idx].color);
            break;
          case '\\':
            drawLineIn(x - 0.5 *config.unitSize, y, nextFlows[idx].color);
            break;
          case ' ':
            break;
          default:
            console.error('Unknown symbol', currentRow[idx], nextFlows[idx] ? nextFlows[idx].color: '');
        }
      }
      flows = nextFlows
    }
   };

  init();
  draw(graphList);
}
// @end-license

---

The above was an example not a complete fix.

[ahttvy]

On https://try.gitea.io/ahaha/glu/graph:

With new gitgraph.js:

[zeripath]

For anyone watching this I've just merged another rework of the GitGraph rendering that has moved its rendering to the server.