To fix this:

First, here's the actual command we want to capture errors from, near line 125 of DrydockLandRepositoryOperation.php:

$interface->execx(
  'git push origin -- %s:%s',
  'HEAD',
  $push_dst);

Currently, if that fails it just throws an exception, and we get a generic error at top level ("Operation encountered an error.").

There's already a mechanism for raising errors and showing them nicely in the UI, which you can find an example of in DrydockWorkingCopyBlueprintImplementation.php near line 448:

try {
  $interface->execx('%C', $real_command);
} catch (CommandException $ex) {
  $this->setWorkingCopyVCSErrorFromCommandException(
    $lease,
    self::PHASE_SQUASHMERGE,
    $show_command,
    $ex);

  throw $ex;
}

So we just need to do something similar at the git push callsite.

The problem is that setWorkingCopyVCSErrorFromCommandException() is a protected method on DrydockWorkingCopyBlueprintImplementation, and DrydockRepositoryOperationType can't call it. We also don't have a $lease to pass.

However, we don't actually need this stuff. This is what happens with the merge error:

• The Blueprint catches it and sets it on the Lease.
• Later, it's copied to the RepositoryOperation.
• Then, the UI reads it from the RepositoryOperation.

The copy step happens in DrydockRepositoryOperationUpdateWorker.php, near line 141:

$vcs_error = $working_copy->getWorkingCopyVCSError($lease);
if ($vcs_error) {
  $operation
    ->setWorkingCopyVCSError($vcs_error)
    ->save();
}

So we can just write the error directly to the RepositoryOperation and skip the lease. I think something like this would work:

+ try {
    $interface->execx(
      'git push origin -- %s:%s',
      'HEAD',
      $push_dst);
+ } catch (CommandException $ex) {
+   $this->setWorkingCopyVCSError(...);
+   throw $ex;
+ }

The ... should be a dictionary like the one built in DrydockWorkingCopyBlueprintImplementation->setWorkingCopyVCSErrorFromCommandException(). It would be good to not duplicate that code. There are also sort of muddled responsibilities here where both WorkingCopyBlueprint and RepositoryOperation are somewhat responsible for handling VCS errors.

I think maybe we should just move this code to a separate class, since neither the Blueprint nor the RepositoryOperation really own it. So the implementation might look something like this:

• Introduce a new DrydockCommandError class. We'll use this to prevent code duplication between RepositoryOperation and WorkingCopyBlueprint.
• Move the logic in DrydockWorkingCopyBlueprintImplementation->setWorkingCopyVCSErrorFromCommandException() to DrydockCommandError::newFromCommandException(), so we'll be able to call it from elsewhere later on.
• Give the class a toDictionary() method, and have DrydockWorkingCopyBlueprintImplementation build a new error, then save it as a dictionary on the lease (we can't put objects in properties, since they get saved to the database).
• Right now, everything should still work normally -- you've only moved some code out so we can call it from elsewhere more easily.
• Add a new try/catch around the git push command, so we can capture any errors.
• Have that do something similar when it catches an exception, except set the error on the Operation. This should now give you somewhat better errors in the UI.
• Possibly rename all the WorkingCopyVCSError methods to just CommandError methods or similar, since this is no longer purely a working-copy-specific class of error transport. This is just optional cleanup to make naming a little more consistent.
• Then you might need to tweak DrydockRepositoryOperationStatusView to render these a little more cleanly (for example, the "merge" phase has some extra text talking about merges, and maybe this will be clearer if it has some extra text talking about pushing).

Hopefully that roughly makes sense? This is a little tricky because it's a small change but affects a lot of different pieces.

+ $this->setWorkingCopyVCSError(...);

Oh, this is probably actually $operation->setWorkingCopyVCSError(...);

It looks like "git push" in DrydockLandRepositoryOperation.php doesn't get executed.
I put some var_dump as the following, and click "Land Revision", but no related log printed out.

+ try {
+ $interface->execx(
+ 'git push origin -- %s:%s',
+ 'HEAD',
+ $push_dst);
+ var_dump("++++++++++++++++");
+ } catch (CommandException $ex) {
+ var_dump("--------------");
+ var_dump($ex->getStdout());
+ var_dump($ex->$ex->getStderr());
+ var_dump("--------------");

Is there anything I missed here?

I think var_dump() will normally be hard to observe, since it will go to stdout but that isn't connected to anything by default so it will just vanish (we don't write stdout to the daemon log). My guess is that the code is running, you just aren't seeing the var_dump() output.

A couple of ways you can see the output are:

• Use phlog() instead of var_dump(), which will go to stderr and should end up in the daemon log. You can phlog(print_r($value, true)) to log a complex value.
• Stop the daemons, then execute tasks from the queue one at a time with bin/worker execute --id <...>. You can use /daemon/ to see which IDs are queued. This will run them in the foreground so you can see stdout. However, it's a bit of a pain because you have to execute several of them (to acquire leases, etc) before you get to the one you actually want.

It would be nice to write a bin/differential land Dxxx to make testing easier, but I think it's a bit complex. We have a PhabricatorWorker::setRunAllTasksInProcess(true); mode to force tasks to run in the foreground, but right now it doesn't properly handle tasks that yield, and the Drydock tasks yield.

So far, I've been able to use a mixture of phlog() and bin/worker execute to debug this stuff without too much of an issue, but it's definitely a bit of a pain. Getting yields to work in the foreground is just also a bit of a pain.

When I put var_dump() in other parts of the code, I can see the output from /var/log/apache/error.log, but just can't see any output for the ones I mentioned above.

Any way, let me try the ways you suggested.