Don't crash in phrevset if Conduit replies before linear search completes

Summary:
When converting a Diff number to a revset, phrevset would do a linear scan of the repo history for the most recent appearance of that Diff number (slow if it's an older commit or not present in this repo) and in parallel, query Phabricator Conduit to try and shortcut the process.

Unfortunately, if Conduit was quicker than the linear scan, we would crash because we can't handle a Conduit response for hg commits made when a Diff lands, or for any repo that doesn't have a callsign in hgrc.

Fix the crashes; don't bother calling Conduit if we can't handle any reasonable answer, and teach the hg branch to handle all reasonable answers.

Test Plan:
Check that the new extension does linear search unconditionally when it lacks a callsign:
```
: /data/users/simonfar/fbsource (hg) [90234606e65a2a04d042caba7cf5ee85a7a5466e]
: simonfar@devvm631  $ hg --config extensions.phrevset=/data/users/simonfar/fb-hgext/phrevset.py log -r D2750128
phrevset.callsign is not set - doing a linear search
changeset:   3c0e6756bed4d7330391719bcde52e7490499e53  D2750128
user:        Yuhan Guo <yhguo@fb.com>
date:        Thu, 07 Jan 2016 00:19:23 -0800
```
Check that it doesn't do a linear search in a repo with a callsign:
```
: /data/users/simonfar/www-hg (hg) [webacademy-graphql]
: simonfar@devvm631  $ echo -e '[phrevset]\ncallsign=E' >> .hg/hgrc

: /data/users/simonfar/www-hg (hg) [webacademy-graphql]
: simonfar@devvm631  $ hg --config extensions.phrevset=/data/users/simonfar/fb-hgext/phrevset.py log -r D2704940
changeset:   4b24ee6737b7e36523eb24c6406d689cc54aadf9  D2704940
user:        xifanyan@2c7ba8d8-a2f7-0310-a573-de162e16dcc7
date:        Mon, 30 Nov 2015 23:38:18 -0800
```
Check that it aborts cleanly if you give it a Diff from a different repo in a repo with a known callsign:
```
: /data/users/simonfar/www-hg (hg) [webacademy-graphql]
: simonfar@devvm631  $ hg --config extensions.phrevset=/data/users/simonfar/fb-hgext/phrevset.py log -r D2750128
abort: Diff callsign 'FBS' is different from repo callsign 'E'
```
Check that linear scan eventually aborts cleanly if you give it a Diff from a different repo (note: this step is slow - over a minute on my devvm):
```
: /data/users/simonfar/fbsource (hg) [90234606e65a2a04d042caba7cf5ee85a7a5466e]
: simonfar@devvm631  $ hg --config extensions.phrevset=/data/users/simonfar/fb-hgext/phrevset.py log -r D2704940
phrevset.callsign is not set - doing a linear search
abort: Could not find diff D2704940 in changelog
```

Reviewers: #sourcecontrol, rmcelroy, ttung, lcharignon

Reviewed By: lcharignon

Subscribers: lcharignon, kanishkparihar

Differential Revision: https://phabricator.fb.com/D2844832

Tasks: 9723813, 9714886

Signature: t1:2844832:1453321767:f445447d8187b4be22db3065ba316e6aa461f757
This commit is contained in:
Simon Farnsworth 2016-01-21 08:11:13 -08:00
parent e8fde2c226
commit f8b263fdd8

View File

@ -27,6 +27,7 @@ from mercurial import extensions
from mercurial import revset
from mercurial import error
from mercurial import util as hgutil
from mercurial.i18n import _
from hgsubversion import util as svnutil
@ -95,7 +96,7 @@ def finddiff(repo, diffid, proc=None):
def forksearch(repo, diffid):
"""Perform a log traversal and Conduit call in parallel
Returns a (revision, arc_response) tuple, where one of the items will be
Returns a (revisions, arc_response) tuple, where one of the items will be
None, depending on which process terminated first"""
repo.ui.debug('[diffrev] Starting Conduit call\n')
@ -127,11 +128,14 @@ def forksearch(repo, diffid):
resp = proc.stdout.read()
return (None, resp)
def parsedesc(repo, resp):
def parsedesc(repo, resp, ignoreparsefailure):
desc = resp['description']
match = DESCRIPTION_REGEX.match(desc)
if not match:
if ignoreparsefailure:
return None
else:
raise error.Abort("Cannot parse Conduit description '%s'"
% desc)
@ -147,11 +151,23 @@ def parsedesc(repo, resp):
def revsetdiff(repo, subset, diffid):
"""Return a set of revisions corresponding to a given Differential ID """
rev, resp = forksearch(repo, diffid)
repo_callsign = repo.ui.config('phrevset', 'callsign')
if repo_callsign is None:
msg = _('phrevset.callsign is not set - doing a linear search\n')
hint = _('This will be slow if the diff was not committed recently\n')
repo.ui.warn(msg)
repo.ui.warn(hint)
rev = finddiff(repo, diffid)
if rev is None:
raise error.Abort('Could not find diff D%s in changelog' % diffid)
else:
return [rev]
if rev is not None:
revs, resp = forksearch(repo, diffid)
if revs is not None:
# The log walk found the diff, nothing more to do
return rev
return revs
jsresp = json.loads(resp)
if not jsresp:
@ -159,8 +175,8 @@ def revsetdiff(repo, subset, diffid):
resp = jsresp.get('response')
if not resp:
error = jsresp.get('errorMessage', 'unknown error')
raise error.Abort('Counduit error: %s' % error)
e = jsresp.get('errorMessage', 'unknown error')
raise error.Abort('Counduit error: %s' % e)
vcs = resp.get('sourceControlSystem')
@ -170,14 +186,14 @@ def revsetdiff(repo, subset, diffid):
# commit has landed in svn, parse the description to get the SVN
# revision and delegate to hgsubversion for the rest
svnrev = parsedesc(repo, resp)
svnrev = parsedesc(repo, resp, ignoreparsefailure=False)
repo.ui.debug("[diffrev] SVN rev is r%s\n" % svnrev)
args = ('string', svnrev)
return svnutil.revset_svnrev(repo, subset, args)
elif vcs == 'git':
gitrev = parsedesc(repo, resp)
gitrev = parsedesc(repo, resp, ignoreparsefailure=False)
repo.ui.debug("[diffrev] GIT rev is %s\n" % gitrev)
peerpath = repo.ui.expandpath('default')
@ -192,7 +208,11 @@ def revsetdiff(repo, subset, diffid):
return [repo[remoterev].rev()]
elif vcs == 'hg':
# commit is still in hg, get its hash
rev = parsedesc(repo, resp, ignoreparsefailure=True)
if rev:
return [rev.encode('utf-8')]
# commit is still local, get its hash
props = resp['properties']
commits = props['local:commits']