I take it these disparate trees have some limitations? Do they always apply to the working directory or something?

In Git, we can just create new commits floating out in the middle of nowhere -- not on any branch, and not the ancestor of any labeled commit -- that you can retrieve if you know the hash but aren't otherwise reachable or visible to users without using very low-level commands. This isn't normally useful for human users, but it's great for reducing side effects in arc land, since we can do all our work in a sort of temporary scratch area that automatically gets cleaned up later.

Woah, yea I don't think there's anything like this in Mercurial. I take it these disparate trees have some limitations? Do they always apply to the working directory or something?

I'm still piecing together Git's handling of branches in my mind as I have used Mercurial for so long.

Just thinking aloud to myself

Off-hand I don't know of a solution that could provide that same guarantee for cherry-picking. Our current process also requires that the engineer who authored the fix is the one responsible for merging it, rather than having release engineers or an unlucky engineer responsible for performing all merges into the subsequent versions.

This could probably be handled by having the server-side hook check for commits with the same task number, as we require all commits to start with a task number. It's not as "comfortable" as ensuring the same changeset exists in the branch but since the changesets require merge commits which can potentially introduce incorrect merges that comfort might be just as misleading.

The commits created as a side effect of arc land --hold aren't actually deleted explicitly, but they're effectively gone in Git as soon as you update your working copy to any other state. I'm not sure how closely this holds (or can hold) in Mercurial.

Ah yea this is one difference with Mercurial and Git's treatment of branches. In Mercurial there is no automatic cleanup, so things must be stripped/pruned manually before they will go away -- regardless of whether they're tagged with a bookmark or not. I'm still piecing together Git's handling of branches in my mind as I have used Mercurial for so long. Conceptually I know how I want to manipulate the repository but then things happen which reveal that how I'm thinking the repository is laid out isn't what Git's doing (Arcanist has been very useful in this respect).

Would the expectation then be that running arc land again would remove the rebased/squashed commits created by --hold, so the result would be as if they hadn't run --hold previously?

Remove TODO from ArcanistMercurialWorkingCopyRevisionHardpointQuery. Add some comments to functions in ArcanistLandEngine.

