Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1.
Use `isset` instead to check if the argument "start" was passed to the CLI command.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=97e163187418), phorge(head=repoMaintStartHistory, ref.master=1c098c273d06, ref.repoMaintStartHistory=0a4a34143528)
#0 PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/repository/management/PhabricatorRepositoryManagementMaintenanceWorkflow.php:45]
```
Closes T15476
Test Plan: After applying this change, executing `./bin/repository maintenance --stop R1` on the CLI shows `Took repository "R1" out of maintenance mode.` as expected, instead of an exception.
Reviewers: O1 Blessed Committers, valerio.bozzolan, aklapper, avivey
Reviewed By: O1 Blessed Committers, valerio.bozzolan, avivey
Subscribers: Sten, avivey, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15476
Differential Revision: https://we.phorge.it/D25296
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=97e163187418), phorge(head=master, ref.master=1c098c273d06)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/repository/xaction/PhabricatorRepositoryMaintenanceTransaction.php:28]
```
Closes T15475
Test Plan:
Play a bit with maintenance messages:
./bin/repository maintenance --start "Start" R1
./bin/repository maintenance --start "Start Yeah" R1
./bin/repository maintenance --stop R1
Then visit `/diffusion/1/manage/history/` and look at `Diffusion put this repository into maintenance mode.` as expected, instead of an exception.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15475
Differential Revision: https://we.phorge.it/D25295
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=97e163187418), phorge(head=diffusionCallsignHistory, ref.master=9b99123ff931, ref.diffusionCallsignHistory=9b99123ff931)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/repository/xaction/PhabricatorRepositoryCallsignTransaction.php:28]
```
Closes T15465
Test Plan: After applying this change, `/diffusion/XYZ/manage/history/` renders correctly and shows `user set the callsign for this repository to XYZ.`
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15465
Differential Revision: https://we.phorge.it/D25289
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=97e163187418), phorge(head=master, ref.master=108cbcd09bd3)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/repository/xaction/PhabricatorRepositoryEncodingTransaction.php:20]
```
Closes T15460
Test Plan: After applying these two changes, changing the text encoding of a Diffusion repository to a valid encoding and going to `/diffusion/1/manage/history/`, page renders correctly and shows no exception.
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15460
Differential Revision: https://we.phorge.it/D25284
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
Closes T15459
Test Plan: After applying this change, going to `/diffusion/17/manage/`, the actual error (`cannot mkdir /var/repo/17: Permission denied`) is propagated and displayed in the "Working Copy Status" section, instead of being hidden by a `strlen()` exception. See T15459 for details.
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15459
Differential Revision: https://we.phorge.it/D25283
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
Passing null to strlen() is deprecated since PHP 8.1. Thus first check that the string is not null.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=b325304b6e52), phorge(head=master, ref.master=83edbffd521d)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/repository/xaction/PhabricatorRepositoryStagingURITransaction.php:20]
```
This is quite similar to D25277.
Closes T15458
Test Plan: After applying this change, editing the Staging URI by saving its already empty value, and going to the repo History page, the page `/diffusion/1/manage/history/` renders without an exception, showing `R1` being Inactive and `user set as the staging area for this repository.` [sic!]
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15458
Differential Revision: https://we.phorge.it/D25282
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=b325304b6e52), phorge(head=master, ref.master=cb938d869c3f)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/repository/xaction/PhabricatorRepositorySVNSubpathTransaction.php:25]
```
Closes T15445
Test Plan: After applying this change, and after setting some "Import Only" value for a new Subversion repository, the page `/diffusion/1/manage/history/` renders showing `R1` being Inactive and `user set the repository "Import Only" path to main`.
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15445
Differential Revision: https://we.phorge.it/D25277
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=b325304b6e52), phorge(head=diffusionCreateIdentity, ref.master=cbc0e661544a, ref.diffusionCreateIdentity=cbc0e661544a)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/repository/xaction/PhabricatorRepositoryIdentityAssignTransaction.php:55]
```
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=b325304b6e52), phorge(head=diffusionCreateIdentity, ref.master=cbc0e661544a, ref.diffusionCreateIdentity=cbc0e661544a)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php:298]
```
```
EXCEPTION: (RuntimeException) mb_detect_encoding(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=b325304b6e52), phorge(head=diffusionCreateIdentity, ref.master=cbc0e661544a, ref.diffusionCreateIdentity=cbc0e661544a)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<arcanist>/src/error/PhutilErrorHandler.php:261]
#1 <#2> mb_detect_encoding(NULL, array) called at [<phorge>/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php:313]
```
Closes T15440
Test Plan: After applying these three changes, going to `/diffusion/identity/edit/form/default/` and selecting the "Create Identity" button will only show a AphrontQueryException unrelated to PHP 8 and covered in T15439 instead.
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15440
Differential Revision: https://we.phorge.it/D25271
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=arcpatch-D25250, ref.master=18554ea76ceb, ref.arcpatch-D25250=fade4603a799), phorge(head=master, ref.master=e11c5486c92b)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/repository/xaction/PhabricatorRepositoryDefaultBranchTransaction.php:25]
```
Closes T15434
Test Plan: After applying this change, after changing the name of the (previously empty) default branch of a Diffusion repository, and then going to the repository History at `/diffusion/<someID>/manage/history/`, the page renders correctly in the web browser instead of showing an exception.
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15434
Differential Revision: https://we.phorge.it/D25265
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=b325304b6e52), phorge(head=diffusionCreateRepoURI, ref.master=d8d65f3f87ed, ref.diffusionCreateRepoURI=d8d65f3f87ed)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/repository/storage/PhabricatorRepositoryURI.php:755]
```
Closes T15444
Test Plan: After applying this change and setting a repo URI following the steps in T15444, the page "R10: URI 58" on `/diffusion/10/uri/view/58/` is correctly rendered without an exception.
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15444
Differential Revision: https://we.phorge.it/D25273
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
Closes T15421
Test Plan:
Applied this change, create a repository, and afterwards the repository page at `/diffusion/9/` is shown with its name and "Empty Repository - This repository does not have any commits yet."
- have a Diffusion repository with Staging Area and Automation already setup
- create a Differential patch from arc
- press the Land button on the UI and write random things inside
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15421
Differential Revision: https://we.phorge.it/D25247
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
Closes T15420
Test Plan: Applied these two changes; afterwards "Browse Repositories" overlay dialog gets rendered in web browser and lists existing repositories.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15420
Differential Revision: https://we.phorge.it/D25244
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
Similarly, `preg_match()` does not accept passing `null` as `$subject` parameter since PHP 8.1.
Closes T15337
Test Plan: Applied these four changes and `/diffusion/4/manage/` finally rendered in web browser.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15337
Differential Revision: https://we.phorge.it/D25182
Summary:
This change avoids some unnecessary uses of the strlen() function,
actually fixing some deprecation warnings in PHP 8.2.
In short, this is the suggested universal replace:
-if(strlen($v))
+if(phutil_nonempty_string($v))
And, if you know PHP, this is also another adoptable replace, but
only for cases where you are sure that the string "0" is not useful:
-if(strlen($v))
+if($v))
As usual the optimal solution depends on the contest.
Other similar patches will probably follow.
Closes T15222
Ref T15190
Test Plan:
- for the first time in my life, with this change, the unit tests are passed in PHP 8.2
- check with your big eyes that there are no obvious typos
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: avivey, speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15199, T15190, T15222
Differential Revision: https://we.phorge.it/D25104
Summary:
Ref T13588. See PHI2198. Some pathways represent SVN commit identifiers (which are numeric) with integers; these raise an exception in `phutil_nonempty_string()`, which is strict.
(Ideally, commit identifiers wouldn't have mixed types, but that's a dramatically less practical change.)
Test Plan: Will deploy.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21852
Summary: Ref T13682. Many subclasses of "CursorPagedPolicyAwareQuery" have the same implementation of "loadPage()", and this is a sensible default behavior.
Test Plan: Examined changes to verify that all removed methods have the same behavior.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21838
Summary: Ref T13676. Ref T13588. Fix some issues that prevent "bin/phd" and "bin/drydock" from executing under PHP 8.1, broadly because `null` is being passed to `strlen()`.
Test Plan: Ran `bin/phd debug task` and `bin/drydock ...` under PHP 8.1.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13676, T13588
Differential Revision: https://secure.phabricator.com/D21795
Summary: Ref T13658.
Test Plan:
This test plan is non-exhaustive.
- Viewed "remarkup.process" Conduit method API page.
- Viewed URIs in a Diffusion repository.
- Viewed editor protocol configuration in Settings.
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21772
Summary:
Ref T13667. The Herald "content added" rule (and other similar rules) do not correctly extract content from empty commits.
When we load an empty raw diff, return an empty changed content map.
Ref T13588. Also fix some PHP8.1 null/string stuff
Test Plan:
- Ran "bin/repository reparse --publish <commit>", with an empty commit hash and a nonempty commit hash.
- Reviewed Herald transcripts for general sanity.
Maniphest Tasks: T13667, T13588
Differential Revision: https://secure.phabricator.com/D21761
Summary:
When previously known commits have been destroyed in a Mercurial repository, Phabricator does not end up marking the commits as unreachable. This results in daemon tasks which continuously fail and retry.
This updates `PhabricatorRepositoryDiscoveryEngine` and `PhabricatorManagementRepositoryMarkReachableWorkflow` to include support of marking commits as unreachable for Mercurial repositories.
The `PhabricatorMercurialGraphStream` also needed updated to support a stream with no starting commit.
Refs T13634
Test Plan:
1. I set up a hosted Mercurial repository.
2. I removed the head commit from the on-disk repository state.
3. I attempted to load the repository page and saw an exception due to a missing commit.
4. I went to `/manage` for the repository and scheduled an update of the repository.
5. After an updated performed, I went to the repository main page and saw there was no exception and the history view properly did not have the commit I had removed.
6. I checked the phd logs and verified there were no exceptions related to the repository.
7. I ran the `./bin/repository mark-reachable` command on the Mercurial repository and it reported that it marked the commit as unreachable.
8. I pushed the same commit back upstream and verified that the commit was found and displayed in the history view of the repository page and `mark-unreachable` did not identify it as being unreachable.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T13634
Differential Revision: https://secure.phabricator.com/D21715
Summary:
Depends on D21720. Ref T13666. See D21720 for additional discussion.
Use "MethodCallFuture", introduced in D21720, so that exceptions raised in "execute()" are thrown when the future is resolved, not when the future is created.
This makes exception behavior for clustered and non-clustered setups consistent, and chooses the intended (clustered) behavior in both cases, which currently deals with errors better.
Test Plan:
- Applied both parts of the patch in T13666 (break history queries, force immediate futures) to reproduce the issue in T13666.
- Loaded a Diffusion landing page, reproduced the error described in that task.
- Applied this patch, verified landing page works again.
- Removed the "break history queries" change, verified landing page works in forced-immediate mode.
- Removed the "force immediate" change, verified landing page works in "actual future" mode.
Reviewers: cspeckmim
Reviewed By: cspeckmim
Maniphest Tasks: T13666
Differential Revision: https://secure.phabricator.com/D21721
Summary: Ref T13660. Clean up callsites to "PhutilExecPassthru->execute()" to prepare to deprecate it.
Test Plan:
- Grepped for "PhutilExecPassthru" and looked for callsites.
- Ran `GIT_SSH=.../ssh-connect git ls-remote origin` to execute the "ssh-connect" code.
- The two passthru future methods have no callers and could possibly be removed, but I'm just letting sleeping dogs lie for now.
Reviewers: cspeckmim
Reviewed By: cspeckmim
Maniphest Tasks: T13660
Differential Revision: https://secure.phabricator.com/D21703
Summary:
Refer to discussion on D21677#275541
Refs D21681 (arcanist changes)
Phabricator has several uses of the `--debug` flag being used with Mercurial. Use of this flag causes additional output to be added which Phabricator needs, however the behavior of `--debug` is not guaranteed to be stable, and in newer versions of Mercurial there have been additional output that has caused Phabricator to choke on parsing the output. This change removes several uses of `--debug` in favor of using `--template` with the `hg log` or `hg annotate` commands in combination with the `{p1.node}` or `{p2.node}` template format.
The use of `{p1node}` format in templates was added in [[ https://www.mercurial-scm.org/wiki/WhatsNew/Archive#Mercurial_2.4_.282012-11-01.29 | Mercurial 2.4 (2012) ]]. This format was deprecated in [[ https://www.mercurial-scm.org/wiki/WhatsNew#Mercurial_4.9_.282019-02-01.29 | Mercurial 4.9 (2019) ]] in favor of using `{p1.node}` format which is unclear when this new format was added (presumably earlier than Mercurial 4.9).
The use of `--template` with `hg annotate` is only officially supported in [[ https://www.mercurial-scm.org/wiki/Release4.6 | Mercurial 4.6 (2018) ]], though does appear to work in 4.5 but is not documented.
Since the `{p1node}` format was introduced in 2.4 this bumps the required version of `hg` to 2.4 (from 1.9). Since the `annotate --template` feature wasn't added until 4.6 (which is still fairly recent), the use of it is gated on a capability test, but still preferred for use where possible to avoid extraneous output from `--debug` flag.
Test Plan:
I verified I could do the following in a mercurial repository, while having mercurial 5.8 installed:
1. Navigate and view files in Diffusion under e.g. `/source/test-repo/`.
2. While viewing a file in Diffusion verified that I could view the blame of the file and the history/annotations looked accurate for the files I was browsing.
3. From the blame sidebar, select to view a commit which loaded and displayed changes properly.
4. View the history of the repository under e.g. `/source/test-repo/history/default/`. I verified the history looked correct and the tree-like structure showing relationship of commits also looked accurate.
I setup mercurial to run version 4.4, created a new repository, added some commits, and verified all the above behavior still works properly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D21679
Summary:
Ref T13658. This just scrubs some of the simple references from the codebase.
Most of what's left is in documentation which won't be relevant for a fork and/or which I need to separately revise (or more-or-less delete) at some point anyway.
I removed the "install RHEL" and "install Ubuntu" scripts outright since I don't have any reasonable way to test them and don't plan to maintain them.
Test Plan: Grepped for "phacility", "epriestley"; ran unit tests.
Reviewers: cspeckmim
Reviewed By: cspeckmim
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21678
Summary: Found a few typos which could be updated.
Test Plan:
I tested the Configuration page change by navigating to `/config` and verifying the page title set in the browser as well as the page title text on the page
|Before|After|
|---|---|
|{F9013208}|{F9013210}|
|{F9013300}|{F9013301}|
I verified the Conduit error message by navigating to `/auth/start/?__conduit__=1`
{F9013289}
The CircleCI error message was not verified due to the involvement of testing with CircleCI however the change is very minor and has very little risk of impacting any functionality.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D21675
Summary: Ref T13614. Provide "bin/repository lock" to temporarily lock repositories for manual maintenance.
Test Plan:
- Read instructions.
- Used `bin/repository lock` according to the instructions.
- Saw Storage tab in Diffusion report lock held during maintenance, released after it completes.
- Saw "maintenance" push log generated and repository version bump.
- Tried to lock some invalid repositories.
Maniphest Tasks: T13614
Differential Revision: https://secure.phabricator.com/D21671
Summary:
Ref T13614. When a script holds the write lock but modifies the repository directly (rather than by pushing), the repository version won't change when the script releases the write lock. Thus, the writes may not propagate to other nodes (it depends which node lucks out and accepts the next write).
To guarantee that writes propagate, allow these scripts to pretend they pushed the repository. These are bare-bones valid events flagged as "Maintenance".
Test Plan:
- Wrote a script to hold the write lock, wait (or pretend to do something), then release the write lock.
- Applied patches, modified script to use new APIs ("newMaintenanceEvent()").
- Ran script, saw repository verison bump and relevant push logs:
{F8814923}
Maniphest Tasks: T13614
Differential Revision: https://secure.phabricator.com/D21670
Summary: Ref T13641. Make "active bindings" a real query and make callers that only care about active bindings only query for active bindings.
Test Plan:
- Queried for "bindings" and "activeBindings" via Conduit.
- Disabled/enabled devices, saw binding status update in UI.
- Loaded Diffusion cluster layout.
- Grepped for `needBindings()`, `getActiveBindings()`, etc.
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21628
Summary:
Ref T13395. "libphutil/" was stripped for parts, but some documentation still references it. This is mostly minor corrections, but:
- Removes "Javelin at Facebook", long obsolete.
- Removes "php FPM warmup", which was always a prototype and is obsoleted by PHP preloading in recent PHP.
Test Plan: `grep` / reading
Maniphest Tasks: T13395
Differential Revision: https://secure.phabricator.com/D21624
Summary:
Ref T13631. Move "PhabricatorAuditStatusConstants" to a more modern object ("PhabricatorAuditRequestStatus").
Expose the status value via Conduit.
Test Plan:
- Ran `bin/audit delete`.
- Viewed a commit with auditors in the web UI.
- Grepped for affected symbols.
- Called Conduit with the "auditors" attachment, saw auditor statuses.
Maniphest Tasks: T13631
Differential Revision: https://secure.phabricator.com/D21599
Summary:
Ref T13631. See that task for discussion.
- "NONE": Probably never used?
- "CC": Obsoleted by subscribers.
- "AUDIT_NOT_REQUIRED": For Owners packages, obsoleted by edges.
- "CLOSED": For "Close Audit", obsoleted by "Request Verification".
Test Plan:
- Grepped for constants, browsed Diffusion.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13631
Differential Revision: https://secure.phabricator.com/D21598
Summary: Ref T13631. For now, this only shows the auditor PHID. The current status constants could use some cleanup before they're exposed.
Test Plan: Queried with "auditors" attachment, saw basic auditor information.
Maniphest Tasks: T13631
Differential Revision: https://secure.phabricator.com/D21597
Summary: Ref T13611. This property worked correctly when implemented in D19357. The behavior was broken by D20775, which tested node-level routing but did not specifically re-test the "writable" property. This was difficult to spot because ref query outcomes weren't observable in the UI, and the ref itself had the correct property value.
Test Plan:
See D21575. After this change, the UI shows the correct state, rather than showing a read-only service ref as writable:
{F8465865}
Maniphest Tasks: T13611
Differential Revision: https://secure.phabricator.com/D21576
Summary:
Ref T13620.
- Make generic edge stories render links with hovercards. Other story types (like subscriptions) already do this so I'm fairly certain this is just old code from before hovercards.
- Include a longer commit message snippet in hovercards.
Test Plan: {F8465645}
Maniphest Tasks: T13620
Differential Revision: https://secure.phabricator.com/D21574
Summary:
Ref T13600. When migrating observed repositories between cluster services, impact can be better controlled by fetching a copy of the repository on the target host before clusterizing it.
In particular, in the Phacility cluster, migrations are generally from one shared shard to one dedicated shard. It's helpful to perform these migrations synchronously without waiting for the cluster to sync in the background (helpful in the sense that there are fewer steps and fewer commands to run).
This supports an "--observe" mode to the internal "bin/services load-repository" workflow, which transfers repository data by refetching it from the remote rather than by getting it from the older host. This fetch occurs before cluster configuration is adjusted.
Test Plan: Ran locally as a sanity check, will apply in production.
Maniphest Tasks: T13600
Differential Revision: https://secure.phabricator.com/D21544
Summary: Ref T13591. This is a minor consistency change to use PHIDs instead of IDs in the commit import processing pipeline. PHIDs are generally more powerful in more contexts and it would be unusual for a modern worker to use an ID here.
Test Plan:
- Made the "accept either ID or PHID" part of the change only.
- Pushed a commit, parsed and reparsed it step by step (this tests that "commitID" tasks can still process normally).
- Made the "write PHIDs" part of the change.
- Pushed a commit, parsed and reparsed it step by step.
- Looked at the task row in the database, saw PHID data.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21533
Summary:
Ref T13591. Improve how parameters are passed between commit worker tasks:
- Always pass "via", to track where tasks came from.
- Always provide "objectPHID" (with the commit PHID).
- Always provide "containerPHID" (with the repository PHID).
Test Plan:
- Pushed a new commit.
- Ran `bin/repository pull` + `bin/repository discover`, saw commit with all parameters.
- Ran `bin/worker execute ...`, saw a Change worker and then a Publish worker with appropriate parameters.
- Ran `bin/repository reparse ... --background`, saw workers queue with appropriate parameters.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21532
Summary:
Ref T13590. Currently, when you encounter a HTTP error in Git, there is no apparent way to make the client show any additional useful information. In particular, the response body is ignored.
We can partially get around this by putting the information in an "X-Phabricator-Message: ..." HTTP header, which is visible with "GIT_CURL_VERBOSE=1 git ...". Users won't normally know to look here, but it's still better than nothing.
Test Plan:
- Ran "GIT_CURL_VERBOSE=1 git fetch" against a Phabricator HTTP URI that returned a HTTP/500 error.
- Before: no clue what happened on the client.
- After: client shows useful message in the "X-Phabricator-Message" header in debug output.
Maniphest Tasks: T13590
Differential Revision: https://secure.phabricator.com/D21523
Summary:
Ref T13593. The commit cache in this Engine has a maximum fixed size (currently 65,535 entries).
If we execute discovery in a repository with more refs than this (e.g., 180K), we get fast lookups for the first 65,535 refs and slow lookups for the remaining refs.
Instead, divide the refs into chunks no larger than the cache size, and perform an explicit cache fill before each chunk is processed.
Test Plan:
- Created a repository with 1K refs. Set cache size to 256. Ran discovery.
- Before patch: saw one large cache fill and then ~750 single-gets.
- After patch: saw four large cache fills.
- Compared `bin/repository discover ... --verbose` output before and after patch for overall effect; saw no differences.
Maniphest Tasks: T13593
Differential Revision: https://secure.phabricator.com/D21521
Summary: Ref T13591. Fixes a few issues with the recent updates here discovered in more thorough testing.
Test Plan:
- Stopped the daemons.
- Created a new copy of Phabricator in Diffusion.
- Pulled it with `bin/repository pull ...`.
- Got 17,278 commits on disk with `git log --all --format=%H`.
- Set permanent refs to "master".
- Discovered it with `bin/repository discover ...`.
- This took 31.5s and inserted 17,278 tasks.
- Verified that all tasks have priority 4,000 (PRIORITY_IMPORT).
- Observed that 16,799 commits have IMPORTED_PERMANENT and 479 commits do not.
- This matches `git log master --format=%H` exactly.
- Ran `bin/repository refs ...`. Expected no changes and saw no changes.
- Ran `bin/worker execute --active` for a minute or two. It processed all the impermanent changes first (since `bin/worker` is LIFO and these are supposed to process last).
- Ran `bin/repository refs`. Expected no changes and saw no changes.
- Marked all refs as permanent.
- Starting state: 16,009 message tasks, all at priority 4000.
- Ran `bin/repository refs`, expecting 479 new tasks at priority 4000.
- Saw count rise to 16,488 as expected.
- Saw all the new tasks have priority 4000 and all commits now have the IMPORTED_PERMANENT flag.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21518
Summary:
Ref T13591. There are currently two pathways to queue an import task for a commit: via repository discovery, or via a ref becoming permanent.
These pathways duplicate some logic and have behavioral differences: one does not set `objectPHID` properly, one does not set the priority correctly.
Unify these pathways, make them both set `objectPHID`, and make them both use the same priority logic.
Test Plan:
- Discovered refs.
- See later changes in this series for more complete test cases.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21516
Summary:
Ref T13591. Currently, the "IMPORTED_PERMANENT" flag (previously "IMPORTED_CLOSEABLE", until D21514) flag is set by using the result of "shouldPublishRef()".
This method returns the wrong value for the flag when there is a repository-level reason not to publish the ref (most commonly, because the repository is currently importing).
Although it's correct that commits should not be published in an importing repository, that's already handled in the "PublishWorker" by testing "shouldPublishCommit()". The "IMPORTED_PERMANENT" flag should only reflect whether a commit is reachable from a permanent ref or not.
- Move the relevant logic to a new method in Publisher.
- Fill "IMPORTED_PERMANENT" narrowly from "isPermanentRef()", rather than broadly from "shouldPublishRef()".
- Deduplicate some logic in "PhabricatorRepositoryRefEngine" which has the same intent as the logic in the Publisher.
Test Plan:
- Ran discovery on a new repository, saw permanent commits marked as permanent from the beginning.
- See later changes in this patch series for additional testing.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21515
Summary:
Ref T13591. This is an old flag with an old name, and there's an import bug because the outdated concept of "closable" is confusing two different behaviors.
This flag should mean only "is this commit reachable from a permanent ref?". Rename it to "IMPORTED_PERMANENT" to make that more clear.
Rename the "Unpublished" query to "Permanent" to make that more clear, as well.
Test Plan:
- Grepped for all affected symbols.
- Queried for all commmits, permament commits, and impermanent commits.
- Ran repository discovery.
- See also further changes in this change series for more extensive tests.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21514
Summary:
Ref T13591. Since D8781, this flag does not function correctly in Git and Mercurial repositories, since ref discovery pre-fills the cache.
Move the "don't look at the database" behavior the flag enables into the cache lookup. D8781 should have been slightly more aggressive and done this, it was just overlooked.
Test Plan:
- Ran `bin/repository discover --help` and read the updated help text.
- Ran `bin/repository discover --repair` in a fully-discovered Git repository.
- Before: no effect.
- After: full rediscovery.
Maniphest Tasks: T13591
Differential Revision: https://secure.phabricator.com/D21513
Summary:
Ref T13589. In D21510, not every ref selector got touched, and this isn't a valid construction in Git:
```
$ git ls-tree ... -- ''
```
Thus:
- Disambiguate more (all?) ref selectors.
- Correct the construction of "git ls-tree" when there is no path.
- Clean some stuff up: make the construction of some flags and arguments more explicit, get rid of a needless "%C", prefer "%Ls" over acrobatics, etc.
Test Plan: Browsed/updated a local Git repository. (This change is somewhat difficult to test exhaustively, as evidenced by the "ls-tree" issue in D21510.)
Maniphest Tasks: T13589
Differential Revision: https://secure.phabricator.com/D21511
