Modify ConvenientLine to work with unicode

John Haley · John Haley · commit 74c19f86eb39 · 2015-12-28T09:54:44.000-07:00
This changes how `ConvenientLines` are calculated with a breaking API change.

`ConvenientLine` used to assume that all content coming back was ASCII characters and
a uniform byte length. This is no longer the case. Now we use the byte length coming
back from libgit2 to extract the string from a buffer using the correct byte length. This
will give back the correct line everytime.

The breaking API change is that we no longer return the new line character with the line
nor do we include that character in the `contentLen` calculation either. I'm on the fence
about that change but it seems like it's what we ought to be doing instead of force all of
the consumers of the library to account for that everywhere.
diff --git a/lib/convenient_line.js b/lib/convenient_line.js
@@ -3,59 +3,63 @@ var NodeGit = require("../");
 function ConvenientLine(raw, i) {
   this.raw = raw;
   this.i = i;
+  this._cache = {};
 }
 
 /**
-* The content of the line
-* @return {String}
+* The type of the line (context, addition, deletion, etc...)
+* @return {Diff.LINE}
 */
 ConvenientLine.prototype.origin = function() {
  return this.raw.origin();
 };
 
 /**
-* The content of the line
-* @return {String}
+* Line number in old file or -1 for added line
+* @return {Number}
 */
 ConvenientLine.prototype.oldLineno = function() {
  return this.raw.oldLineno();
 };
 
 /**
-* The content of the line
-* @return {String}
+* Line number in new file or -1 for deleted line
+* @return {Number}
 */
 ConvenientLine.prototype.newLineno = function() {
  return this.raw.newLineno();
 };
 
 /**
-* The content of the line
-* @return {String}
+* Number of newline characters in content
+* @return {Number}
 */
 ConvenientLine.prototype.numLines = function() {
  return this.raw.numLines();
 };
 
 /**
-* The content of the line
-* @return {String}
+* Number of characters in the string
+* @return {Number}
 */
 ConvenientLine.prototype.contentLen = function() {
- return this.raw.contentLen();
-};
+  if (!this._cache.contentLen) {
+    this._cache.contentLen = this.content().length;
+  }
 
