Another possible idea: we can approve this change (again, premising that isset($message) and $message !== null are equivalent - but I don't have an opinion - so OK to keep isset($message))

And, would it be nice to also add something like this, to clarify a bit the internal expectations?

if ($is_start && $message === '') {
  throw new PhutilArgumentUsageException(pht("The start message cannot be empty"));
}

As already said the legacy situation deserved an additional check to me to prevent confusing situations of --stop --start "" wrongly recognized as stopped.

Hi @avivey what do you think about this legacy possible situation?

$ ./bin/repository maintenance --stop --start "" R9
Took repository "R9" out of maintenance mode.

Maybe this Diff is a good moment to introduce a fix also to that, with a dedicated message

In D25296#9658, @valerio.bozzolan wrote:

Hi @avivey what do you think about this legacy possible situation?

$ ./bin/repository maintenance --stop --start "" R9
Took repository "R9" out of maintenance mode.

Maybe this Diff is a good moment to introduce a fix also to that, with a dedicated message

To prevent --start and --stop from being used together, you should be able to add conflicts to one of the argument definitions. Could be named something else, I don't remember exactly. Search for it in other flows (including arcanist flows).

In D25296#9660, @avivey wrote:

To prevent --start and --stop from being used together, you should be able to add conflicts to one of the argument definitions. Could be named something else, I don't remember exactly. Search for it in other flows (including arcanist flows).

Interesting. See line 55. The check was already there, but not fired for that case since the old $is_start was wrong. So just adopting $is_start = $message !== null; should fix.

Changing to the (bool)strlen() to phutil_nonempty_string() is the simplest fix...