This is an archive of the discontinued LLVM Phabricator instance.

[llvm][docs] commit phabricator patch
ClosedPublic

Authored by nickdesaulniers on Jul 6 2022, 3:16 PM.

Details

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/

Diff Detail

Event Timeline

nickdesaulniers created this revision.Jul 6 2022, 3:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2022, 3:16 PM
Herald added a subscriber: StephenFan. · View Herald Transcript
nickdesaulniers requested review of this revision.Jul 6 2022, 3:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2022, 3:16 PM
ychen added a subscriber: ychen.Jul 6 2022, 3:42 PM

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) {

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 added inline comments.
llvm/docs/GettingStarted.rst
479–482 ↗(On Diff #442697)

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

ychen added a comment.Jul 6 2022, 3:57 PM

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 added a comment.EditedJul 6 2022, 6:45 PM

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 added a comment.EditedJul 7 2022, 12:44 AM

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 added inline comments.
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 added inline comments.Jul 7 2022, 11:11 AM
llvm/docs/GettingStarted.rst
479–482 ↗(On Diff #442697)

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

  • rebase, commit patchfile
nickdesaulniers retitled this revision from [llvm][docs] add another patch for `arc patch` fix to [llvm][docs] commit phabricator patch.Jul 8 2022, 3:10 PM
nickdesaulniers edited the summary of this revision. (Show Details)
MaskRay added a comment.EditedJul 8 2022, 3:57 PM

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?

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.

MaskRay accepted this revision.Jul 11 2022, 11:26 AM
This revision is now accepted and ready to land.Jul 11 2022, 11:26 AM
This revision was landed with ongoing or failed builds.Jul 11 2022, 12:34 PM
This revision was automatically updated to reflect the committed changes.