This is a little misleading because the user may not have specified the symbolic head, but the message will say they did.

I think it would be better to default the symbolic head to null, have getHeadCommit() deal with resolution, and then specialize the error messages to be explicit about what was specified.

Minor nits:

• Don't put text on the /** line; start on the next line.
• For consistency, use full sentences with periods and such, both in the main documentation and in the parameter/return documentation.
• The use of "HEAD^" is a bit confusing to me, consider something like: (like "HEAD^")
• Don't specify variable names, just types (@param string The symbolic commit to resolve.). This is unique to this codebase, but the parameter names are completely redundant with the method signature and I think they're not very useful so we don't use them.

$commit_hash or similar might be a better name for this variable than $merge_base?

I don't think we let you get here with a dirty working copy, but this will change the behavior if we do.

Instead of --range, maybe this should be something like --head, --tip, --to, or similar? Particularly:

• Calls like arc diff X --range Y..Z are confusing/ambiguous.
• Calls like arc diff --base X --range Y..Z are confusing/ambiguous.
• a..b is a valid branch name in Mercurial.
• Generally, there's no way to say "use my base rule normally, just stop at HEAD^" without a lot of extra work. This seems somewhat useful to support.

On the other side of things, it seems easy to run arc diff X --head Y instead of arc diff --range X..Y, so we aren't losing anything by doing this.

This should use full sentences and explicitly state that features will be disabled.

(Probably mooted by the above, but this could use some polish if it sticks around. For example, --range quack will explode right now, I think.)

This documentation should probably be explicit about what happens when $parent and $child are the same. Notably, in Mercurial, a commit is a descendant of itself.

Can this be private?

You can just trim(), there won't be any \2 in the output in this case.

When an error is a user's fault, throw ArcanistUsageException instead of Exception.

Generally, strings should be full sentences: use uppercase initial letters and terminal punctuation.

This message is flipped -- "base commit {$head_commit}" should be "base commit {$base_commit}".

Since this is in a final/leaf class and Git supports commit ranges, you can omit this check.

(null should be the default-by-default, and can be omitted?)

"ranged" -> "Ranges"

For consistency, prefer {$x} over ${x}.

This should use the specified HEAD.

Sleeping on it, I realize we do actually diff against the dirty working copy in one case: if you run arc lint, we'll call ArcanistBaseWorkflow::getChangedLines() and end up here, expecting to use the full diff (including dirty changes in the working copy) to figure out which lines have been changed by the user, so we can filter/adjust messages (e.g., not show warnings on unchanged lines).

This will alter the behavior, causing us to ignore dirty changes in the working copy.

I think ArcanistGitAPI::getAllLocalChanges() can be tweaked to pass a flag to getFullGitDiff(), which can then do a one-argument diff (i.e., no end of the range).

This is a little messy, but as far as I know there's no Y we can write in X..Y to mean "changes since X, including dirty changes in the working copy", so I think this ultimately needs to have two flavors of invocation anyway. This is also easy to clean up later if it turns out to not have made much sense.