Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T13659: `arc land` may fail with missing rebase extension

Commits

rARCac365c3ee509: Refactor how Mercurial runs commands that require extensions

Summary

Currently there are a number of --config extensions.blah= sprinkled throughout the Mercurial APIs which aren't pleasant to look at. Additionally it turns out the rebase command requires the rebase extension which is not turned on by default. Uses of rebase should also be including this flag to enable the extension when it's in use.

This adds ArcanistMercurialAPI::buildMercurialExtensionCommand() which allows running a Mercurial command that requires an extension just by naming the extension instead of providing the full --config.. argument.

It can be used e.g.

$api->execxLocalWithExtension(
  'strip',
  'strip --rev %s --',
  $rev);

$api->execxLocalWithExtension(
  'arc-hg',
  'arc-amend --logfile %s',
  $tmp_file);

Which produces

$ hg --encoding utf-8 --config 'extensions.strip=' strip --rev 82567759e2d703e1e0497f5f41363de3a1500188 --

$ hg --encoding utf-8 --config 'extensions.arg-hg=/Users/cspeckrun/Source/phacility/arcanist/support/hg/arc-hg.py' arc-amend --logfile /private/var/folders/cg/364w44254v5767ydf_x1tg_80000gn/T/6bwck66ccx0kwskw/89989-5F8JaL

Refs T13659

Test Plan

I ran through several scenarios of running arc diff and arc land with uncommitted changes on non-head commits, while not having the evolve extension enabled. Using the --trace argument I verified that rebase, strip, arc-amend, and arc-ls-markers were all invoked and the corresponding arc diff and arc land commands succeeded. I also ran arc land --onto-remote default while arc-hg.py raised an exception for the "remote" case and verified that the error was caught by Arcanist and displayed the error and prevented further execution. I removed the error from arc-hg.py and re-ran the command and verified it succeeded.

Diff Detail

Repository

rARC Arcanist

Branch

hgext

Lint

Lint Errors

Severity	Location	Code	Message
Error	src/repository/api/ArcanistMercurialAPI.php:1076	XHP86	Unsafe Usage of Dynamic String

Unit

Tests Passed

Build Status

Buildable 25483
Build 35232: Run Core Tests
Build 35231: arc lint + arc unit

Event Timeline

cspeckmim created this revision.Jul 20 2021, 4:36 AM

cspeckmim held this revision as a draft.

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptJul 20 2021, 4:36 AM

Herald added a subscriber: epriestley. · View Herald Transcript

Harbormaster completed remote builds in B25472: Diff 51669.Jul 20 2021, 4:36 AM

cspeckmim added inline comments.Jul 20 2021, 4:41 AM

src/repository/api/ArcanistMercurialAPI.php
39–40	Missed this in D21676

Make buildMercurialExtensionCommand() non-static, was experimenting while testing out reflection.
Also further simplify logic in ArcanistMercurialRepositoryMarkerQuery related to local vs. remote command.

Harbormaster completed remote builds in B25473: Diff 51670.Jul 20 2021, 4:43 AM

Fix the updated pass-thru command to include space after the encoding and the concatenated argument.

Harbormaster completed remote builds in B25474: Diff 51671.Jul 20 2021, 4:47 AM

Ugh fix comment too why not

Harbormaster completed remote builds in B25475: Diff 51672.Jul 20 2021, 4:48 AM

Be consistent with formatting

Harbormaster completed remote builds in B25483: Diff 51680.Jul 21 2021, 12:44 AM

Make the command argument interpolation different, possibly less questionable

Harbormaster completed remote builds in B25486: Diff 51683.Jul 21 2021, 1:12 AM

Okay I think this is ready for review. I'm not too certain I'm handling the command pattern population properly but I learned more about PHP reflection.

cspeckmim added inline comments.Jul 21 2021, 1:26 AM

src/repository/api/ArcanistMercurialAPI.php
1044	Agh I think I can simplify this, slight change incoming

Simplify some of the code by splitting out the bit which produces the flag to enable the extension and the bit that combines everything into an array of arguments to be passed into e.g. execxLocal().

Also fixed erroneous static reference to instance function

Harbormaster completed remote builds in B25487: Diff 51684.Jul 21 2021, 1:47 AM

On a non-head dirty commit I ran arc diff --trace and could confirm these ran properly

$ hg --encoding utf-8 --config 'extensions.arg-hg=/Users/cspeckrun/Source/phacility/arcanist/support/hg/arc-hg.py' arc-amend --logfile /private/var/folders/cg/364w44254v5767ydf_x1tg_80000gn/T/cglmx89uf3ks4gow/45330-yqYFDu

$ hg --encoding utf-8 --config 'extensions.rebase=' rebase --dest eedb14aad8e365e397f17b01d8c24c1064900ce8 --source c250351212afea2386969ba4500998108e0eb029 --source c6590e49efb7b10e609f83cb8ec52baa5ca9a7ec --source 0ae49ec540f00addc4b459f0a4d21fd7d1efaa24 --

