[llvm][docs] commit phabricator patch

[nickdesaulniers]

Summary

Users upgrading to PHP 8.1 might start observing failures with arc.
Commit @ychen's suggestions as a patch in tree that can be applied since
arcanist is no longer accepting patches.

Also, remove the suggestion to apply an external patch updating CA
certs. It seems that this was fixed in upstream arcanist before they
stopped accepting patches. Compare
https://github.com/rashkov/arcanist/commit/e3659d43d8911e91739f3b0c5935598bceb859aa
vs
https://github.com/rashkov/arcanist/commit/13d3a3c3b100979c34dda261fe21253e3571bc46

Link: https://secure.phabricator.com/book/phabcontrib/article/contributing_code/

[ychen]

I accumulated these in my local arcanist copy. Feel free to apply as well :-)

diff --git a/src/differential/ArcanistDifferentialDependencyGraph.php b/src/differential/ArcanistDifferentialDependencyGraph.php
index 64e2f9c7..b5613808 100644
--- a/src/differential/ArcanistDifferentialDependencyGraph.php
+++ b/src/differential/ArcanistDifferentialDependencyGraph.php
@@ -42,7 +42,7 @@ final class ArcanistDifferentialDependencyGraph extends AbstractDirectedGraph {
     $edges = array();
     foreach ($dependencies as $dependency) {
       $dependency_revision = $this->getCommitHashFromDict($dependency);
-      if ($repository_api->hasLocalCommit($dependency_revision)) {
+      if (phutil_nonempty_string($dependency_revision) && $repository_api->hasLocalCommit($dependency_revision)) {
         $edges[$dependency['phid']] = array();
         continue;
       }
diff --git a/src/lint/linter/ArcanistScriptAndRegexLinter.php b/src/lint/linter/ArcanistScriptAndRegexLinter.php
index 0c3d9d9a..b9f6924e 100644
--- a/src/lint/linter/ArcanistScriptAndRegexLinter.php
+++ b/src/lint/linter/ArcanistScriptAndRegexLinter.php
@@ -338,7 +338,7 @@ final class ArcanistScriptAndRegexLinter extends ArcanistLinter {
     }
 
     $line = idx($match, 'line');
-    if (strlen($line)) {
+    if (phutil_nonempty_string($line) && strlen($line)) {
       $line = (int)$line;
       if (!$line) {
         $line = 1;
diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php
index 6c6d2ac4..4a17f5f2 100644
--- a/src/repository/api/ArcanistGitAPI.php
+++ b/src/repository/api/ArcanistGitAPI.php
@@ -1143,7 +1143,7 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
 
   public function hasLocalCommit($commit) {
     try {
-      if (!$this->getCanonicalRevisionName($commit)) {
+      if (!phutil_nonempty_string($commit) || !$this->getCanonicalRevisionName($commit)) {
         return false;
       }
     } catch (CommandException $exception) {

[nickdesaulniers]

In D129232#3634072, @ychen wrote:

I accumulated these in my local arcanist copy. Feel free to apply as well :-)

Sure, would you like to send me a PR to my branch or would you prefer me to add a new commit myself crediting you with the suggestions?

[dblaikie]

llvm/docs/GettingStarted.rst

479–482 ↗

(On Diff #442697)

perhaps we should put these patches somewhere in the llvm repo?

[ychen]

In D129232#3634102, @nickdesaulniers wrote:

In D129232#3634072, @ychen wrote:

I accumulated these in my local arcanist copy. Feel free to apply as well :-)

Sure, would you like to send me a PR to my branch or would you prefer me to add a new commit myself crediting you with the suggestions?

Nope, no need to credit me, please include the change directly if it makes sense to you. Thanks for submitting this patch to help other people.

[MaskRay]

This chapter in GettingStarted.rst appears to duplicate with Phabricator.rst a bit. We probably should deduplicate the content and add the arc detail to Phabricator.rst instead.
I suspect Contributing.rst has some duplicated content as well.

I don't consider "Contributing to llvm-project" as parted of GettingStarted.

[MaskRay]

In D129232#3634377, @MaskRay wrote:

This chapter in GettingStarted.rst appears to duplicate with Phabricator.rst a bit. We probably should deduplicate the content and add the arc detail to Phabricator.rst instead.
I suspect Contributing.rst has some duplicated content as well.

I don't consider "Contributing to llvm-project" as parted of GettingStarted.

Created D129255 to move "code contribution" part from GettingStarted.rst to Contributing.rst.
If it is accepted, you may add the workaround information to Contributing.rst instead.

[nickdesaulniers]

llvm/docs/GettingStarted.rst

479–482 ↗

(On Diff #442697)

Great idea! If others delete their github branches, these instructions become stale. I will change these directions to apply patches from the llvm-project sources that we can commit/revise. Thoughts on where I should commit such patches in the tree?

Perhaps I should create llvm/utils/phabricator/ ?

[dblaikie]

llvm/docs/GettingStarted.rst

479–482 ↗

(On Diff #442697)

Sounds OK for now/in lieu of anything better :)

[nickdesaulniers]

• rebase, commit patchfile

[MaskRay]

Does this make all of

arc patch D129232
arc diff 'HEAD^'  # upload a patch but don't update the summary
arc diff --head=HEAD 'HEAD^' --verbatim  # upload a patch and update its summary with the local commit message
arc amend  # update the local commit message with the summary
arc branch

work?

[nickdesaulniers]

In D129232#3640103, @MaskRay wrote:

Does this make all of

arc patch D129232
arc diff 'HEAD^'  # upload a patch but don't update the summary
arc diff --head=HEAD 'HEAD^' --verbatim  # upload a patch and update its summary with the local commit message
arc amend  # update the local commit message with the summary
arc branch

work?

Yes, but arc branch produces the following exception for me:

$ arc branch
(Assuming "branch" is the British spelling of "branches".)
 USAGE EXCEPTION  Found more than 1,000 unpublished commits which are ancestors of heads.

That doesn't appear related/not sure what "success" looks like for that command since I've never run it before your suggestion.

llvm/docs/Phabricator.rst

 the command line. To get you set up, follow the
 `Arcanist Quick Start`_ instructions.
 
 You may need to apply a fix to your arcanist repo in order to submit a patch:
 
 .. code-block:: console
 
   % cd arcanist
-  % git fetch https://github.com/rashkov/arcanist update_cacerts
-  % git cherry-pick e3659d43d8911e91739f3b0c5935598bceb859aa
+  % git am <path/to/llvm-project>/llvm/utils/phabricator/*.patch
 
 You can learn more about how to use arc to interact with
 Phabricator in the `Arcanist User Guide`_.
 The basic way of creating a revision for the current commit in your local
 repository is to run:
 
 ::
 

llvm/utils/phabricator/0001-Fix-PHP-8.1-incompatibility-with-arc-patch-D-12345.patch

+From bb2e9a0394a48f4ef3576e8d91607e8129b2b716 Mon Sep 17 00:00:00 2001
+From: Nick Desaulniers <[e-mail]>
+Date: Fri, 8 Jul 2022 14:55:46 -0700
+Subject: [PATCH] Fix PHP 8.1 incompatibility with `arc patch D<12345>`
+
+Fixes the following observed error with PHP 8.1:
+
+  EXCEPTION: (RuntimeException) preg_match(): Passing null to parameter phacility#2 ($subject) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
+  arcanist(head=master, ref.master=acec17af414f)
+    #0 PhutilErrorHandler::handleError(integer, string, string, integer)
+    phacility#1 preg_match(string, NULL, NULL) called at [<arcanist>/src/repository/api/ArcanistGitAPI.php:603]
+    phacility#2 ArcanistGitAPI::getCanonicalRevisionName(NULL) called at [<arcanist>/src/repository/api/ArcanistGitAPI.php:1146]
+    phacility#3 ArcanistGitAPI::hasLocalCommit(NULL) called at [<arcanist>/src/workflow/ArcanistPatchWorkflow.php:433]
+    phacility#4 ArcanistPatchWorkflow::run() called at [<arcanist>/src/workflow/ArcanistPatchWorkflow.php:398]
+    phacility#5 ArcanistPatchWorkflow::run() called at [<arcanist>/scripts/arcanist.php:427]
+
+Link: https://secure.phabricator.com/book/phabcontrib/article/contributing_code/
+Link: https://reviews.llvm.org/D129232#3634072
+Suggested-by: Yuanfang Chen <[e-mail]>
+Signed-off-by: Nick Desaulniers <[e-mail]>
+---
+ src/differential/ArcanistDifferentialDependencyGraph.php | 3 ++-
+ src/lint/linter/ArcanistScriptAndRegexLinter.php         | 2 +-
+ src/repository/api/ArcanistGitAPI.php                    | 3 ++-
+ 3 files changed, 5 insertions(+), 3 deletions(-)
+
+diff --git a/src/differential/ArcanistDifferentialDependencyGraph.php b/src/differential/ArcanistDifferentialDependencyGraph.php
+index 64e2f9c71c20..fbc4e6594f0e 100644
+--- a/src/differential/ArcanistDifferentialDependencyGraph.php
++++ b/src/differential/ArcanistDifferentialDependencyGraph.php
+@@ -42,7 +42,8 @@ final class ArcanistDifferentialDependencyGraph extends AbstractDirectedGraph {
+     $edges = array();
+     foreach ($dependencies as $dependency) {
+       $dependency_revision = $this->getCommitHashFromDict($dependency);
+-      if ($repository_api->hasLocalCommit($dependency_revision)) {
++      if (phutil_nonempty_string($dependency_revision) &&
++          $repository_api->hasLocalCommit($dependency_revision)) {
+         $edges[$dependency['phid']] = array();
+         continue;
+       }
+diff --git a/src/lint/linter/ArcanistScriptAndRegexLinter.php b/src/lint/linter/ArcanistScriptAndRegexLinter.php
+index 0c3d9d9a11ca..b9f6924ec997 100644
+--- a/src/lint/linter/ArcanistScriptAndRegexLinter.php
++++ b/src/lint/linter/ArcanistScriptAndRegexLinter.php
+@@ -338,7 +338,7 @@ final class ArcanistScriptAndRegexLinter extends ArcanistLinter {
+     }
+ 
+     $line = idx($match, 'line');
+-    if (strlen($line)) {
++    if (phutil_nonempty_string($line) && strlen($line)) {
+       $line = (int)$line;
+       if (!$line) {
+         $line = 1;
+diff --git a/src/repository/api/ArcanistGitAPI.php b/src/repository/api/ArcanistGitAPI.php
+index 6c6d2ac42a19..13907d5634be 100644
+--- a/src/repository/api/ArcanistGitAPI.php
++++ b/src/repository/api/ArcanistGitAPI.php
+@@ -1143,7 +1143,8 @@ final class ArcanistGitAPI extends ArcanistRepositoryAPI {
+ 
+   public function hasLocalCommit($commit) {
+     try {
+-      if (!$this->getCanonicalRevisionName($commit)) {
++      if (!phutil_nonempty_string($commit) ||
++          !$this->getCanonicalRevisionName($commit)) {
+         return false;
+       }
+     } catch (CommandException $exception) {
+-- 
+2.37.0.rc0.161.g10f37bed90-goog
+