Page MenuHomePhabricator

Improve error message / UX when fields are specified twice in revision information
Closed, ResolvedPublic


@zeeg reports a user putting "Reviewers: Let me tell you a story about reviewers ..." in a summary (presumably via web UI) and then receiving an unclear error when updating with arc diff:

[2014-09-08 08:59:36] EXCEPTION: (ArcanistDifferentialCommitMessageParserException) Field "reviewerPHIDs" occurs twice in commit message! at [/home/.../.arc_install/arcanist/src/differential/ArcanistDifferentialCommitMessage.php:79]

This is because there are now two "Reviewers" fields. We should possibly try to make the parser figure this out, or at least improve the clarity of the error.

Event Timeline

epriestley raised the priority of this task from to Normal.
epriestley updated the task description. (Show Details)
epriestley added a project: Differential.
epriestley added subscribers: epriestley, zeeg.

I've run into this problem by copy pasting my commit message into the description field so that I can update the description when my commit updates. If I also copy in the metadata fields, this breaks the parser, e.g.

[2015-08-05 19:44:11] EXCEPTION: (ArcanistDifferentialCommitMessageParserException) Field "test
Plan" occurs twice in commit message! at [<arcanist>/src/differential/ArcanistDifferentialCommi

This is extremely confusing because the failure to parse the commit message has nothing to do with the local commit message (which is fine); it's the remote message that Arcanist retrieved and is now attempting to parse. (It's unclear to the user why Phabricator should even care about such a message.)

Isn't the right answer to make the web interface reject things that would choke the parser?

epriestley claimed this task.

Isn't the right answer to make the web interface reject things that would choke the parser?

After D16846, the web interface rejects ambiguous summaries and test plans.

Note that arc diff --verbatim / arc diff --edit -- or updating fields by editing them via the web UI, rather than the CLI, and letting arc propagate the change itself -- are likely easier than editing from the CLI and then copy-pasting into the web UI.

  • After D17122, we will raise this error with the human-readable field name (like "Test Plan") instead of the internal field key (like "testPlan").
  • T12053 discusses an expectation that fields are parsed as remarkup and fields inside ``` blocks are ignored. Commit messages are not parsed as remarkup, and I do not plan to parse them as remarkup. See T11085 for some additional discussion.

At some point, the CLI for this also got cleaned up and now shows a more human-readable error:

$ arc diff --create
Commit message has errors:

      - Field "Reviewers" occurs twice in commit message!

You must resolve these errors to continue.

    Do you want to edit the message? [Y/n]