From 222800a86ed002c564e2760d6c5d9e93810b5b96 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 19 Jun 2018 12:06:15 -0700 Subject: [PATCH 1/4] Parse Mercurial changeset evolution "instability" log field Summary: See PHI718. Modern Mercurial with the "evolve" extension enabled may emit this field. Test Plan: As D19262. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19498 --- src/repository/parser/ArcanistMercurialParser.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/repository/parser/ArcanistMercurialParser.php b/src/repository/parser/ArcanistMercurialParser.php index 4eb10e2f..33e59c5c 100644 --- a/src/repository/parser/ArcanistMercurialParser.php +++ b/src/repository/parser/ArcanistMercurialParser.php @@ -175,8 +175,9 @@ final class ArcanistMercurialParser extends Phobject { $commit['bookmark'] = $value; break; case 'obsolete': - // This is an extra field added by the "evolve" extension even - // if HGPLAIN=1 is set. See PHI502. + case 'instability': + // These are extra fields added by the "evolve" extension even + // if HGPLAIN=1 is set. See PHI502 and PHI718. break; default: throw new Exception( From 875d018360374cb4b1287309782fcb9a75d4bcbf Mon Sep 17 00:00:00 2001 From: Aviv Eyal Date: Mon, 9 Jul 2018 17:59:15 +0000 Subject: [PATCH 2/4] Fix `arc diff` when adding large new file with new git Summary: See https://discourse.phabricator-community.org/t/arc-not-supporting-git-2-17-1/. When treating it a large file binary, we try to get the "old" and "new" content using `git ls-tree` and `cat-file`. If the file is new or deleted, there is no old file, so we try to work with filename `null`. Under git < 2.17.1, that gets treated as `git ls-tree -- .`, which falls in the next condition under "no such path". In git 2.18, etc, this is an error. Explicitly bail out if there is no filename. Test Plan: Add a new, large (>4Mb) file, arc diff. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D19513 --- src/repository/api/ArcanistGitAPI.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php index c7149bd1..0643ab9d 100644 --- a/src/repository/api/ArcanistGitAPI.php +++ b/src/repository/api/ArcanistGitAPI.php @@ -1032,6 +1032,11 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI { // and treat it as though it as a file containing a list of other files, // which is silly. + if (!strlen($path)) { + // No filename, so there's no content (Probably new/deleted file). + return null; + } + list($stdout) = $this->execxLocal( 'ls-tree %s -- %s', $revision, From d9a4293ae734756823b4a3ca202f185c57f3e834 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 3 Aug 2018 12:57:55 -0700 Subject: [PATCH 3/4] Consolidate redundant "should should" from some linter help strings in Arcanist Summary: See . Test Plan: Read carefully. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19561 --- .../xhpast/rules/ArcanistDeprecationXHPASTLinterRule.php | 2 +- .../rules/ArcanistUnsafeDynamicStringXHPASTLinterRule.php | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/lint/linter/xhpast/rules/ArcanistDeprecationXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistDeprecationXHPASTLinterRule.php index bc57bb2d..d94192b5 100644 --- a/src/lint/linter/xhpast/rules/ArcanistDeprecationXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistDeprecationXHPASTLinterRule.php @@ -20,7 +20,7 @@ final class ArcanistDeprecationXHPASTLinterRule 'xhpast.deprecated.functions' => array( 'type' => 'optional map', 'help' => pht( - 'Functions which should should be considered deprecated.'), + 'Functions which should be considered deprecated.'), ), ); } diff --git a/src/lint/linter/xhpast/rules/ArcanistUnsafeDynamicStringXHPASTLinterRule.php b/src/lint/linter/xhpast/rules/ArcanistUnsafeDynamicStringXHPASTLinterRule.php index bf3c8e51..67cdfcc0 100644 --- a/src/lint/linter/xhpast/rules/ArcanistUnsafeDynamicStringXHPASTLinterRule.php +++ b/src/lint/linter/xhpast/rules/ArcanistUnsafeDynamicStringXHPASTLinterRule.php @@ -21,14 +21,14 @@ final class ArcanistUnsafeDynamicStringXHPASTLinterRule 'xhpast.dynamic-string.classes' => array( 'type' => 'optional map', 'help' => pht( - 'Classes which should should not be used because they represent the '. + 'Classes which should not be used because they represent the '. 'unsafe usage of dynamic strings.'), ), 'xhpast.dynamic-string.functions' => array( 'type' => 'optional map', 'help' => pht( - 'Functions which should should not be used because they represent '. - 'the unsafe usage of dynamic strings.'), + 'Functions which should not be used because they represent the '. + 'unsafe usage of dynamic strings.'), ), ); From e1e93271e6e0f38d2bfa318d1d3d644d43b40962 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 24 Aug 2018 10:37:21 -0700 Subject: [PATCH 4/4] Make the ArcanistBundle algorithm do what "diff -u" does when hunks are arguably mergeable Summary: Ref T13187. See PHI838. If two hunks are separated by 7 lines of context, we can render them as either: ```lang=diff + Hunk A Context 1 Context 2 Context 3 Context 4 Context 5 Context 6 Context 7 + Hunk B ``` ...or: ```lang=diff + Hunk A Context 1 Context 2 Context 3 @@ +1,2 -3,4 @@ Context 5 Context 6 Context 7 + Hunk B ``` Since we get the same number of output lines either way and the first one is more human-readable, we picked that one. However, `diff -u` does the second one. Since human-readability is probably less important than compatibility, change the behavior to be more similar to `diff -u`. Test Plan: Added unit tests for the edge cases with default parameters (6 context lines, 7 context lines) and made them pass. Reviewers: amckinley Maniphest Tasks: T13187 Differential Revision: https://secure.phabricator.com/D19603 --- src/parser/ArcanistBundle.php | 11 +++++++++- .../__tests__/ArcanistBundleTestCase.php | 18 +++++++++++++++++ src/parser/__tests__/bundle/merge-hunks.diff | 19 ++++++++++++++++++ src/parser/__tests__/bundle/merge-hunks.new | 17 ++++++++++++++++ src/parser/__tests__/bundle/merge-hunks.old | 15 ++++++++++++++ .../__tests__/bundle/no-merge-hunks.diff | 20 +++++++++++++++++++ .../__tests__/bundle/no-merge-hunks.new | 17 ++++++++++++++++ .../__tests__/bundle/no-merge-hunks.old | 15 ++++++++++++++ 8 files changed, 131 insertions(+), 1 deletion(-) create mode 100644 src/parser/__tests__/bundle/merge-hunks.diff create mode 100644 src/parser/__tests__/bundle/merge-hunks.new create mode 100644 src/parser/__tests__/bundle/merge-hunks.old create mode 100644 src/parser/__tests__/bundle/no-merge-hunks.diff create mode 100644 src/parser/__tests__/bundle/no-merge-hunks.new create mode 100644 src/parser/__tests__/bundle/no-merge-hunks.old diff --git a/src/parser/ArcanistBundle.php b/src/parser/ArcanistBundle.php index 0ec9a2ef..6d72047b 100644 --- a/src/parser/ArcanistBundle.php +++ b/src/parser/ArcanistBundle.php @@ -560,7 +560,16 @@ final class ArcanistBundle extends Phobject { } } - if ($jj - $last_change > (($context + 1) * 2)) { + // If the context value is "3" and there are 7 unchanged lines + // between the two changes, we could either generate one or two hunks + // and end up with the same number of output lines. If we generate + // one hunk, the middle line will be a line of source. If we generate + // two hunks, the middle line will be an "@@ -1,2 +3,4 @@" header. + + // We choose to generate two hunks because this is the behavior of + // "diff -u". See PHI838. + + if ($jj - $last_change >= ($context * 2 + 1)) { // We definitely aren't going to merge this with the next hunk, so // break out of the loop. We'll end the hunk at $break_here. break; diff --git a/src/parser/__tests__/ArcanistBundleTestCase.php b/src/parser/__tests__/ArcanistBundleTestCase.php index f64e8992..0b183324 100644 --- a/src/parser/__tests__/ArcanistBundleTestCase.php +++ b/src/parser/__tests__/ArcanistBundleTestCase.php @@ -642,6 +642,24 @@ EODIFF; 'disjoint-hunks.new')->toUnifiedDiff()); } + public function testMergeHunks() { + // Hunks should merge if represented by sufficiently few unchanged + // lines. + $this->assertEqual( + $this->loadResource('merge-hunks.diff'), + $this->loadOneChangeBundle( + 'merge-hunks.old', + 'merge-hunks.new')->toUnifiedDiff()); + + // Hunks should not merge if they are separated by too many unchanged + // lines. + $this->assertEqual( + $this->loadResource('no-merge-hunks.diff'), + $this->loadOneChangeBundle( + 'no-merge-hunks.old', + 'no-merge-hunks.new')->toUnifiedDiff()); + } + public function testNonlocalTrailingNewline() { // Diffs without changes near the end of the file should not generate a // bogus, change-free hunk if the file has no trailing newline. diff --git a/src/parser/__tests__/bundle/merge-hunks.diff b/src/parser/__tests__/bundle/merge-hunks.diff new file mode 100644 index 00000000..ea5ca64b --- /dev/null +++ b/src/parser/__tests__/bundle/merge-hunks.diff @@ -0,0 +1,19 @@ +Index: file +=================================================================== +--- file ++++ file +@@ -2,12 +2,14 @@ + B + C + D ++Chocolate + E + F + G + H + I + J ++Caramel + K + L + M diff --git a/src/parser/__tests__/bundle/merge-hunks.new b/src/parser/__tests__/bundle/merge-hunks.new new file mode 100644 index 00000000..a02f54dd --- /dev/null +++ b/src/parser/__tests__/bundle/merge-hunks.new @@ -0,0 +1,17 @@ +A +B +C +D +Chocolate +E +F +G +H +I +J +Caramel +K +L +M +N +O diff --git a/src/parser/__tests__/bundle/merge-hunks.old b/src/parser/__tests__/bundle/merge-hunks.old new file mode 100644 index 00000000..cf494a19 --- /dev/null +++ b/src/parser/__tests__/bundle/merge-hunks.old @@ -0,0 +1,15 @@ +A +B +C +D +E +F +G +H +I +J +K +L +M +N +O diff --git a/src/parser/__tests__/bundle/no-merge-hunks.diff b/src/parser/__tests__/bundle/no-merge-hunks.diff new file mode 100644 index 00000000..878450c9 --- /dev/null +++ b/src/parser/__tests__/bundle/no-merge-hunks.diff @@ -0,0 +1,20 @@ +Index: file +=================================================================== +--- file ++++ file +@@ -2,6 +2,7 @@ + B + C + D ++Chocolate + E + F + G +@@ -9,6 +10,7 @@ + I + J + K ++Caramel + L + M + N diff --git a/src/parser/__tests__/bundle/no-merge-hunks.new b/src/parser/__tests__/bundle/no-merge-hunks.new new file mode 100644 index 00000000..0158638d --- /dev/null +++ b/src/parser/__tests__/bundle/no-merge-hunks.new @@ -0,0 +1,17 @@ +A +B +C +D +Chocolate +E +F +G +H +I +J +K +Caramel +L +M +N +O diff --git a/src/parser/__tests__/bundle/no-merge-hunks.old b/src/parser/__tests__/bundle/no-merge-hunks.old new file mode 100644 index 00000000..cf494a19 --- /dev/null +++ b/src/parser/__tests__/bundle/no-merge-hunks.old @@ -0,0 +1,15 @@ +A +B +C +D +E +F +G +H +I +J +K +L +M +N +O