Page MenuHomePhabricator
Create Task
Maniphest T6050

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

Description

@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.

Related Objects

Event Timeline

epriestley created this task.Sep 8 2014, 7:02 PM
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.
sophiebits added a subscriber: sophiebits.Sep 30 2014, 11:30 PM
ezyang added a subscriber: ezyang.Aug 5 2015, 7:59 PM
Herald added a subscriber: eadler. · View Herald TranscriptAug 5 2015, 7:59 PM
ezyang added a comment.Aug 5 2015, 8:02 PM

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
tMessage.php:55]

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 moved this task from Backlog to v3 (Infrastructure) on the Differential board.Dec 29 2016, 4:33 PM
chad merged a task: T12053: Arcanist tries to parse phpunit output in the test plan as another test plan field.Dec 31 2016, 4:30 AM
chad added subscribers: dereckson, chad.
epriestley mentioned this in D17122: Always parse the first line of a commit message as a title.Jan 1 2017, 4:48 PM
epriestley closed this task as Resolved.Jan 1 2017, 4:54 PM
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]
Phabricator · Built by Phacility