Merge pull request #1262 from bakkerthehacker/fix_rename_diff

Fix diffs not appearing for renamed files

components/commit/commit.js

@@ -55,8 +55,8 @@ class CommitViewModel {
     this.authorDateFromNow(this.authorDate().fromNow());
     this.authorName(args.authorName);
     this.authorEmail(args.authorEmail);
-    this.numberOfAddedLines(args.fileLineDiffs.length > 0 ? args.fileLineDiffs[0][0] : 0);
-    this.numberOfRemovedLines(args.fileLineDiffs.length > 0 ? args.fileLineDiffs[0][1] : 0);
+    this.numberOfAddedLines(args.total.additions);
+    this.numberOfRemovedLines(args.total.deletions);
     this.fileLineDiffs(args.fileLineDiffs);
     this.isInited = true;
     this.commitDiff = ko.observable(components.create('commitDiff', {

components/commitdiff/commitdiff.html

@@ -19,7 +19,7 @@
 <div data-bind="foreach: commitLineDiffs" class="commitdiff">
   <div class="file">
     <div class="head commit-diff-filename" data-bind="click: fileNameClick">
-      <span data-bind="text: fileName"></span>
+      <span data-bind="text: displayName"></span>
       <span class="file-stats" data-bind="visible: added() != '-'">
         (
         <span data-bind="text: added"></span>

components/commitdiff/commitdiff.js

@@ -14,7 +14,6 @@ class CommitDiff {
     this.wordWrap = args.wordWrap = args.wordWrap || components.create('textdiff.wordwrap');
     this.whiteSpace = args.whiteSpace = args.whiteSpace || components.create('textdiff.whitespace');
 
-    args.fileLineDiffs.shift();  // remove first line that has "total"
     this.loadFileLineDiffs(args);
   }
 

components/commitdiff/commitlinediff.js

@@ -4,10 +4,12 @@ const programEvents = require('ungit-program-events');
 
 class CommitLineDiff {
   constructor(args, fileLineDiff) {
-    this.added = ko.observable(fileLineDiff[0]);
-    this.removed = ko.observable(fileLineDiff[1]);
-    this.fileName = ko.observable(fileLineDiff[2]);
-    this.fileType = fileLineDiff[3];
+    this.added = ko.observable(fileLineDiff.additions);
+    this.removed = ko.observable(fileLineDiff.deletions);
+    this.fileName = ko.observable(fileLineDiff.fileName);
+    this.oldFileName = ko.observable(fileLineDiff.oldFileName);
+    this.displayName = ko.observable(fileLineDiff.displayName);
+    this.fileType = fileLineDiff.type;
     this.isShowingDiffs = ko.observable(false);
     this.repoPath = args.repoPath;
     this.server = args.server;
@@ -21,6 +23,7 @@ class CommitLineDiff {
   getSpecificDiff() {
     return components.create(`${this.fileType}diff`, {
       filename: this.fileName(),
+      oldFilename: this.oldFileName(),
       repoPath: this.repoPath,
       server: this.server,
       sha1: this.sha1,

components/imagediff/imagediff.js

@@ -7,6 +7,7 @@ components.register('imagediff', args => new ImageDiffViewModel(args));
 class ImageDiffViewModel {
   constructor(args) {
     this.filename = args.filename;
+    this.oldFilename = args.oldFilename;
     this.repoPath = args.repoPath;
     this.isNew = ko.observable(false);
     this.isRemoved = ko.observable(false);
@@ -16,9 +17,9 @@ class ImageDiffViewModel {
       if (this.isRemoved()) return 'removed';
       return 'changed';
     });
-    const gitDiffURL = `${ungit.config.rootPath}/api/diff/image?path=${encodeURIComponent(this.repoPath())}&filename=${this.filename}&version=`;
-    this.oldImageSrc = gitDiffURL + (this.sha1 ? this.sha1 + '^' : 'HEAD');
-    this.newImageSrc = gitDiffURL + (this.sha1 ? this.sha1 : 'current');
+    const gitDiffURL = `${ungit.config.rootPath}/api/diff/image?path=${encodeURIComponent(this.repoPath())}`;
+    this.oldImageSrc = gitDiffURL + `&filename=${this.oldFilename}&version=${(this.sha1 ? this.sha1 + '^' : 'HEAD')}`;
+    this.newImageSrc = gitDiffURL + `&filename=${this.filename}&version=${(this.sha1 ? this.sha1 : 'current')}`;
     this.isShowingDiffs = args.isShowingDiffs;
     this.rightArrowIcon = octicons['arrow-right'].toSVG({ 'height': 100 });
     this.downArrowIcon = octicons['arrow-down'].toSVG({ 'height': 100 });

components/staging/staging.js

@@ -175,16 +175,16 @@ class StagingViewModel {
 
   setFiles(files) {
     const newFiles = [];
-    for(const file in files) {
-      let fileViewModel = this.filesByPath[file];
+    for(let fileStatus of Object.values(files)) {
+      let fileViewModel = this.filesByPath[fileStatus.fileName];
       if (!fileViewModel) {
-        this.filesByPath[file] = fileViewModel = new FileViewModel(this, file);
+        this.filesByPath[fileStatus.fileName] = fileViewModel = new FileViewModel(this, fileStatus.fileName, fileStatus.oldFileName, fileStatus.displayName);
       } else {
         // this is mainly for patching and it may not fire due to the fact that
         // '/commit' triggers working-tree-changed which triggers throttled refresh
         fileViewModel.diff().invalidateDiff();
       }
-      fileViewModel.setState(files[file]);
+      fileViewModel.setState(fileStatus);
       newFiles.push(fileViewModel);
     }
     this.files(newFiles);
@@ -324,12 +324,13 @@ class StagingViewModel {
 }
 
 class FileViewModel {
-  constructor(staging, name) {
+  constructor(staging, name, oldName, displayName) {
     this.staging = staging;
     this.server = staging.server;
     this.editState = ko.observable('staged'); // staged, patched and none
     this.name = ko.observable(name);
-    this.displayName = ko.observable(name);
+    this.oldName = ko.observable(oldName);
+    this.displayName = ko.observable(displayName);
     this.isNew = ko.observable(false);
     this.removed = ko.observable(false);
     this.conflict = ko.observable(false);
@@ -341,7 +342,7 @@ class FileViewModel {
       // only show modfied whe not removed, not conflicted, not new, not renamed
       // and length of additions and deletions is 0.
       return !this.removed() && !this.conflict() && !this.isNew() &&
-        !this.renamed() && this.additions().length === 0 && this.deletions().length === 0;
+        this.additions().length === 0 && this.deletions().length === 0;
     });
     this.fileType = ko.observable('text');
     this.patchLineList = ko.observableArray();
@@ -366,6 +367,8 @@ class FileViewModel {
   getSpecificDiff() {
     return components.create(!this.name() || `${this.fileType()}diff`, {
       filename: this.name(),
+      oldFilename: this.oldName(),
+      displayFilename: this.displayName(),
       repoPath: this.staging.repoPath,
       server: this.server,
       textDiffType: this.staging.textDiffType,
@@ -444,7 +447,6 @@ class FileViewModel {
   }
 
   toggleDiffs() {
-    if (this.renamed()) return; // do not show diffs for renames
     this.isShowingDiffs(!this.isShowingDiffs());
   }
 

components/textdiff/textdiff.js

@@ -62,6 +62,7 @@ class WhiteSpace {
 class TextDiffViewModel {
   constructor(args) {
     this.filename = args.filename;
+    this.oldFilename = args.oldFilename
     this.repoPath = args.repoPath;
     this.server = args.server;
     this.sha1 = args.sha1;
@@ -99,6 +100,7 @@ class TextDiffViewModel {
   getDiffArguments() {
     return {
       file: this.filename,
+      oldFile: this.oldFilename,
       path: this.repoPath(),
       sha1: this.sha1 ? this.sha1 : '',
       whiteSpace: this.whiteSpace.value()

source/git-api.js

@@ -260,7 +260,7 @@ exports.registerApi = (env) => {
 
   app.get(`${exports.pathPrefix}/diff`, ensureAuthenticated, ensurePathExists, (req, res) => {
     const isIgnoreWhiteSpace = req.query.whiteSpace === "true" ? true : false;
-    jsonResultOrFailProm(res, gitPromise.diffFile(req.query.path, req.query.file, req.query.sha1, isIgnoreWhiteSpace));
+    jsonResultOrFailProm(res, gitPromise.diffFile(req.query.path, req.query.file, req.query.oldFile, req.query.sha1, isIgnoreWhiteSpace));
   });
 
   app.get(`${exports.pathPrefix}/diff/image`, ensureAuthenticated, ensurePathExists, (req, res) => {
@@ -327,11 +327,11 @@ exports.registerApi = (env) => {
   });
 
   app.get(`${exports.pathPrefix}/show`, ensureAuthenticated, (req, res) => {
-    jsonResultOrFailProm(res, gitPromise(['show', '--numstat', req.query.sha1], req.query.path).then(gitParser.parseGitLog));
+    jsonResultOrFailProm(res, gitPromise(['show', '--numstat', '-z', req.query.sha1], req.query.path).then(gitParser.parseGitLog));
   });
 
   app.get(`${exports.pathPrefix}/head`, ensureAuthenticated, ensurePathExists, (req, res) => {
-    const task = gitPromise(['log', '--decorate=full', '--pretty=fuller', '--parents', '--max-count=1'], req.query.path)
+    const task = gitPromise(['log', '--decorate=full', '--pretty=fuller', '-z', '--parents', '--max-count=1'], req.query.path)
       .then(gitParser.parseGitLog)
       .catch((err) => {
         if (err.stderr.indexOf('fatal: bad default revision \'HEAD\'') == 0)
@@ -614,7 +614,7 @@ exports.registerApi = (env) => {
   });
 
   app.get(`${exports.pathPrefix}/stashes`, ensureAuthenticated, ensurePathExists, (req, res) => {
-    const task = gitPromise(['stash', 'list', '--decorate=full', '--pretty=fuller', '--parents', '--numstat'], req.query.path)
+    const task = gitPromise(['stash', 'list', '--decorate=full', '--pretty=fuller', '-z', '--parents', '--numstat'], req.query.path)
       .then(gitParser.parseGitLog);
     jsonResultOrFailProm(res, task);
   });

source/git-parser.js

@@ -3,24 +3,36 @@ const fileType = require('./utils/file-type.js');
 const _ = require('lodash');
 
 exports.parseGitStatus = (text, args) => {
-  const lines = text.split('\n');
+  const lines = text.split('\x00');
   const files = {};
   // skipping first line...
-  lines.slice(1).forEach((line) => {
-    if (line == '') return;
+  let lineIterator = lines.slice(1).values();
+
+  for(let line of lineIterator){
+    if (line == '') continue;
     const status = line.slice(0, 2);
-    const filename = line.slice(3).trim().replace(/^"(.*)"$/, '$1'); // may contain old and renamed file name.
-    const finalFilename = status[0] == 'R' ? filename.slice(filename.indexOf('>') + 2) : filename;
-    files[finalFilename] = {
-      displayName: filename,
+    const newFileName = line.slice(3).trim();
+    let oldFileName;
+    let displayName;
+    if (status[0] == 'R') {
+      oldFileName = lineIterator.next().value
+      displayName = `${oldFileName} → ${newFileName}`;
+    } else {
+      oldFileName = newFileName;
+      displayName = newFileName;
+    }
+    files[newFileName] = {
+      fileName: newFileName,
+      oldFileName: oldFileName,
+      displayName: displayName,
       staged: status[0] == 'A' || status[0] == 'M',
       removed: status[0] == 'D' || status[1] == 'D',
       isNew: (status[0] == '?' || status[0] == 'A') && !(status[0] == 'D' || status[1] == 'D'),
       conflict: (status[0] == 'A' && status[1] == 'A') || status[0] == 'U' || status[1] == 'U',
       renamed: status[0] == 'R',
-      type: fileType(finalFilename)
+      type: fileType(newFileName)
     };
-  });
+  }
 
   return {
     isMoreToLoad: false,
@@ -30,16 +42,19 @@ exports.parseGitStatus = (text, args) => {
   };
 };
 
+const fileChangeRegex = /(?<additions>[\d-]+)\t(?<deletions>[\d-]+)\t((?<fileName>[^\x00]+?)\x00|\x00(?<oldFileName>[^\x00]+?)\x00(?<newFileName>[^\x00]+?)\x00)/g;
+
 exports.parseGitStatusNumstat = (text) => {
   const result = {};
-  text.split('\n').forEach((line) => {
-    if (line == '') return;
-    const parts = line.split('\t');
-    result[parts[2]] = {
-      additions: parts[0],
-      deletions: parts[1]
+  fileChangeRegex.lastIndex = 0;
+  let match = fileChangeRegex.exec(text);
+  while (match !== null) {
+    result[match.groups.fileName || match.groups.newFileName] = {
+      additions: match.groups.additions,
+      deletions: match.groups.deletions
     };
-  });
+    match = fileChangeRegex.exec(text);
+  }
   return result;
 };
 
@@ -141,24 +156,50 @@ exports.parseGitLog = (data) => {
     currentCommmit.message += row.trim();
   }
   const parseFileChanges = (row, index) => {
-    if (rows.length === index + 1 || rows[index + 1] && rows[index + 1].indexOf('commit ') === 0) {
-      const total = [0, 0, 'Total'];
-      for (let n = 0; n < currentCommmit.fileLineDiffs.length; n++) {
-        const fileLineDiff = currentCommmit.fileLineDiffs[n];
-        if (!isNaN(parseInt(fileLineDiff[0], 10))) {
-          total[0] += fileLineDiff[0] = parseInt(fileLineDiff[0], 10);
-        }
-        if (!isNaN(parseInt(fileLineDiff[1], 10))) {
-          total[1] += fileLineDiff[1] = parseInt(fileLineDiff[1], 10);
-        }
+    // git log is using -z so all the file changes are on one line
+    // merge commits start the file changes with a null
+    if (row[0] === '\x00'){
+      row = row.slice(1);
+    }
+    fileChangeRegex.lastIndex = 0;
+    while (row[fileChangeRegex.lastIndex] && row[fileChangeRegex.lastIndex] !== '\x00') {
+      let match = fileChangeRegex.exec(row);
+      let fileName = match.groups.fileName || match.groups.newFileName;
+      let oldFileName = match.groups.oldFileName || match.groups.fileName;
+      let displayName;
+      if(match.groups.oldFileName) {
+        displayName = `${match.groups.oldFileName} → ${match.groups.newFileName}`;
+      } else {
+        displayName = fileName;
       }
-      currentCommmit.fileLineDiffs.splice(0, 0, total);
-      parser = parseCommitLine;
-      return;
+      currentCommmit.fileLineDiffs.push({
+        additions: match.groups.additions,
+        deletions: match.groups.deletions,
+        fileName: fileName,
+        oldFileName: oldFileName,
+        displayName: displayName,
+        type: fileType(fileName)
+      });
+    }
+    const nextRow = row.slice(fileChangeRegex.lastIndex + 1);
+    const total = {
+      additions: 0,
+      deletions: 0
+    };
+    for (let fileLineDiff of currentCommmit.fileLineDiffs) {
+      if (!isNaN(parseInt(fileLineDiff.additions, 10))) {
+        total.additions += fileLineDiff.additions = parseInt(fileLineDiff.additions, 10);
+      }
+      if (!isNaN(parseInt(fileLineDiff.deletions, 10))) {
+        total.deletions += fileLineDiff.deletions = parseInt(fileLineDiff.deletions, 10);
+      }
+    }
+    currentCommmit.total = total;
+    parser = parseCommitLine;
+    if (nextRow) {
+      parser(nextRow, index);
     }
-    const splitted = row.split('\t');
-    splitted.push(fileType(splitted[2]));
-    currentCommmit.fileLineDiffs.push(splitted);
+    return;
   }
   let parser = parseCommitLine;
   const rows = data.split('\n');

source/git-promise.js

@@ -177,17 +177,17 @@ const getGitError = (args, stderr, stdout) => {
 
 git.status = (repoPath, file) => {
   return Bluebird.props({
-    numStatsStaged: git([gitOptionalLocks, 'diff', '--no-renames', '--numstat', '--cached', '--', (file || '')], repoPath)
+    numStatsStaged: git([gitOptionalLocks, 'diff', '--numstat', '--cached', '-z', '--', (file || '')], repoPath)
       .then(gitParser.parseGitStatusNumstat),
     numStatsUnstaged: Bluebird.resolve().then(() => {
       if (config.isEnableNumStat) {
-        return git([gitOptionalLocks, 'diff', '--no-renames', '--numstat', '--', (file || '')], repoPath)
+        return git([gitOptionalLocks, 'diff', '--numstat', '-z', '--', (file || '')], repoPath)
           .then(gitParser.parseGitStatusNumstat);
       } else {
         return {}
       }
     }),
-    status: git([gitOptionalLocks, 'status', '-s', '-b', '-u', (file || '')], repoPath)
+    status: git([gitOptionalLocks, 'status', '-s', '-b', '-u', '-z', (file || '')], repoPath)
       .then(gitParser.parseGitStatus)
       .then((status) => {
         return Bluebird.props({
@@ -282,11 +282,15 @@ git.binaryFileContent = (repoPath, filename, version, outPipe) => {
   return git(['show', `${version}:${filename}`], repoPath, null, outPipe);
 }
 
-git.diffFile = (repoPath, filename, sha1, ignoreWhiteSpace) => {
+git.diffFile = (repoPath, filename, oldFilename, sha1, ignoreWhiteSpace) => {
   if (sha1) {
     return git(['rev-list', '--max-parents=0', sha1], repoPath).then((initialCommitSha1) => {
       let prevSha1 = sha1 == initialCommitSha1.trim() ? gitEmptyReproSha1 : `${sha1}^`;
-      return git(['diff', ignoreWhiteSpace ? '-w' : '', prevSha1, sha1, "--", filename.trim()], repoPath);
+      if (oldFilename && oldFilename !== filename) {
+        return git(['diff', ignoreWhiteSpace ? '-w' : '', `${prevSha1}:${oldFilename.trim()}`, `${sha1}:${filename.trim()}`], repoPath);
+      } else {
+        return git(['diff', ignoreWhiteSpace ? '-w' : '', prevSha1, sha1, "--", filename.trim()], repoPath);
+      }
     });
   }
 
@@ -304,6 +308,8 @@ git.diffFile = (repoPath, filename, sha1, ignoreWhiteSpace) => {
       } else {
         if (file && file.isNew) {
           return git(['diff', '--no-index', isWindows ? 'NUL' : '/dev/null', filename.trim()], repoPath, true);
+        } else if (file && file.renamed) {
+          return git(['diff', ignoreWhiteSpace ? '-w' : '', `HEAD:${oldFilename}`, filename.trim()], repoPath);
         } else {
           return git(['diff', ignoreWhiteSpace ? '-w' : '', 'HEAD', '--', filename.trim()], repoPath);
         }
@@ -437,7 +443,7 @@ git.revParse = (repoPath) => {
 }
 
 git.log = (path, limit, skip, maxActiveBranchSearchIteration) => {
-  return git(['log', '--cc', '--decorate=full', '--show-signature', '--date=default', '--pretty=fuller', '--branches', '--tags', '--remotes', '--parents', '--no-notes', '--numstat', '--date-order', `--max-count=${limit}`, `--skip=${skip}`], path)
+  return git(['log', '--cc', '--decorate=full', '--show-signature', '--date=default', '--pretty=fuller', '-z', '--branches', '--tags', '--remotes', '--parents', '--no-notes', '--numstat', '--date-order', `--max-count=${limit}`, `--skip=${skip}`], path)
     .then(gitParser.parseGitLog)
     .then((log) => {
       log = log ? log : [];

test/spec.git-api.branching.js

@@ -134,9 +134,12 @@ describe('git-api branching', function () {
 
   it('status should list the changed file', () => {
     return common.get(req, '/status', { path: testDir }).then(res => {
+
       expect(Object.keys(res.files).length).to.be(1);
       expect(res.files[testFile1]).to.eql({
         displayName: testFile1,
+        fileName: testFile1,
+        oldFileName: testFile1,
         isNew: false,
         staged: false,
         removed: false,