Page MenuHomePhabricator

Refactor how Mercurial runs commands that require extensions
ClosedPublic

Authored by cspeckmim on Jul 20 2021, 4:36 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 15, 10:32 AM
Unknown Object (File)
Feb 6 2024, 6:46 AM
Unknown Object (File)
Feb 5 2024, 7:48 PM
Unknown Object (File)
Feb 5 2024, 7:36 PM
Unknown Object (File)
Feb 4 2024, 1:14 AM
Unknown Object (File)
Jan 9 2024, 7:49 AM
Unknown Object (File)
Jan 7 2024, 4:34 PM
Unknown Object (File)
Jan 7 2024, 4:42 AM
Subscribers

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

cspeckmim held this revision as a draft.
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.

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

Ugh fix comment too why not

Be consistent with formatting

Make the command argument interpolation different, possibly less questionable

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.

src/repository/api/ArcanistMercurialAPI.php
1043

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

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
35

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.

1053

Can this be "private"?

1082

The first param is a "@param string", and it has the "extension name" (not "command name") to enable, right?

1091

This parameter signature is really $extension_name, $pattern, ..., I think?

1091

Can this be "private"?

1112

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

...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
1053

Ah, yes, will update

1082

Oops yea, I forgot to update these after the last bit of cleanup.

1091

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.

1112

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

cspeckmim marked 5 inline comments as done.
  • 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.
src/repository/api/ArcanistMercurialAPI.php
1091

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.

1112

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.)

src/repository/api/ArcanistMercurialAPI.php
1091

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)

src/repository/api/ArcanistMercurialAPI.php
1091

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.

1112

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.

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.