I expect these 3 calls will be abstracted to 3 "providers" or something, which the flow will have to pick and match based on repo type and features.

I only avoided extracting interfaces/classes due to strict adherence to YAGNI, and I plan my own "push provider" for not-hosted git repos.

Initial thoughts are to create /differential/landing dir which will hold all interfaces/implementations, and somehow config them in the repo control.

I just needed some place to make the link for this.

This is fine, we'll move it into Drydock eventually in some form.

Discussed a bit below, but since you don't need any of the stuff it offers, using PhabricatorRepository is more consistent. Broadly, these classes (PhabricatorRepository vs RepositoryAPI vs all the Diffusion queries) probably were not perfectly architected and should theoretically split their responsibilities out a little differently than they do, but the current split doesn't seem to cause any major issues.

Use $repo->newRemoteCommandFuture() so we pick up all the environmental hacks we need on some systems.

This is slightly more correct as clone -- %s than clone %s (that is, add -- to terminate flags), since $path could some day start with a "-".

Use PhabricatorGlobalLock (probably much easier) or PhutilFileLock (probably not a good fit). These locks have better release semantics, unit tests, etc.

See PhabricatorRepositoryPullLocalDaemon for an example of creating a GlobalLock on a repo. I'd just put a lock on the whole repo for now -- by the time we support mor than one simultaneous workspace, hopefully we'll be on Drydock.

Using $repo->execxLocalCommand() is probably slightly preferable here if you don't need the GitAPI elsewhere. In theory we should maybe share more code between Repository and the *API classes, but in practice they mostly tend to deal with dissimilar situations.

Although it doesn't impact anything for now, this locking should probably happen in the Strategy so we can release it at the right time.

Double-quote?

You can use isFormPost() to verify the request is a POST with CSRF. To make it be POST, you can use workflow on the action link. The non-post case should pop a confirmation dialog ("Really land this?") for users with broken JS or right click -> open in new window. Roughly:

if ($this->isFormPost()) {
  do_land();
  return success_dialog();
}

return confirm_dialog();

If you always want to show the confirm dialog, you can use isDialogFormPost() instead of isFormPost().

All this return array(...) stuff should probably just let the exception escape, or wrap it in a ProxyException and rethrow. None of it ever returns more than one string.

This file has a spurious +x.

This can probably just be a base class. I don't anticipate needing to add this interface to other things, and we'll probably want to put concrete code here (maybe for "is revision landable" or "does user have permission to push this repository", or "grab a lock" or whatever else).

You should be able to get a Future and then write(...) to it to skip this temp file stuff.

(We use temp files in arc in some cases, but that's just so the file is left over on failure so we can tell the user to go look at it. That seems less useful here.)

It would be slightly more correct to take the original diff's author/email if they're available (you'd have to fish around in Diff properties), and then fall back to something like this. Not a big deal.

Ideally, we should pass --date, too, although that's not very important.

We may need to --allow-empty and --allow-empty-message, although those seem fine to skip for now.

For the commiter, I think this is OK (taking the email address), although maybe we should let you designate a "VCS Email" in your profile. We can do that in a followup if we decide we care, though.

We'll probably have to move away from hard coding master at some point, but I don't have a strong sense of how configurable vs prompty vs automatic this should be.

Like we discussed on irc - it's kinda awkward right now to use PhabricatorRepository. ArcanistGitApi is mostly missing the "HOME=" env var (which was solved in git 1.8.3.1).

getLocalCommandFuture() seems to be a better match (Because we ignore the repo's own remote, and our remote here is file://.

For now I'd feel safer with having the confirmation dialog always show up, and maybe explain what it'd going to do (As in "This is going to squash this revision, rebase, and push it to master").

I'll remove this next iteration; it's not used any more.

I'd moved the lock into the "main" controller, to minimize the risk of a strategy doing something wrong with the locks. It's also simpler here.

I'm not very happy about this api being exposed here, in what's likely to become "reference implementation" for strategies; But separating it to another parameter is also not ideal.

Maybe make it a method of PhabricatorRepository? That would at least let us hide it and easily replace with drydock.

I actually couldn't find where this method is defined, but it seems to do the right thing.

The only reason for this method is to explain what's wrong; but this method will not be invoked under normal conditions if it's not supposed to work, so it should go away; Probably replace with doesSupportRepository() that just return a boolean. Or just assume that since this class is in charge on creating the link that enables it, than it will always be called under the right conditions.

It felt safer to not provide the $view or $event to the strategy, and limiting it to providing the menu items.

It's one of the magic property getters because $revision is a DAO as far as I know (all tables have a dateCreated and dateModified column).

The default handler doesn't special-case ProxyException, and it doesn't add the <pre>, which is important for git errors.

I can reduce some code here though.