1515328 - Make moz-phab less confusing than 'arc patch' if it fails

Status: RESOLVED FIXED

(Conduit :: moz-phab, defect)

Description

[Mark Banner (:standard8)]

I was working with someone yesterday who is fairly new to development and version control. They had reset their repository and then were trying to use `arc patch` to get back a two-commit series. The second patch was bitrotted, and so they were confused at the output and the state they were left in.

Here's an example output that I generated myself from the commits we were trying to fix:

```
$ arc patch --nobranch --diff 40936
 OKAY  Successfully committed patch.


    This diff is against commit a5c9fb92dd3505cd82458cdee997ca0eb62c0738, but
    the commit is nowhere in the working copy. Try to apply it against the
    current working copy state? (.) [Y/n] y


 Patch Failed! 
 Exception 
Command failed with error #255!
COMMAND
HGPLAIN=1 hg import --no-commit -

STDOUT
applying patch from stdin


STDERR
patching file .eslintignore
Hunk #1 FAILED at 189
1 out of 1 hunks FAILED -- saving rejects to file .eslintignore.rej
abort: patch failed to apply

(Run with `--trace` for a full exception trace.)
```

The problem with this, is that it basically says that it failed. Nowhere does it say "you've got a partially applied patch that you just have to fix the bitrot and re-commit" - and it doesn't clarify that one commit landed fine, but the second didn't.

It'd also be useful if it gave you the commit message or a command so that you can either simply fix and commit or reset your repository to what it was - similar in style to git rebase / hg histedit / cherry-pick.

Comment 1

[Mark Banner (:standard8)]

I just happened upon this with git and it is way better:

```
 Cherry Pick Failed!
 Exception 
Command failed with error #1!
COMMAND
git cherry-pick 'arcpatch-D14131'

STDOUT
(empty)

STDERR
warning: inexact rename detection was skipped due to too many files.
warning: you may want to set your merge.renamelimit variable to at least 1097 and retry the command.
error: could not apply 16d4fd2e30efe... Bug 1508988 - Enable ESLint for dom/abort/, dom/asmjscache/, dom/battery/, dom/broadcastchannel/ and dom/console/ (automatic changes). r?standard8!
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'
```

Comment 2

[Mark Côté [:mcote]]

We can try to improve this in moz-phab.  I'll mark this as blocking the "moz-phab patch" feature.

Comment 3

[Piotr Zalewa [:zalun]]

moz-phab will fail if no base commit is found before changing anything.
Then the user would need to use --force option to run patching.
The patching itself is using underlying VCS (Git or Mercurial).

A diff (creating file X):

$ moz-phab patch D80 --nocommit
diff --git a/X b/X
new file mode 100644
--- /dev/null
+++ b/X
@@ -0,0 +1 @@
+a

This diff applied when the file exists:

$ moz-phab patch D80 --force
Patching revision: ['D80']
error: X: already exists in working directory
CalledProcessError: Command '['git', '-c', 'user.email=[e-mail]', '-c', 'user.name=Conduit Test User', '-c', 'cinnabar.helper=/home/phab/git-cinnabar/git-cinnabar-helper', 'apply', '/tmp/tmpUK9oUe']' returned non-zero exit status 1

with DEBUG

$ DEBUG=1 moz-phab patch D80 --force
...
DEBUG    2019-01-08 10:27:11,126 $ git -c user.email=[e-mail] -c 'user.name=Conduit Test User' -c cinnabar.helper=/home/phab/git-cinnabar/git-cinnabar-helper apply /tmp/tmpT52BDq
error: X: already exists in working directory
ERROR    2019-01-08 10:27:11,135 Traceback (most recent call last):
  File "/home/phab/review/moz-phab", line 2828, in main
    args.func(repo, args)
  File "/home/phab/review/moz-phab", line 2628, in patch
    repo.apply_patch(raw, body)
  File "/home/phab/review/moz-phab", line 1534, in apply_patch
    self.git(["apply", patch_file])
  File "/home/phab/review/moz-phab", line 1215, in git
    check_call(self._git + command, cwd=self.path, env=self._env, **kwargs)
  File "/home/phab/review/moz-phab", line 201, in check_call
    subprocess.check_call(command, **kwargs)
  File "/usr/lib/python2.7/subprocess.py", line 186, in check_call
    raise CalledProcessError(retcode, cmd)
CalledProcessError: Command '['git', '-c', 'user.email=[e-mail]', '-c', 'user.name=Conduit Test User', '-c', 'cinnabar.helper=/home/phab/git-cinnabar/git-cinnabar-helper', 'apply', '/tmp/tmpT52BDq']' returned non-zero exit status 1

Comment 4

[Piotr Zalewa [:zalun]]

The idea is slightly changed:

If no base commit is found moz-phab returns an Error.
To make it pull down the patch a --nocommit switch has to be used.