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}".

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.

But the original message explicitly mentioned HEAD, which is the default here.

getHeadCommit() is problematic, since it resolves it to the commit hash, which is more confusing.

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

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.

Oh, that's true. Should I alter this so it will diff against the passed commit if --range isn't being used? Is it ever desirable to diff against working copy?

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.

Ah yes. Ok, let me take care of it.