1448137 - 'arc diff' fails with "Unknown Mercurial log field 'obsolete'!"

Status: RESOLVED FIXED

(Conduit :: Phabricator, defect)

Description

[Māris Fogels [:mars] (please needinfo)]

I created a bookmark containing obsolete/hidden changesets in the version-control-tools repo using 'hg rebase' and 'hg histedit'. I have the evolve extension enabled. 'arc diff' crashes if I run it from that bookmark:

$ arc diff
 Exception 
Unknown Mercurial log field 'obsolete'!
(Run with `--trace` for a full exception trace.)


Here is the output of 'arc diff --trace':

$ arc diff --trace
 ARGV  '/home/mars/usr/arcanist/arcanist/bin/../scripts/arcanist.php' 'diff' '--trace'
 LOAD  Loaded "phutil" from "/home/mars/usr/arcanist/libphutil/src".
 LOAD  Loaded "arcanist" from "/home/mars/usr/arcanist/arcanist/src".
Config: Reading user configuration file "/home/mars/.arcrc"...
Config: Did not find system configuration at "/etc/arcconfig".
Working Copy: Unable to find .arcconfig in any of these locations: /home/mars/work/version-control-tools/.arcconfig.
Working Copy: Path "/home/mars/work/version-control-tools" is part of `hg` working copy "/home/mars/work/version-control-tools".
Working Copy: Project root is at "/home/mars/work/version-control-tools".
Config: Reading local configuration file "/home/mars/work/version-control-tools/.hg/arc/config"...
>>> [0] <http> https://phabricator-dev.allizom.org/api/user.whoami
<<< [0] <http> 442,955 us
>>> [1] <exec> $ HGPLAIN=1 hg status
<<< [1] <exec> 135,268 us
>>> [2] <event> diff.didCollectChanges <listeners = 0>
<<< [2] <event> 45 us
>>> [3] <exec> $ HGPLAIN=1 hg help phase
<<< [3] <exec> 87,424 us
>>> [4] <exec> $ HGPLAIN=1 hg branch
<<< [4] <exec> 87,924 us
>>> [5] <exec> $ HGPLAIN=1 hg log --branch 'default' -r 'draft()' --style default
<<< [5] <exec> 111,374 us

[2018-03-22 18:37:45] EXCEPTION: (Exception) Unknown Mercurial log field 'obsolete'! at [<arcanist>/src/repository/parser/ArcanistMercurialParser.php:178]
arcanist(head=master, ref.master=dcd7ef66d0e4), phutil(head=master, ref.master=1ad42491e44a)
  #0 ArcanistMercurialParser::parseMercurialLog(string) called at [<arcanist>/src/repository/api/ArcanistMercurialAPI.php:169]
  #1 ArcanistMercurialAPI::buildBaseCommit(NULL) called at [<arcanist>/src/repository/api/ArcanistRepositoryAPI.php:599]
  #2 ArcanistRepositoryAPI::getBaseCommit() called at [<arcanist>/src/repository/api/ArcanistMercurialAPI.php:619]
  #3 ArcanistMercurialAPI::getCommitMessageLog() called at [<arcanist>/src/repository/api/ArcanistMercurialAPI.php:641]
  #4 ArcanistMercurialAPI::loadWorkingCopyDifferentialRevisions(ConduitClient, array) called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:1493]
  #5 ArcanistDiffWorkflow::buildCommitMessage() called at [<arcanist>/src/workflow/ArcanistDiffWorkflow.php:469]
  #6 ArcanistDiffWorkflow::run() called at [<arcanist>/scripts/arcanist.php:394]


My Mercurial installation info:

$ hg version
Mercurial Distributed SCM (version 4.4.1)

$ hg help extensions
...
    enabled extensions:

     evolve        extends Mercurial feature related to Changeset Evolution
     firefoxtree   work with Firefox source repositories more easily.
     histedit      interactive history editing
     pager         browse command output with an external pager (DEPRECATED)
     purge         command to delete untracked files from the working
                   directory
     rebase        command to move sets of revisions to a different ancestor
     shelve        save and restore changes to the working directory
     strip         strip changesets and their descendants from history
