revi-archive/phorge-arcanist
1
0
Fork 0
mirror of https://we.phorge.it/source/arcanist.git synced 2024-11-29 10:12:41 +01:00
Code Issues Releases Wiki Activity

Don't consider a no-op amend to be an error in Mercurial

Browse source 
Summary:
Unlike git, Mercurial considers an `hg commit --amend` which doesn't change the working copy to be an error.

The current behavior for this is fairly bad, since the user gets an exception wrapping the entire command ("Command Failed! ...") and it's pretty verbose and not obvious what has happened.

Some alternatives are:

  1. Detect this condition and raise a more tailored exception, like a UsageException.
  2. Detect this condition and succeed.

Although I tend to think (1) is the right approach in general (that is, `arc x` should usually behave like `git x` or `hg x`), I went with (2) here because we have a handful of amend callsites and they all assume git semantics (no-op amends are successful), and because I think Mercurial's behavior is a little silly (the working copy ends up in the correct / expected state, which seems fairly clearly like a success to me).

Test Plan:
  - Had reporting user verify patch.
  - Ran `arc amend --revision Dxxx` twice in a Mercurial working copy.

The old output looked like this:

  $ arc amend --revision 922
  Amending commit message to reflect revision D922: Improve performance of PhutilSymbolLoader::selectAndLoadSymbols().
  Exception
  Command failed with error #1!
  COMMAND
  HGPLAIN=1 hg commit --amend -l '/private/var/folders/8k/c3vkmjy5335gcxdzxkhwq82w0000gn/T/71k8q057844c4g84/3539-gfkvV4'

  STDOUT
  nothing changed

  STDERR
  (empty)
  (Run with --trace for a full exception trace.)

The new output looks like this:

  $ arc amend --revision 922
  Amending commit message to reflect revision D922: Improve performance of PhutilSymbolLoader::selectAndLoadSymbols().
  Done.

Reviewers: btrahan, richardvanvelzen

Reviewed By: richardvanvelzen

CC: aran

Differential Revision: https://secure.phabricator.com/D8083
This commit is contained in:
epriestley 2014-01-28 03:03:30 -08:00
parent 7116659289
commit d1a68ccb37
1 changed files with 17 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

20
src/repository/api/ArcanistMercurialAPI.php
View file

@ -750,9 +750,23 @@ final class ArcanistMercurialAPI extends ArcanistRepositoryAPI {
$tmp_file = new TempFile(); $tmp_file = new TempFile();
Filesystem::writeFile($tmp_file, $message); Filesystem::writeFile($tmp_file, $message);
$this->execxLocal(
'commit --amend -l %s', try {
$tmp_file); $this->execxLocal(
'commit --amend -l %s',
$tmp_file);
} catch (CommandException $ex) {
if (preg_match('/nothing changed/', $ex->getStdOut())) {
// NOTE: Mercurial considers it an error to make a no-op amend. Although
// we generally defer to the underlying VCS to dictate behavior, this
// one seems a little goofy, and we use amend as part of various
// workflows under the assumption that no-op amends are fine. If this
// amend failed because it's a no-op, just continue.
} else {
throw $ex;
}
}
$this->reloadWorkingCopy(); $this->reloadWorkingCopy();
} }