As an example, given this code

private $name;

I was ready to suggest that after further investigation was made, recalling the "recent" refactor work of the mercurial command-with-extensions changes we looked at 😄

Gotcha. I'm going to take a swing at updating past 8.0 and see what crops up. Also thanks for the tip with PHPAST. I haven't looked too much into it other than trying to get it working on Windows a year or so back

Would this be reasonable to address with a new server-side setup check, similar to the checks for post_max_size and op_cache values?

In T13588#256584, @epriestley wrote:

PHP 8.1 disallows strlen(null).
...
Broadly, Phabricator likely will not pass the empty string as a constraint in these cases...
...
D21762 introduces phutil_nonempty_string(), which is like this test but slightly stricter (it raises an exception for wonky values) and easier to revisit later (i.e., use of this function is a hint that the code can be made more strict about value types).
...
Upshot:

• if (strlen($maybe_null)) may be replaced in all cases with if (phutil_nonempty_string($maybe_null)) to work in PHP 8.1 and not break any code which wasn't already extremely suspicious (...stringlike() may be required if the value may be an object, usually a URI object).
• This is usually not the most desirable replacement, but probably 95% of these aren't realistically worth being surgical about.
• In new code, prefer increasing type strictness over these more-flexible tests.

Thank you!

A user just ran into this exception with Arcanist (current master changeset) while running arc diff (using Mercurial) with PHP 8.1. I haven't looked too in depth but it seems similar to other PHP 8.1 issues.

[2022-05-17 20:49:47] EXCEPTION: (RuntimeException) substr(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=3cc486d5c156)
  #0 PhutilErrorHandler::handleError(integer, string, string, integer)
  #1 substr(NULL, integer, integer) called at [<arcanist>/src/repository/api/ArcanistRepositoryAPI.php:807]
  #2 ArcanistRepositoryAPI::getDisplayHash(NULL) called at [<arcanist>/src/repository/marker/ArcanistRepositoryMarkerQuery.php:67]
  #3 ArcanistRepositoryMarkerQuery::execute() called at [<arcanist>/src/repository/query/ArcanistRepositoryQuery.php:20]
  #4 ArcanistRepositoryQuery::executeOne() called at [<arcanist>/src/repository/api/ArcanistMercurialAPI.php:988]
  #5 ArcanistMercurialAPI::getActiveBookmark() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:2253]
  #6 ArcanistDiffWorkflow::buildDiffSpecification() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:380]
  #7 ArcanistDiffWorkflow::run() called at [<arcanist>/scripts/arcanist.php:427]
<<< [1] (+8,567) <exec> 8,567,426 us

I find this very intriguing.

Phabricator would benefit from having access certain services (full-text search, full-codebase search, repository graph storage) that very likely aren't practical to write in PHP

Is the purpose of implementing introspection of libraries to allow for building native components that would get picked up and auto-loaded by arcanist?

Browsing Diffusion (namely, loading a file while the published changeset is head, and trying to Skip past commit):

I couldn't immediately reproduce this; it's also probably a bit of a messy fix.

I'll plan to investigate further if possible. I believe in this case the repository was running in observed mode rather than hosted in case that might be a factor.

Ah thank you!

@epriestley - the fix for this landed in the master branch but it doesn't look like that has carried over yet into the GitHub mirror which is used as the install location for most users, and is the current recommendation on https://secure.phabricator.com/book/phabricator/article/arcanist_quick_start/. Is there a delay before the mirror observes the changes or does something need poked for it to propagate?

So that part is correct, but swapping the commit and branch was incidental, and I overlooked it because we usually get the same result: in common cases, this is moot because ancestors(<branch name>) is equivalent to ancestors(<heads of that branch>), and branch(<commit hash>) is equivalent to branch(<branch of that commit>). The latter is always equivalent; the former is equivalent when commit is the single head of branch, which it almost always is in regular use.

But this behavior (on this install) is unambiguously wrong:

...since b7be5c961297 (the parameter) is an ancestor of 162a9c1e8e44 (the first commit in the result set). I'll send you a diff.

One tangential thing I noticed from the error messages in these cases, in DiffusionHistoryQueryConduitAPIMethod the getMercurialResult() function executes this hg command

$path_args = array();
if (strlen($path)) {
  $path_args[] = $path;
  $revset_arg = hgsprintf(
    'reverse(ancestors(%s))',
    $commit_hash);
} else {
  $revset_arg = hgsprintf(
    'reverse(ancestors(%s)) and branch(%s)',
    $drequest->getBranch(),
    $commit_hash);
}

With just the updates to the future handling this is now the landing page I see with missing commits:

I'm able to reproduce this but I'll investigate further

I think I did conflate this partially - namely that this isn't directly caused by the issue discussed in T13365. Where this issue came up for me is while testing out D21715, I removed the head commit from the on-disk repo which caused the landing page to not render properly:

In Mercurial, I'm not exactly sure how wide this window is. Since you normally can't strip commits in the remote by pushing (AFAIK?)

This is true, though it does change a bit if we enable evolve server-side. Then when commits/branches are pruned when pushing/pulling those obsolete markers will also be transferred, allowing similar disappearing commits (retrievable via —hidden). However I believe by default mercurial does not allow pruning public changesets (would require forcibly changing phases on client/server, or running a server/branch in non-publishing mode). Supporting evolve/topics server-side is probably large enough that it would be its own big project. So I think this assumption holds up well.