$ hg --encoding utf-8 --config 'extensions.strip=' strip --rev 12d91b37ffc43f57380001327931465cc156f90b --

I was able to confirm the marker query with remote by running arc land --onto default --trace. This command doesn't print out when running with --trace but I added some echo to make sure that code path was executing and that it succeeded.

DOING THING: Array
(
    [0] => --config 'extensions.arg-hg=/Users/cspeckrun/Source/phacility/arcanist/support/hg/arc-hg.py' arc-ls-markers %Ls
    [1] => Array
        (
            [0] => --output
            [1] => /private/var/folders/cg/364w44254v5767ydf_x1tg_80000gn/T/5zj3k0mrl7s4c8cg/56041-kFATyL
            [2] => --
            [3] => default
        )

)

Thanks! Couple of minor inlines but I didn't catch anything substantial.

This command doesn't print out when running with --trace...

It's supposed to and seems to for me (at HEAD of master):

$ arc land --hold --trace
 ARGV  /Users/epriestley/dev/arcanist/bin/arc land --hold --trace
 PCNTL  Unable to install signal handler, pcntl_signal() unavailable. Continuing without signal handling.
>>> [0] (+0) <http> https://secure.phabricator.com/api/user.whoami
<<< [0] (+284) <http> 284,438 us
 STRATEGY  Merging with "squash" strategy, the default strategy.


>>> [1] (+286) <exec> $ hg --encoding utf-8 --config 'extensions.arc-hg=/Users/epriestley/dev/core/lib/arcanist/support/hg/arc-hg.py' arc-ls-markers --

^^^ That, right?

<<< [1] (+394) <exec> 107,172 us

...or did you mean something else?

src/repository/api/ArcanistMercurialAPI.php
34	`PhutilExecPassthru` and `ExecFuture` both extend `PhutilExecutableFuture`, which provides `setEnv()` and `setCWD()`, so this could be further simplified into: protected function buildLocalFuture(...) { return $this->newConfiguredFuture(new ExecFuture(...)); } protected function newPassthru(...) { return $this->newConfiguredFuture(new PhutilExecPassthru(...)); } private function newConfiguredFuture(PhutilExecutableFuture $future) { return $future ->setCWD(...) ->setEnv(...) } I can shoot you a diff for that, though.
1054	Can this be "private"?
1083	The first param is a "@param string", and it has the "extension name" (not "command name") to enable, right?
1092	This parameter signature is really `$extension_name, $pattern, ...`, I think?
1092	Can this be "private"?
1113	Passing `func_get_args()` directly may not work in the general case when the called function includes reference parameters: https://3v4l.org/ICX97 Phabricator doesn't[1] use reference parameters and this is always safe if you know the function you're calling doesn't use reference parameters, which you do in this case, but most/all of the `func_get_args()` elsewhere in the codebase does `$args = ...` to avoid hiccups here in the most general case. Of course, this is kind of silly since it's not really anything specific to `func_get_args()` (any other function has the same problem). I thought there was some specific `func_get_args()` issue beyond this, but I can't come up with a reproduction case that does anything unusual in any version of PHP, so maybe I imagined there was a more general problem here very early on when I was learning PHP and never disabused myself of that notion. So: this is safe unless I've double-tricked myself, and the `$args = func_get_args();`-even-when-you-don't-need-it pattern I've used in the rest of the codebase is almost certainly some pointless mumbo-jumbo I invented well more than a decade ago. [1] It uses them in one place I think, inside `xsprintf()`.

This revision is now accepted and ready to land.Jul 21 2021, 3:14 AM

Herald added a subscriber: Korvin. · View Herald TranscriptJul 21 2021, 3:14 AM

...or did you mean something else?

Specifically the arc-ls-markers which uses the --output argument, which appears to only be used when a remote marker is specified for landing. That uses $api->newPassthru(). When checking the output from arc land --onto-remote default it doesn't appear to log the execution of the passthru.

src/repository/api/ArcanistMercurialAPI.php
1054	Ah, yes, will update
1083	Oops yea, I forgot to update these after the last bit of cleanup.
1092	This parameter signature is really `$extension_name, $pattern, ...`, I think? Yea - in that case would I just use those variables directly but still use `array_slice(func_get_args(), 2)` to get the varargs after those two? Can this be "private"? This one is called by `ArcanistMercurialRepositoryMarkerQuery` to build up the arguments and invoke them with a passthru. I wasn't sure if that behavior (calls `passthru->execute()` was the same behavior as the `newPassthru()->resolve()` that the API uses. If so I can make a `executePassthruWithExtension()` variant and have it use that, then make this private.
1113	I'll update this. I don't quite catch on to the difference though. Does inlining the function call not implicitly store the return value in a temporary variable that it could then pass the reference to?

Oh I forgot to include that it does launch the regular local version of arc-ls-markers in my sample output, which may have provided more context for my comment about the one with --onto-remote default not appearing in the trace output

