Remove only trailing "#" and empty lines as comments from CLI editors

Summary: Don't strip numbered lists in comment bodies, etc.

Test Plan: Unit tests, meta-testing this diff.

Reviewers: vrana, btrahan

Reviewed By: vrana

CC: aran

Maniphest Tasks: T1124

Differential Revision: https://secure.phabricator.com/D2262

src/__phutil_library_map__.php

@@ -23,6 +23,8 @@ phutil_register_library_map(array(
     'ArcanistChooseNoRevisionsException' => 'exception',
     'ArcanistCloseRevisionWorkflow' => 'workflow/close-revision',
     'ArcanistCloseWorkflow' => 'workflow/close',
+    'ArcanistCommentRemover' => 'parser/commentremover',
+    'ArcanistCommentRemoverTestCase' => 'parser/commentremover/__tests__',
     'ArcanistCommitWorkflow' => 'workflow/commit',
     'ArcanistConduitLinter' => 'lint/linter/conduit',
     'ArcanistConfiguration' => 'configuration',
@@ -129,6 +131,7 @@ phutil_register_library_map(array(
     'ArcanistCallConduitWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistCloseRevisionWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistCloseWorkflow' => 'ArcanistBaseWorkflow',
+    'ArcanistCommentRemoverTestCase' => 'ArcanistPhutilTestCase',
     'ArcanistCommitWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistConduitLinter' => 'ArcanistLinter',
     'ArcanistCoverWorkflow' => 'ArcanistBaseWorkflow',

src/parser/commentremover/ArcanistCommentRemover.php

@@ -0,0 +1,44 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class ArcanistCommentRemover {
+
+  /**
+   * Remove comment lines from a commit message. Strips trailing lines only,
+   * and requires "#" to appear at the beginning of a line for it to be
+   * considered a comment.
+   */
+  public static function removeComments($body) {
+    $lines = explode("\n", $body);
+    $lines = array_reverse($lines);
+    foreach ($lines as $key => $line) {
+      if (!strlen($line)) {
+        unset($lines[$key]);
+        continue;
+      }
+      if ($line[0] == '#') {
+        unset($lines[$key]);
+        continue;
+      }
+      break;
+    }
+    $lines = array_reverse($lines);
+    return implode("\n", $lines)."\n";
+  }
+
+}

src/parser/commentremover/__init__.php

@@ -0,0 +1,10 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+
+phutil_require_source('ArcanistCommentRemover.php');

src/parser/commentremover/__tests__/ArcanistCommentRemoverTestCase.php

@@ -0,0 +1,48 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class ArcanistCommentRemoverTestCase extends ArcanistPhutilTestCase {
+
+  public function testRemover() {
+    $test = <<<EOTEXT
+Here is a list:
+
+  # Stuff
+  # More Stuff
+
+The end.
+
+# Instructional comments.
+# Appear here.
+# At the bottom.
+EOTEXT;
+
+    $expect = <<<EOTEXT
+Here is a list:
+
+  # Stuff
+  # More Stuff
+
+The end.
+
+EOTEXT;
+
+    $this->assertEqual($expect, ArcanistCommentRemover::removeComments($test));
+  }
+
+}

src/parser/commentremover/__tests__/__init__.php

@@ -0,0 +1,13 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('arcanist', 'parser/commentremover');
+phutil_require_module('arcanist', 'unit/engine/phutil/testcase');
+
+
+phutil_require_source('ArcanistCommentRemoverTestCase.php');

src/workflow/diff/ArcanistDiffWorkflow.php

@@ -1186,10 +1186,7 @@ EOTEXT
         "No explanation provided.");
     }
 
-    $template = preg_replace('/^\s*#.*$/m', '', $new_template);
-    $template = rtrim($template)."\n";
-
-    return $template;
+    return ArcanistCommentRemover::removeComments($new_template);
   }
 
 
@@ -1357,8 +1354,7 @@ EOTEXT
         throw new ArcanistUsageException("Template not edited.");
       }
 
-      $template = preg_replace('/^\s*#.*$/m', '', $new_template);
-      $template = rtrim($template)."\n";
+      $template = ArcanistCommentRemover::removeComments($new_template);
       $wrote = $this->writeScratchFile('create-message', $template);
       $where = $this->getReadableScratchFilePath('create-message');
 
@@ -1516,10 +1512,8 @@ EOTEXT
       ->setName('differential-update-comments')
       ->editInteractively();
 
-    $comments = preg_replace('/^\s*#.*$/m', '', $comments);
-    $comments = rtrim($comments);
-
-    if (!strlen($comments)) {
+    $comments = ArcanistCommentRemover::removeComments($comments);
+    if (!strlen(trim($comments))) {
       throw new ArcanistUserAbortException();
     }
 

src/workflow/diff/__init__.php

@@ -10,6 +10,7 @@ phutil_require_module('arcanist', 'difference');
 phutil_require_module('arcanist', 'differential/commitmessage');
 phutil_require_module('arcanist', 'exception/usage');
 phutil_require_module('arcanist', 'exception/usage/userabort');
+phutil_require_module('arcanist', 'parser/commentremover');
 phutil_require_module('arcanist', 'parser/diff');
 phutil_require_module('arcanist', 'parser/diff/changetype');
 phutil_require_module('arcanist', 'repository/api/base');

src/workflow/patch/ArcanistPatchWorkflow.php

@@ -662,11 +662,8 @@ EOTEXT
         ->setName('arcanist-patch-commit-message')
         ->editInteractively();
 
-      $commit_message = preg_replace('/^\s*#.*$/m',
-                                     '',
-                                     $commit_message);
-      $commit_message = rtrim($commit_message);
-      if (!strlen($commit_message)) {
+      $commit_message = ArcanistCommentRemover::removeComments($commit_message);
+      if (!strlen(trim($commit_message))) {
         throw new ArcanistUserAbortException();
       }
     }

src/workflow/patch/__init__.php

@@ -11,6 +11,7 @@ phutil_require_module('arcanist', 'differential/constants/revisionstatus');
 phutil_require_module('arcanist', 'exception/usage');
 phutil_require_module('arcanist', 'exception/usage/userabort');
 phutil_require_module('arcanist', 'parser/bundle');
+phutil_require_module('arcanist', 'parser/commentremover');
 phutil_require_module('arcanist', 'parser/diff/changetype');
 phutil_require_module('arcanist', 'workflow/base');