Another caveat here is that if you destroy commits directly in Phabricator's working copy in certain states, it's possible it won't be able to trace ancestry to find all the commits you destroyed in order to mark them unreachable. However, this window is narrow. For Git, you can bin/repository mark-reachable to rebuild reachability if you know you've done a bunch of dangerous mutations to the on-disk state.

Marking this as resolved by D21700. If there are further details or input we can re-open to further address.

I'm also surprised that rebase extension isn't enabled by default. I guess I have been turning it on in my default setup.

I like that idea. I'll take a look.

One thought here is that updating variables_order is fine on the server, but I really don't want to force arc users to muck with php.ini.

This seems to be resolved already but just wanted to add a note - I believe Windows users currently need to modify php.ini to enable the curl extension for arcanist to work. This is from our internal documentation for installing PHP for arcanist:

Browse to C:\tools\php[some version number]. Edit php.ini as listed below.

• Note: The following modifications remove the semi-colon at the start of the line. Semi-colons are comments in .ini files and these changes are uncommenting and updating the lines.
  • Find the line ;extension=curl and remove the semicolon.
  • Locate the line ;date.timezone = and replace it with date.timezone = UTC to prevent PHP from yelling at you.

One other case where "Phabricator" appears is in HTTP and Email headers, e.g. X-Phabricator-XYZ. For a separate project wanting to update these I think a slow migration approach is needed, to allow recipients currently expecting the existing fields. I'm guessing there isn't a reasonable change here for the upstream.

Depending on how many of these we end up with, I think an email-header-prefix sort of config option might be reasonable.

Oh and then for existing installs this would just default to X-Phabricator-XYZ to continue with existing behavior?

In T13658#255994, @epriestley wrote:

Create Platform::getProjectRepo() to return "phabricator", or instead detect the repository name.

I suspect having this method do anything nontrivial may lead to some weird boostrapping issues, where it gets called very early during startup or with a partially built environment or during exception handling or whatever and not everything you expect to be available is actually available, so I suspect you'll have the best luck by just hard-coding this.

(For example, Phabricator reads PHABRICATOR_INSTANCE from the environment very early.)

Ah I did have a suspicion that would be tricky but was thinking to give it a shot. Hardcoding is probably just going to be easiest.

Interestingly this is the output I get when using hg log to print out the commit message containing smart quotes with different encodings -- this behavior is the the same for both PowerShell (chcp 65001) and cmd.exe (chcp 437) and no errors occur.

$ hg log -r tip --template "{desc}" # "default" encoding
A test

In Mercurial, with certain character sets, some arc workflows may fail in arc-hg while trying to perform UTF8 operations.

I'm doing some testing to find an appropriate update to arcanist for this. I ran into another issue, when patching down a revision which has unicode characters in the summary on a windows machine. I could try to whack-a-mole these different issues but since Mercurial's --encoding argument is a global option do you think it would be wise to update ArcanistMercurialAPI::buildLocalFuture() with this?

diff --git a/src/repository/api/ArcanistMercurialAPI.php b/src/repository/api/ArcanistMercurialAPI.php
index e5c2078b..ac72c356 100644
--- a/src/repository/api/ArcanistMercurialAPI.php
+++ b/src/repository/api/ArcanistMercurialAPI.php
@@ -15,7 +15,7 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI {
   protected function buildLocalFuture(array $argv) {
     $env = $this->getMercurialEnvironmentVariables();

Thank you @epriestley and all of Phacility. Phabricator is a shining example of a well-engineered product that itself encourages better engineering processes and having fun while doing so. I had a great time learning and using Phabricator and it has left a big impact. I only wish I could have been more involved in the development and community. Wishing everyone the best!

Hmm okay that might only take effect at the time of making commits, though I've run into other issues where phabricator/arcanist fails to parse the contents of a commit message and forcing it to interpret as UTF-8 via --encoding utf-8 in the mercurial command has worked.

To clarify I think having the environment variable set as HGENCODING=utf-8 would be a possible solution/workaround.

I have not tried this out yet but mercurial claims to respect an HGENCODING environment variable (https://www.mercurial-scm.org/doc/hg.1.html#environment-variables). It's possible that setting this on the phabricator server might fix the issue by affecting any hg commands run by phabricator. Note that the environment variable would need to be set/exported for both the system user account which phabricator web server runs under (like httpd or nginx) and the system user account which the daemons run under, as they both run mercurial commands.

Adding a data point, "Foist Upon" was a surprise hit among the developers here and additionally we have successfully utilized the conduit API to take advantage of foisting

There are probably also some off-label use cases where, for example, bots create revisions on behalf of users. I'm not thrilled about these, ...

oops 😬

Would the use of an extension make additional setup necessary for observed repositories vs. hosted repositories, or are observed repositories out of scope for these set of changes?

Wow thank you for this. I came across a diff that mixed a lot of whitespace with minimal code change it's much easier to differentiate what happened.

ugh, zucchini

Mercurial 4.6 has added hg help internals.bundle2 which will render a book to your console with details about the format/protocol. I did a high-speed skim and then tried out hg help internals.changegroup on a whim which produced an even longer book about the stream protocol (SSH only maybe?).