...

Comment 1

[Māris Fogels [:mars] (please needinfo)]

Arcanist was running from a fresh install:

$ arc version
arcanist dcd7ef66d0e419db4d97f1ebb624ec3c55e1fe4e (3 Mar 2018)
libphutil 1ad42491e44a1866975b366ae552f1d47761e35b (15 Mar 2018)

Comment 2

[Māris Fogels [:mars] (please needinfo)]

When I run "HGPLAIN=1 hg log --branch 'default' -r 'draft()' --style default" I get log output like the following:

changeset:   4858:d1a4348be46f
bookmark:    mozreview flags tests
user:        Māris Fogels <[e-mail]>
date:        Fri Nov 04 17:53:55 2016 -0400
obsolete:    rewritten as 4880:be2ef3870b95
summary:     WIP mozreview flags tests

Looking in src/repository/parser/ArcanistMercurialParser.php there is a switch statement that loops over the log lines, processing each.  The line marked 'obsolete' triggers the default case, raising an error.

Comment 3

[Māris Fogels [:mars] (please needinfo)]

I'm trying to figure out if the fix is needed in arcanist, hg, or both.

When I run "HGPLAIN=1 hg log -l5 --style default" on a branch that I know contains obsolete revisions, I do not see any.

When I use the --hidden flag and run "HGPLAIN=1 hg --hidden log -l5 --style default" on the branch with obsolete revisions I can see the obsolete revisions.

When I run "HGPLAIN=1 hg log --branch 'default' -r 'draft()' --style default" it shows obsolete revisions even though I did not use the --hidden flag.

Maybe the draft() revision specifier is the problem.  arcanist could be assuming it should never see an obsolete revision.  Is draft() supposed to return obsolete changesets even if the caller didn't use the --hidden flag?

gps, do you have any insight here?

Comment 4

[Gregory Szorc [:gps]]

There's a lot to unpack here.

But at the crux of it is probably whether HGPLAIN=1 should suppress the printing of "obsolete: " lines in `hg log`. I'm on the fence about that. Augie?

But that horse has left the barn: shipping versions of Mercurial can print "obsolete: " lines. So even if Mercurial changes its behavior to suppress that line, Arcanist will need to deal with old versions of Mercurial that print it. So Arcanist needs to change to not barf when encountering "obsolete: " lines.

I don't think there is a bug around displaying obsoleted changesets. Mercurial will show a hidden changeset without --hidden if the changeset is still relevant. It could be relevant by having a bookmark attached, being the parent of a non-obsolete changeset, etc.

For the record, `arc` running so many `hg` commands is really sub-optimal. On the Firefox repo, I expect the redundancy between command invocations to contribute hundreds of milliseconds of overhead. This overhead could be avoided by using Mercurial's command server or by writing an extension that does all the data gathering as one operation. Yet more justification in support of ditching Arcanist (bug 1366401).

Comment 5

[Augie Fackler]

I remember we had this discussion at a sprint (long enough ago that mpm was involved). We decided that the variety of parser that we should be most paranoid about is grep(1) in shell scripts, and so *adding* lines of output would be pretty low risk. Further, there's existing case-law for adding lines to log's default output with bookmarks. I think we should document in hg that parsers should either use the json format *or* be resilient to new output lines in the default log format they don't understand (and safely ignore them.)

Agreed that the best path is replacing arcanist, probably with a native hg extension.

Comment 6

[Māris Fogels [:mars] (please needinfo)]

A patch is in Arcanist upstream. See https://secure.phabricator.com/rARCb8c9c385a7f525d5f8c90620b805637de2de8e11

Comment 7

[Mark Côté [:mcote]]

I threw this on our story list, though it looks like it's being handled entirely by upstream (yay!).