From ab17a7d4bf84c83c5a8fc7b896feced118130542 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 1 Jan 2017 07:34:15 -0800 Subject: [PATCH] Be more lenient when accepting "Differential Revision" in the presence of custom ad-hoc commit message fields Summary: Fixes T8360. We will now parse revisions out of "Differential Revision: X" followed by other ad-hoc fields which we do not recognize. Previously, these fields would be treated as part of the value. (In the general case, other fields may line wrap so we can't assume that fields are only one line long. However, we can make that assumption safely for this field.) Also maybe fix whatever was going on in T9965 although that didn't really have a reproduction case. Test Plan: Added unit tests. Reviewers: chad Reviewed By: chad Maniphest Tasks: T8360 Differential Revision: https://secure.phabricator.com/D17121 --- src/__phutil_library_map__.php | 2 ++ ...fferentialRevisionIDCommitMessageField.php | 19 +++++++++++- ...DifferentialCommitMessageFieldTestCase.php | 30 +++++++++++++++++++ .../DifferentialCommitMessageParser.php | 5 +++- 4 files changed, 54 insertions(+), 2 deletions(-) create mode 100644 src/applications/differential/field/__tests__/DifferentialCommitMessageFieldTestCase.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 02761bc1be..318095411b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -381,6 +381,7 @@ phutil_register_library_map(array( 'DifferentialCloseConduitAPIMethod' => 'applications/differential/conduit/DifferentialCloseConduitAPIMethod.php', 'DifferentialCommitMessageCustomField' => 'applications/differential/field/DifferentialCommitMessageCustomField.php', 'DifferentialCommitMessageField' => 'applications/differential/field/DifferentialCommitMessageField.php', + 'DifferentialCommitMessageFieldTestCase' => 'applications/differential/field/__tests__/DifferentialCommitMessageFieldTestCase.php', 'DifferentialCommitMessageParser' => 'applications/differential/parser/DifferentialCommitMessageParser.php', 'DifferentialCommitMessageParserTestCase' => 'applications/differential/parser/__tests__/DifferentialCommitMessageParserTestCase.php', 'DifferentialCommitsField' => 'applications/differential/customfield/DifferentialCommitsField.php', @@ -5030,6 +5031,7 @@ phutil_register_library_map(array( 'DifferentialCloseConduitAPIMethod' => 'DifferentialConduitAPIMethod', 'DifferentialCommitMessageCustomField' => 'DifferentialCommitMessageField', 'DifferentialCommitMessageField' => 'Phobject', + 'DifferentialCommitMessageFieldTestCase' => 'PhabricatorTestCase', 'DifferentialCommitMessageParser' => 'Phobject', 'DifferentialCommitMessageParserTestCase' => 'PhabricatorTestCase', 'DifferentialCommitsField' => 'DifferentialCustomField', diff --git a/src/applications/differential/field/DifferentialRevisionIDCommitMessageField.php b/src/applications/differential/field/DifferentialRevisionIDCommitMessageField.php index 1c4a2c9d60..8c85553e71 100644 --- a/src/applications/differential/field/DifferentialRevisionIDCommitMessageField.php +++ b/src/applications/differential/field/DifferentialRevisionIDCommitMessageField.php @@ -18,8 +18,25 @@ final class DifferentialRevisionIDCommitMessageField } public function parseFieldValue($value) { - // If the value is just "D123" or similar, parse the ID from it directly. + // If the complete commit message we are parsing has unrecognized custom + // fields at the end, they can end up parsed into the field value for this + // field. For example, if the message looks like this: + + // Differential Revision: xyz + // Some-Other-Field: abc + + // ...we will receive "xyz\nSome-Other-Field: abc" as the field value for + // this field. Ideally, the install would define these fields so they can + // parse formally, but we can reasonably assume that only the first line + // of any value we encounter actually contains a revision identifier, so + // start by throwing away any other lines. + $value = trim($value); + $value = phutil_split_lines($value, false); + $value = head($value); + $value = trim($value); + + // If the value is just "D123" or similar, parse the ID from it directly. $matches = null; if (preg_match('/^[dD]([1-9]\d*)\z/', $value, $matches)) { return (int)$matches[1]; diff --git a/src/applications/differential/field/__tests__/DifferentialCommitMessageFieldTestCase.php b/src/applications/differential/field/__tests__/DifferentialCommitMessageFieldTestCase.php new file mode 100644 index 0000000000..295da8ca59 --- /dev/null +++ b/src/applications/differential/field/__tests__/DifferentialCommitMessageFieldTestCase.php @@ -0,0 +1,30 @@ + 123, + 'd123' => 123, + " \n d123 \n " => 123, + "D123\nSome-Custom-Field: The End" => 123, + "{$base_uri}D123" => 123, + "{$base_uri}D123\nSome-Custom-Field: The End" => 123, + ); + + $env = PhabricatorEnv::beginScopedEnv(); + $env->overrideEnvConfig('phabricator.base-uri', $base_uri); + + foreach ($tests as $input => $expect) { + $actual = id(new DifferentialRevisionIDCommitMessageField()) + ->parseFieldValue($input); + $this->assertEqual($expect, $actual, pht('Parse of: %s', $input)); + } + + unset($env); + } + +} diff --git a/src/applications/differential/parser/DifferentialCommitMessageParser.php b/src/applications/differential/parser/DifferentialCommitMessageParser.php index 7a1625c67a..4a5bf6a27b 100644 --- a/src/applications/differential/parser/DifferentialCommitMessageParser.php +++ b/src/applications/differential/parser/DifferentialCommitMessageParser.php @@ -155,7 +155,10 @@ final class DifferentialCommitMessageParser extends Phobject { $field = $key_title; $seen = array(); - $lines = explode("\n", trim($corpus)); + + $lines = trim($corpus); + $lines = phutil_split_lines($lines, false); + $field_map = array(); foreach ($lines as $key => $line) { $match = null;