1879129 - `moz-phab uplift` should be clearer that a failed uplift rebase means patches won't apply on landing

Status: RESOLVED FIXED

(Conduit :: moz-phab, defect)

Description

[Paul Adenot (:padenot)]

Attachment: logs.txt

moz-phab version (from moz-phab --version):

trees/mozilla-unified::7b6078b99$ moz-phab --version
MozPhab 1.5.1 (Python 3.11.6, Linux)
OS: Ubuntu 23.10 nothing fancy

Comment 1

[Paul Adenot (:padenot)]

Attachment: new_logs.txt

mozphab/mercurial.py around line 749 now reads, with --config in the end of the arg list:

        out = self.hg_out(
            [
                # Send messages to `stderr` so `stdout` is pure JSON.
                "rebase",
                "-r",
                self.revset,
                "-d",
                dest,
                # Don't obsolete the original changesets.
                "--keep",
                "-T",
                "json",
                "--config",
                "ui.message-output=stderr",
            ],
            split=False,
        )

Comment 2

[:glob ✱]

The second log is because of merge conflicts during a rebase:

merging third_party/libwebrtc/webrtc.gni
rebasing 784012:9ccbf82caacc moz-phab_9ccbf82caacc tip "Bug 1875201 - Add method for conditionally including media/libvpx/libvpx - moz.build file updates. r?ng"
merging CLOBBER
hit merge conflicts; rebasing that commit again in the working copy
merging CLOBBER
warning: conflicts while merging CLOBBER! (edit, then use 'hg resolve --mark')
merging third_party/libwebrtc/moz.build
unresolved conflicts (see 'hg resolve', then 'hg rebase --continue')

Comment 3

[:glob ✱]

Here's the relevant part of the log for the --config error:

DEBUG	2024-02-07 16:04:24,330 $ hg bookmark --delete moz-phab_1b599a2a1d23+ --pager never
ERROR	2024-02-07 16:04:24,412 Traceback (most recent call last):
  File "/home/padenot/.local/pipx/venvs/mozphab/lib/python3.11/site-packages/mozphab/mozphab.py", line 126, in main
	args.func(repo, args)
  File "/home/padenot/.local/pipx/venvs/mozphab/lib/python3.11/site-packages/mozphab/commands/uplift.py", line 78, in uplift
	submit(repo, args)
  File "/home/padenot/.local/pipx/venvs/mozphab/lib/python3.11/site-packages/mozphab/commands/submit.py", line 710, in submit
	raise e
  File "/home/padenot/.local/pipx/venvs/mozphab/lib/python3.11/site-packages/mozphab/commands/submit.py", line 701, in submit
	_submit(repo, args)
  File "/home/padenot/.local/pipx/venvs/mozphab/lib/python3.11/site-packages/mozphab/commands/submit.py", line 489, in _submit
	avoid_local_changes = local_uplift_if_possible(args, repo, commits)
                      	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/padenot/.local/pipx/venvs/mozphab/lib/python3.11/site-packages/mozphab/commands/submit.py", line 464, in local_uplift_if_possible
	commits = repo.uplift_commits(unified_head, commits)
          	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/padenot/.local/pipx/venvs/mozphab/lib/python3.11/site-packages/mozphab/mercurial.py", line 749, in uplift_commits
	out = self.hg_out(
      	^^^^^^^^^^^^
  File "/home/padenot/.local/pipx/venvs/mozphab/lib/python3.11/site-packages/mozphab/mercurial.py", line 210, in hg_out
	out = self.repository.rawcommand(command_bytes, eh=error_handler)
      	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/padenot/.local/pipx/venvs/mozphab/lib/python3.11/site-packages/hglib/client.py", line 265, in rawcommand
	return eh(ret, out, err)
       	^^^^^^^^^^^^^^^^^
  File "/home/padenot/.local/pipx/venvs/mozphab/lib/python3.11/site-packages/mozphab/mercurial.py", line 199, in error_handler
	raise CommandError(
mozphab.exceptions.CommandError: command '--config' failed to complete successfully

Looks like the problem is at in mercurial.py:

        out = self.hg_out(
            [
                # Send messages to `stderr` so `stdout` is pure JSON.
                "--config",
                "ui.message-output=stderr",
                "rebase",
                "-r",
                self.revset,
                "-d",
                dest,
                # Don't obsolete the original changesets.
                "--keep",
                "-T",
                "json",
            ],
            split=False,
        )

Which is weird, because --config should be valid anywhere in the command.
With mercurial 6.6.2 this works: hg --config ui.message-output=stderr status

Comment 4

[:glob ✱]

Ahh; the error handling in moz-phab assumes the first argument to hg_out is the command name:

            raise CommandError(
                "command '%s' failed to complete successfully"
                % command_bytes[0].decode(),
                exit_code,
            )

A correctly worded error would be mozphab.exceptions.CommandError: command 'rebase' failed to complete successfully.

Sure enough, earlier in the log we see:

DEBUG	2024-02-07 16:04:24,317 $ hg --config ui.message-output=stderr rebase -r 3724737c385d::9ccbf82caacc -d beta --keep -T json --pager never
DEBUG	2024-02-07 16:04:24,321 abort: rebase in progress
(use 'hg rebase --continue', 'hg rebase --abort', or 'hg rebase --stop')

Moving the --config switch would fix the confusing error message, which is likely the only problem here.

Comment 5

[Connor Sheehan [:sheehan]]

I think we ought to make it clearer that this error state means "We tried to rebase your patches onto the latest commits from <uplift train> but we couldn't. This means your patches will fail to apply without resolving merge conflicts. Please rebase your changes manually and submit them for uplift."

Paul and I noticed the issue with assuming the 0th element of the command arguments list is the command name, but I never added that context here. :)

Comment 6

[Connor Sheehan [:sheehan]]

Attachment: hg: move `--config` so `rebase` is the first command arg (Bug 1879129) r?glob,zeid

When hg_out fails to run a command, the message that is used for
the exception is the a "command" failed, where the command is the
0th element of the command arguments. Since we pass the --config
option before the rebase command in uplift_commits, this causes
the error message to indicate a failure of the --config command.

Move the --config flag to the end of the command, so the actual
rebase command ends up in the exception error message.

Comment 7

[Connor Sheehan [:sheehan]]

Attachment: vcs: indicate that a rebase failure requires manual merge conflict resolution (Bug 1879129) r?glob,zeid

Add extra error messaging when a rebase failure occurs, indicating that
the rebase must be completed manually for the patch to land.