+  return this._cache.contentLen;
+};
 
 /**
-* The content of the line
-* @return {String}
+* Offset in the original file to the content
+* @return {Number}
 */
 ConvenientLine.prototype.contentOffset = function() {
  return this.raw.contentOffset();
 };
 
 /**
-* The content of the line
+* Pointer to diff text, not NUL-byte terminated
 * @return {String}
 */
 ConvenientLine.prototype.rawContent = function() {
@@ -67,8 +71,14 @@ ConvenientLine.prototype.rawContent = function() {
 * @return {String}
 */
 ConvenientLine.prototype.content = function() {
-  return this.raw.content()
-    .substring(0, this.raw.contentLen()).replace("\n", "");
+  if (!this._cache.content) {
+    this._cache.content = new Buffer(this.raw.content())
+      .slice(0, this.raw.contentLen())
+      .toString("utf8")
+      .replace("\n", "");
+  }
+
+  return this._cache.content;
 };
 
 NodeGit.ConvenientLine = ConvenientLine;
diff --git a/test/tests/commit.js b/test/tests/commit.js
@@ -559,30 +559,30 @@ describe("Commit", function() {
       //check all hunk lines
       assert.equal(lines.length, 12);
       assert.equal(lines[0].origin(), Diff.LINE.CONTEXT);
-      assert.equal(lines[1].contentLen(), 9);
+      assert.equal(lines[1].contentLen(), 8);
 
-      assert.equal(getLineText(lines[1]), "line   s\n");
+      assert.equal(getLineText(lines[1]), "line   s");
       assert.equal(lines[1].origin(), Diff.LINE.CONTEXT);
       assert.equal(lines[2].origin(), Diff.LINE.CONTEXT);
 
-      assert.equal(lines[3].contentLen(), 1);
-      assert.equal(getLineText(lines[3]), "\n");
+      assert.equal(lines[3].contentLen(), 0);
+      assert.equal(getLineText(lines[3]), "");
       assert.equal(lines[3].origin(), Diff.LINE.ADDITION);
 
       assert.equal(lines[4].origin(), Diff.LINE.CONTEXT);
 
-      assert.equal(lines[5].contentLen(), 7);
-      assert.equal(getLineText(lines[5]), "line v\n");
+      assert.equal(lines[5].contentLen(), 6);
+      assert.equal(getLineText(lines[5]), "line v");
       assert.equal(lines[5].origin(), Diff.LINE.DELETION);
-      assert.equal(lines[6].contentLen(), 8);
-      assert.equal(getLineText(lines[6]), "line v1\n");
+      assert.equal(lines[6].contentLen(), 7);
+      assert.equal(getLineText(lines[6]), "line v1");
       assert.equal(lines[6].origin(), Diff.LINE.ADDITION);
 
       assert.equal(lines[7].origin(), Diff.LINE.CONTEXT);
       assert.equal(lines[8].origin(), Diff.LINE.CONTEXT);
 
-      assert.equal(lines[9].contentLen(), 4);
-      assert.equal(getLineText(lines[9]), "\t\t\t\n");
+      assert.equal(lines[9].contentLen(), 3);
+      assert.equal(getLineText(lines[9]), "\t\t\t");
       assert.equal(lines[9].origin(), Diff.LINE.ADDITION);
 
       assert.equal(lines[10].origin(), Diff.LINE.CONTEXT);
diff --git a/test/tests/convenient_line.js b/test/tests/convenient_line.js
@@ -0,0 +1,69 @@
+var assert = require("assert");
+var repoSetup = require("../utils/repository_setup");
+var promisify = require("promisify-node");
+var fse = promisify(require("fs-extra"));
+var path = require("path");
+var local = path.join.bind(path, __dirname);
+
+describe("ConvenientLine", function() {
+  var repoPath = local("../repos/convenientLineTest");
+  var unicodeLine = "Ťḥ𝖎ṧ ℓỈ𝓃ệ çǒ𝚗ẗảḭṋṦ Û𝐧ǐ𝗰ṓḍ𝔢";
+  var asciiLine = "but this line doesn't";
+
+  beforeEach(function() {
+    var test = this;
+
+    return repoSetup.createRepository(repoPath)
+      .then(function(repo) {
+        return repoSetup.commitFileToRepo(
+          repo,
+          "fileWithUnicodeChars",
+          unicodeLine + "\n" + asciiLine + "\n"
+        );
+      })
+      .then(function(commit) {
+        return commit.getDiff();
+      })
+      .then(function(diff) {
+        return diff[0].patches();
+      })
+      .then(function(patches) {
+        return patches[0].hunks();
+      })
+      .then(function(hunks) {
+        return hunks[0].lines();
+      })
+      .then(function(lines) {
+        test.unicodeLine = lines[0];
+        test.asciiLine = lines[1];
+      });
+  });
+
+  after(function() {
+    return fse.remove(repoPath);
+  });
+
+  it("can parse the byte length of a unicode string", function() {
+    var line = this.unicodeLine;
+
+    assert.equal(line.contentLen(), 32);
+  });
+
+  it("can get a line that contains unicode", function() {
+    var line = this.unicodeLine;
+
+    assert.equal(line.content(), unicodeLine);
+  });
+
+  it("can parse the byte length of a ascii string", function() {
+    var line = this.asciiLine;
+
+    assert.equal(line.contentLen(), 21);
+  });
+
+  it("can get a line that contains ascii", function() {
+    var line = this.asciiLine;
+
+    assert.equal(line.content(), asciiLine);
+  });
+});
diff --git a/test/tests/diff.js b/test/tests/diff.js
@@ -114,17 +114,17 @@ describe("Diff", function() {
         assert.equal(lines[2].origin(), Diff.LINE.CONTEXT);
 
         var oldContent = "__Before submitting a pull request, please ensure " +
-          "both unit tests and lint checks pass.__\n";
-        assert.equal(lines[3].rawContent(), oldContent);
+          "both unit tests and lint checks pass.__";
+        assert.equal(lines[3].content(), oldContent);
         assert.equal(lines[3].origin(), Diff.LINE.DELETION);
-        assert.equal(lines[3].contentLen(), 90);
+        assert.equal(lines[3].contentLen(), oldContent.length);
 
         var newContent = "__Before submitting a pull request, please ensure " +
           "both that you've added unit tests to cover your shiny new code, " +
-          "and that all unit tests and lint checks pass.__\n";
-        assert.equal(lines[4].rawContent(), newContent);
+          "and that all unit tests and lint checks pass.__";
+        assert.equal(lines[4].content(), newContent);
         assert.equal(lines[4].origin(), Diff.LINE.ADDITION);
-        assert.equal(lines[4].contentLen(), 162);
+        assert.equal(lines[4].contentLen(), newContent.length);
       });
   });
 
