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?

"Cascading" is updating all local refs which depend on refs that were selected-and-modified by the land operation. So if you have dependent features in several dependent branches named change1 -> change2 -> change3 locally, and arc land change2, we want to (in the general case) rebase your local change3 onto the landed version of change2 so that it no longer has out-of-date ancestors.

In D21679#275638, @epriestley wrote:

This specific issue is probably the relevant one, the other thing linked from there looks like it's just a --debug problem, which this patch should fix.

https://discourse.phabricator-community.org/t/error-cloning-mercurial-repository-http-after-first-install/1200/2

Thank you for the tips.

1. Bring the 2.4 version bump upstream, without directly addressing annotate --debug.
2. Deal with annotate --debug by either:
   1. bumping the minimum version to 4.6 in your fork and using --template; and/or
   2. defining another Mercurial version capability test for annotate --template and selecting that in versions of Mercurial which support it (this can come upstream).

Your call on whether you want to do the legwork on (2B).

I'll plan to do (2B) now that I'm more familiar/comfortable with the version-checking approach. Bumping the required version to something so much newer isn't really justified for this, especially considering the versions that are available on official distribution packages (centos7 currently has mercurial-2.6). Using --template for newer versions will be less error-prone though so I'll do the legwork. I'll try to get something up later tonight.

This specific issue is probably the relevant one, the other thing linked from there looks like it's just a --debug problem, which this patch should fix.

Requiring very recent Mercurial may also run into issues described here:

reconcileLocalState() is another possible place to put the post-push update logic -- that runs exactly once, and a little later.

A possible approach would be to try to put the "failure" commands into ArcanistMercurialLocalState. I think that would look like snapshotting every bookmark (not just the active one), and then resetting all bookmarks when restoring state. I'm not really sure if this would be better -- your approach seems fairly reasonable to me, and has the advantage of doing all the bookmark management pretty narrowly in one place, and this should be the only place that this work needs to happen. Offhand, I think I favor the approach you've taken here.

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.

Ah, I didn't realize annotate --template arrived so recently. Phacility production has Mercurial 2.8.2 and I can't easily fix that at the moment, so I don't want to bring version bump all the way to 4.6 upstream today. I'm okay with the jump to 2.4 upstream, and also okay with leaving annotate --debug without filtering upstream (since no one has complained in all this time, and not addressing it doesn't make things worse). So I'd propose:

This is still in early stage but marking for review because I wanted to get some early feedback if I'm on the right track. With these changes when I land a diff with other arbitrary local commits the pull -B master that it ends up running does pull down and update the local master bookmark to the right revision, on a hosted mercurial repository. The changes to splitting the $tail command array into two separate arrays feels a bit off to me. As for updating the working directory I think that change is going to be a bit more involved but I'm not sure if that's something appropriate to be done in pushChange() or that should happen higher up in ArcanistLandEngine::exeute() by adding some other functions to be overridden by the different engines or similar.

"fix" "typos"

Okay I have these changes in and verified that things seem to be working fine with mercurial 5.8, though I'm not sure your thoughts on requiring mercurial 4.6 as the minimum version. If we restore the hg annotate --debug then the minimum version required would only jump to 2.4.

I should probably just set up a blog or something since I don't expect to bring any microcontroller components upstream, but until then:

According to their changelog the {p1node} style was added in 2.4 (2012) and was deprecated in 4.9 (2019) in favor of the new {p1.node} format
https://www.mercurial-scm.org/wiki/WhatsNew/Archive#Mercurial_2.4_.282012-11-01.29

I started a draft change in D21679. I also have changes for rARC but haven't yet created a diff for that (or yet figured out the exact versions to define)

Ah thank you for those details for the better change. I was planning to play around with what would be needed to remove the --debug flag(s) but I hadn't yet gone into the details. I'll take a look at these changes. From quick testing on recent mercurial versions (5.8) it looks like the output does not include the local revision number, e.g instead of
693:8b39f63eb209dd2bdfd4bd3d0721a9e38d75a6d3 -1:0000000000000000000000000000000000000000
it prints out
8b39f63eb209dd2bdfd4bd3d0721a9e38d75a6d3 0000000000000000000000000000000000000000

(DiffusionLowLevelMercurialPathsQuery has an example of using PhutilBinaryAnalyzer to do a Mercurial capability test.)

On navigating the version stuff, I think the pathway would be:

Keep the protocol/domain on one line and the path on another (in this case path is <80 chars)?

For the invalid branch cache message remove the (served): bit which might differ on hosted vs. observed repositories.

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.

Ah I did come across these as well and forgot to note.

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.

Proof-of-concept only for class_alias():

This is a bit out there, but: PHP once had something called classkit and later runkit, which perhaps exists as runkit7 now, that let you dynamically rename classes at runtime. In theory, you could use this as a compatibility layer to support mass renaming of classes: if code asks for PhabricatorXYZ, look for GenericXYZ (or XYZ, or consult a mapping), then make a runtime copy of the class. This narrow capability may be in PHP's core as class_alias(), now.

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

As another test I tried creating a new commit on Windows and using smart-quotes and em-dash in the commit message and mercurial bombed with

$ hg ci # nvim opened an editor in UTF-8 encoding mode and let me write in a commit message: Test commit with “smart quotes” and "em—dash"
transaction abort!
rollback completed
note: commit message saved in .hg\last-message.txt
note: use 'hg commit --logfile .hg/last-message.txt --edit' to reuse it
abort: decoding near 't quotesΓÇ¥ and "emΓ': 'charmap' codec can't decode byte 0x9d in position 34: character maps to <undefined>!

I ran through another test just in case and things do seem to function fine. Another interesting point is when using nvim on Windows it will properly render the smart-quotes and em-dash, but mercruial's output uses the weird characters. Just in case I made sure to test having smart quotes and em-dash both in the Title and Summary fields as well as the content of the file change. Patching and landing the change from Windows, then pulling the commit onto the Mac and the end result on the Mac looks correct for all places.

The results in T13649 -- where none of these results actually print a character that anyone would consider to be a "smart quote" (?) -- aren't exactly inspiring, but I think this is a step forward at least.

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

I think the global flag is reasonable.

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();

Oops -- I assumed you were already in Blessed Committers. I added you, you should be able to either arc land or "Land Revision" now (if arc diff uploaded to staging properly). See that project description for some details, or yell if you're still having issues.

For landing the revision can that only be done through the "Land Revision" button? It's greyed out for me.

Re-balance the text line formatting with the new phrasing

Update the phrasing

Oh, just for completeness -- you can trigger the first message by visiting this URI while logged out (e.g., in an incognito window):

• Allow "owners.search" API to read this setting.

Thank you! Looks like it worked.

I added you to Blessed Committers, so you should be able to land this yourself. See that project description for guidance, or let me know if you run into issues.

(That was really me.)

I did see that, but @epriestley (or possibly an impostor) said:

I think a reasonable strategy for the moment is just to send patches upstream for review normally (with me as a reviewer) until I stop getting to them or it's clear that we don't have similar viewpoints on what constitutes a desirable patch.

Wondering if you saw:

Effective June 1, 2021: Phabricator is no longer actively maintained.

My company was a loving user of Phabricator (local server!) for over 10 years. I still fondly remember the discussions here and talking through new applications, testing changes, and so much more. I know that the use of this tool permanently shaped hundreds of engineers' views on code quality, review, the software development lifecycle, and so on. It will be missed.