From 315870d56a334f4e0b7618c376afdaf40f8e890e Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Mar 2012 14:13:48 -0700 Subject: [PATCH] Fix various newline problems in the difference engines Summary: I'll mark this one up inline since it's all separate bugs. Test Plan: - Created a diff with eight changes: (newline absent -> newline present, newline present -> newline absent, newline present -> newline present, newline absent -> newline absent) x (short file with change near end, long file with change near middle). - Viewed diff in Ignore All, Ignore Most, Ignore Trailing and Show All whitespace modes. - All 32 results seemed sensible. - Really wish this stuff was better factored and testable. Need to fix it. :( Reviewers: vrana, btrahan Reviewed By: btrahan CC: aran, epriestley Maniphest Tasks: T1030 Differential Revision: https://secure.phabricator.com/D1992 --- .../changeset/DifferentialChangesetParser.php | 13 ++++-- .../storage/hunk/DifferentialHunk.php | 42 +++++++++++++++++-- .../engine/PhabricatorDifferenceEngine.php | 26 +++++++++++- 3 files changed, 73 insertions(+), 8 deletions(-) diff --git a/src/applications/differential/parser/changeset/DifferentialChangesetParser.php b/src/applications/differential/parser/changeset/DifferentialChangesetParser.php index 4a6e94a99f..2a70a466dc 100644 --- a/src/applications/differential/parser/changeset/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/changeset/DifferentialChangesetParser.php @@ -228,7 +228,7 @@ final class DifferentialChangesetParser { 'text' => (string)substr($lines[$cursor], 1), 'line' => $new_line, ); - if ($type == '\\' && $cursor > 1) { + if ($type == '\\') { $type = $types[$cursor - 1]; $data['text'] = ltrim($data['text']); } @@ -338,9 +338,14 @@ final class DifferentialChangesetParser { break; } if ($similar) { - $o_desc['type'] = null; - $n_desc['type'] = null; - $skip_intra[count($old)] = true; + if ($o_desc['type'] == '\\') { + // These are similar because they're "No newline at end of file" + // comments. + } else { + $o_desc['type'] = null; + $n_desc['type'] = null; + $skip_intra[count($old)] = true; + } } else { $changed = true; } diff --git a/src/applications/differential/storage/hunk/DifferentialHunk.php b/src/applications/differential/storage/hunk/DifferentialHunk.php index 09f6c88d81..629c2e4f79 100644 --- a/src/applications/differential/storage/hunk/DifferentialHunk.php +++ b/src/applications/differential/storage/hunk/DifferentialHunk.php @@ -40,13 +40,49 @@ final class DifferentialHunk extends DifferentialDAO { final private function makeContent($exclude) { $results = array(); $lines = explode("\n", $this->changes); + + // NOTE: To determine whether the recomposed file should have a trailing + // newline, we look for a "\ No newline at end of file" line which appears + // after a line which we don't exclude. For example, if we're constructing + // the "new" side of a diff (excluding "-"), we want to ignore this one: + // + // - x + // \ No newline at end of file + // + x + // + // ...since it's talking about the "old" side of the diff, but interpret + // this as meaning we should omit the newline: + // + // - x + // + x + // \ No newline at end of file + + + $use_next_newline = false; + $has_newline = true; foreach ($lines as $line) { - if (isset($line[0]) && $line[0] == $exclude) { - continue; + if (isset($line[0])) { + if ($line[0] == $exclude) { + $use_next_newline = false; + continue; + } + if ($line[0] == '\\') { + if ($use_next_newline) { + $has_newline = false; + } + continue; + } } + $use_next_newline = true; $results[] = substr($line, 1); } - return implode("\n", $results); + + $possible_newline = ''; + if ($has_newline) { + $possible_newline = "\n"; + } + + return implode("\n", $results).$possible_newline; } } diff --git a/src/infrastructure/diff/engine/PhabricatorDifferenceEngine.php b/src/infrastructure/diff/engine/PhabricatorDifferenceEngine.php index a9847c259a..16a3bae770 100644 --- a/src/infrastructure/diff/engine/PhabricatorDifferenceEngine.php +++ b/src/infrastructure/diff/engine/PhabricatorDifferenceEngine.php @@ -1,7 +1,7 @@ $line) { $entire_file[$k] = ' '.$line; } + $len = count($entire_file); $entire_file = implode("\n", $entire_file); + // TODO: If both files were identical but missing newlines, we probably + // get this wrong. Unclear if it ever matters. + // This is a bit hacky but the diff parser can handle it. $diff = "--- {$old_name}\n". "+++ {$new_name}\n". "@@ -1,{$len} +1,{$len} @@\n". $entire_file."\n"; + } else { + if ($this->ignoreWhitespace) { + + // Under "-bw", `diff` is inconsistent about emitting "\ No newline + // at end of file". For instance, a long file with a change in the + // middle will emit a contextless "\ No newline..." at the end if a + // newline is removed, but not if one is added. A file with a change + // at the end will emit the "old" "\ No newline..." block only, even + // if the newline was not removed. Since we're ostensibly ignoring + // whitespace changes, just drop these lines if they appear anywhere + // in the diff. + + $lines = explode("\n", rtrim($diff)); + foreach ($lines as $key => $line) { + if (isset($line[0]) && $line[0] == '\\') { + unset($lines[$key]); + } + } + $diff = implode("\n", $lines); + } } return $diff;