Updated the MercurialMarkerQuery to use a similar execPassthruWithExtension(). This results in printing the execution with --trace and better encapsulates the functions handling the extension-enabling details.
Made newConfiguredFuture() to combine the environment/cwd configuration for mercurial executions.
Updated to store result of func_get_args() into a variable first instead of passing directly into a function call.
Updated comments.

Harbormaster completed remote builds in B25490: Diff 51687.Jul 21 2021, 4:01 AM

epriestley added inline comments.Jul 21 2021, 4:04 AM

src/repository/api/ArcanistMercurialAPI.php
1092	You don't have to use the variables if you don't want to -- `func_get_args()` will still grab everything. Making the signature more precise only improves what the documentation generator can do. `$args[0]` or `$extension_name` will both work fine to access the value.
1113	Right: this is PHP, so inlining the function call just breaks rather than using a temporary variable as you might reasonably expect from a language which had gone through more of a "design" or "thinking about it" phase. (PHP is better these days, this behavior is a carryover from long long ago.)

cspeckmim added inline comments.Jul 21 2021, 4:05 AM

src/repository/api/ArcanistMercurialAPI.php
1092	I went ahead and created `executePassthruWithExtension()` which is a slightly different code path for the passthru (it now calls `passthru->resolve()` instead of `passthru->execute()` but I did test the error case with the hg extension blowing up and it does catch the error still (and also logs the command in `--trace`)

cspeckmim added inline comments.Jul 21 2021, 4:10 AM

src/repository/api/ArcanistMercurialAPI.php
1092	Gotcha - yea I think it's better clarity to name these. I wasn't sure if there's a better pattern to use but I went ahead and updated and it works better. I was initially confused reading some existing code as to what was functionally working vs. convention. I think I'm getting a better understanding of the reflection.
1113	I have used PHP previously back in ~2002 but I don't really remember much from back then and I'm not sure I understood it more at the time. Somehow I managed to write a forum board and music catalog system. I might have that code somewhere...

...the one with --onto-remote default not appearing in the trace output...

Ah. This appears to be a pre-existing bug with some code resolving "passthru" futures improperly by directly calling execute() instead of by calling resolve(). This is complicated; I believe execute() should not really be public, but is wrapped up in a bunch of history and may not be trivial to make private. I'll look into the particulars in a bit more detail and try to shoot you a diff to fix it.

cspeckmim edited the test plan for this revision. (Show Details)Jul 21 2021, 4:19 AM

Closed by commit rARCac365c3ee509: Refactor how Mercurial runs commands that require extensions (authored by cspeckmim). · Explain WhyJul 21 2021, 4:40 AM

This revision was automatically updated to reflect the committed changes.

cspeckmim added a commit: rARCac365c3ee509: Refactor how Mercurial runs commands that require extensions.

Do you use an editor like PHPStorm or WebStorm? So far I've been using SublimeText and ripgrep as the primarily tools.

I just use TextMate, which is a fairly lightweight editor. I have it minimally configured to disable a couple of default behaviors and add a couple aliases, and I have a crude "jump-to-definition" script set up that tries to open whatever file defines the class/function under the cursor and succeeds about 95% of the time.

On the CLI I have a couple of small scripts: get <fragment> prints all filenames containing that fragment or opens the file if only one matches (e.g., get <classname> usually opens the class definition file). And give accepts grep output and opens all matching files, e.g. git grep -i <symbol> | give makes it easy to open every file that uses a particular symbol.

I don't use autocomplete or any kind of editor auto-formatting or refactoring features. I don't use a debugger.

I've dabbled with IDEs in the past, although not for many years. I think that every time I've tried out an IDE I've encountered some kind of infuriating UI lag (e.g., slow to open files, or slow to switch between files, or weird typing lag, or saving files locks up the UI, or whatever else) in the first few minutes that feels way worse than any theoretical benefit the IDE could ever provide.

epriestley mentioned this in T13659: `arc land` may fail with missing rebase extension.Jul 21 2021, 4:44 PM

cspeckmim mentioned this in D21706: Update other usages of "hg rebase" to use the new extension-enabling function.Jul 21 2021, 8:59 PM

cspeckmim mentioned this in rARC76a2976fd9a4: Update other usages of "hg rebase" to use the new extension-enabling function.Jul 21 2021, 9:11 PM

Refactor how Mercurial runs commands that require extensions
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 51680

src/land/engine/ArcanistMercurialLandEngine.php

src/repository/api/ArcanistMercurialAPI.php

src/repository/marker/ArcanistMercurialRepositoryMarkerQuery.php

src/repository/state/ArcanistMercurialLocalState.php

Refactor how Mercurial runs commands that require extensionsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 51680

src/land/engine/ArcanistMercurialLandEngine.php

src/repository/api/ArcanistMercurialAPI.php

src/repository/marker/ArcanistMercurialRepositoryMarkerQuery.php

src/repository/state/ArcanistMercurialLocalState.php

Refactor how Mercurial runs commands that require extensions
ClosedPublic
Actions

Revision Contents
Changeset List