Oh just noticed your comment on this, marking for review so emails go out (I don't plan to make further changes here).

There are a bunch of libraries to help with this that I obviously won't be using because I didn't write them...

im a wizard

If I want to go spelunking later to see about improving/adding this behavior is this the right place to do this or is there another place this TODO should go?

Made updates from comments. Re-ran the "Landing a non-tip revision" test and everything still looks good.

I'll make these last few minor updates and do a quick few tests to make sure everything is still in working order

If there was a way to run the rebase --abort and discard the result of that it would probably be best.

I think not all versions of Windows + PHP require adjusting php.ini, but some nontrivial subset do -- enough that arc itself emits some flavor of guidance on this:

This is no longer my fight.

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.

Sorry for the churn on this, it sorta became a whackamole for an assortment of issues I ran into. I think most of the test cases I've tried out now indicate that the behavior is pretty solid at this point. For now I don't have any remaining test cases to cover and I think this is in a state for review.

Uncovered an issue when using the evolve extension, in which landing a non-tip revision would leave behind a set of commits in a hideous state.

Don't actually unset the found rebased active commit. Not sure why I thought that was a good idea.

Just did another pass-through of all these changes to do some cleanup.

• Removed an old line of code I was testing out trying to handle multiple onto-markers when restoring the working state
• Updated some comments and formatting of marker ref queries
• Found some logic which didn't quite carry over from refactoring of newPushCommands() in handling of cleaning up markers to be deleted

Whoops - that last change results in the same original behavior. It turns out confirmCommits() reverses the order that selectCommits() creates.

To confirm the min/max change I reviewed the revset x..y documentation along with the expected ordering of commits in the ArcanistLandCommitSet, then renamed + added some comments to clarify this behavior.

I normally use arc:bookmark however I recently switched it to arc:this so it's also compatible with git. All of my tests I've run currently were using arc:this.

Went back through https://secure.phabricator.com/book/phabricator/article/arcanist_commit_ranges/ and realized I can specify a ruleset of arc:bookmark, arc:this to get the behavior I want in both mercurial and git. Huzzah!

I'm guessing the --base argument would affect what commits get snagged up during landing (or is that captured during diff phase only?)

Ah it looks like the --base flag isn't part of diff workflow

$ arc land test --base arc:bookmark
Usage Exception: Argument "--base" is unrecognized. Use "--" to indicate the end of flags.

I'm going to do a few more tests -- I'm guessing the --base argument would affect what commits get snagged up during landing (or is that captured during diff phase only?). I normally use arc:bookmark however I recently switched it to arc:this so it's also compatible with git. All of my tests I've run currently were using arc:this.

Add back in the rebase --abort for now

When landing from a working directory that isn't in the set of changes being landed, restore the working directory to that commit after the land.

Not-working state test

• I created a diff with an inactive bookmark test but working directory on an older commit not related to the one being landed
• I landed that diff
• I verified the ending resulting working state was on the older commit I was on prior to landing

Doing a basic test this behavior looks good

$ hg wip
@   19:823c7457b7ef cspeck (30 seconds ago) tip test             <- draft commit to be landed, working state when running `arc land`
|     test commit
| o   17:33c3ae4d1277 cspeck (8 minutes ago) master
|/      test commit
o   14:82892d507c28 cspeck (24 minutes ago)
|     test commit
~

Determine whether the working directory was rebased, and if so update to it during reconcileLocalState()

For now, this has been working fine as a simple CLI flow.

When using --template I think I was not properly escaping quotes for how they are eventually invoked when launching the process

To sum up what I think the issues I ran into last night were

1. I was not purging caches when switching versions of mercurial or when making changes to DiffusionMercurialBlameQuery
2. I had improperly inverted strpos(':', $line) instead of strpos($line, ':')
3. When using --template I think I was not properly escaping quotes for how they are eventually invoked when launching the process

There's one minor bug with annotations using mercurial 4.2 for file lines that contain : not being annotated but it works on 5.8.

This was because I had made modifications to the resolveBlameFuture() in my testing. I undid these changes and things are working again. I tested both mercurial 5.8 and 4.4, purging all caches in between and verifying that annotations and history appear correctly, including annotations of files with code lines including ':'.

Update the handling of template arguments to avoid using quotes around the template being used

I've been incorporating ./bin/cache purge --all into my test process and things are making a lot more sense now. I'll hopefully have some changes up here shortly. There's one minor bug with annotations using mercurial 4.2 for file lines that contain : not being annotated but it works on 5.8.

Ah, yes; DiffusionBlameQuery has a cache.

Is it possible the annotate results are cached somewhere and might be causing some of the inconsistency I’m seeing?

I'll take another look tomorrow. Somehow I managed to get the code to emit output and found the issue with strpos() but then after fixing that it now seems busted again for both versions of mercurial. No exceptions are happening but it's not actually building up the annotation data properly (which is just the ordered list of the commit hashes I think?)

Fix strpos()

Best guess is that it's maaaaaaybe because we pass HGPLAIN=1 in the environment (DiffusionMercurialCommandEngine) and your version of Mercurial has annotate set up as an extension? That seems like a big stretch, but wouldn't show up.

I don't believe annotate is setup as an extension. This is our global .hgrc

# Ansible managed

I added some logs and then removed them but functionality nothing else should have changed (except the --template quoting I mention) and now when running hg 4.4 it no longer errors, but it the blame doesn't list changesets (but does put color next to the lines which I think is just a default if nothing was found?)

is there an easy way to log the command that will run from the CommandFuture?

Buh there must be something going on in the environment that's causing issues. I logged out the command it's running and tried running it directly in the repo

[cspeck@repo-testing1 85]$ hg --version
Mercurial Distributed SCM (version 4.4.2)
(see https://mercurial-scm.org for more information)

Oh that error happens when resolveBlameFuture() returns null because there was a problem invoking the command... looking a little more into what issue is happening

Hmm this is the error I'm getting when using mercurial 4.4

2021/07/08 22:35:31 [error] 9693#9693: *1 FastCGI sent in stderr: "

I setup mercurial to run version 4.4, created a new repository, added some commits, and verified all the above behavior still works properly.

This isn't quite true -- annotations don't appear to be showing up. I'll take a quick look

More updates based on comments. Tested out history view and annotation view and things look good.

Fix a regex typo and add back in the trim() on the hash

%C ("raw, unescaped comment")

Found one issue, which seems to fix the improper graph structure but now it's rendering disjointly

Something isn't quite right here. Looking at the history of a repo, the graph rendering doesn't match what it was previously doing

😎

Run arc liberate for the new test file

forgot to submit my comments~

Do capability-check for using hg annotate --template vs --debug, set the required version to 2.4, address other comments

Adding a capability check for hg annotate --template

Ah okay, I'll do some other testing. I was also wondering if there would be cases in Phabricator where something is doing something like if (get_class($a) == 'PhabricatorEnv') {... which if so I'm guessing would cause failures.

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"