( )⚙ D684 phrevset: fix code that was returning commit strings

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D684

phrevset: fix code that was returning commit strings
ClosedPublic

Authored by simpkins on Sep 11 2017, 6:16 PM.
Tags
None
Subscribers
None

Details

Reviewers
quark
durham
Group Reviewers
Restricted Project
Commits
rFBHGX420517cb58ca: phrevset: fix code that was returning commit strings
Summary

Fix another code path in phrevset that could incorrectly return commit strings
instead of rev numbers.

Test Plan

Manually tweaked phrevset to force it to hit this code path (to ensure it would
not find the commit from the local log walk first), and confirmed it now
returned a rev ID instead of commit strings.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Branch
default
Lint
Lint OK
Unit
Unit Tests OK

Event Timeline

simpkins created this revision.Sep 11 2017, 6:16 PM
quark accepted this revision.Sep 11 2017, 7:03 PM
quark added inline comments.
hgext3rd/phrevset.py
96

It seems we can raise directly here to simplify the code. But that could be a separate diff.

243

Maybe use mercurial.encoding.unitolocal

249–250

I think it makes more sense to use filtered repo here. An obsoleted commit may or may not have successors and there could be a split. If there is a successor, the successor would be visible and would be searched.

252

rev is already encoded in line 243?

267

nit: results is already a set

This revision is now accepted and ready to land.Sep 11 2017, 7:03 PM
Closed by commit rFBHGX420517cb58ca: phrevset: fix code that was returning commit strings (authored by simpkins). · Explain WhyOct 16 2017, 6:26 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext3rd/phrevset.py (17 lines)

Diff 1732

hgext3rd/phrevset.py

changectx = repo[rev] changectx = repo[rev]
desc = changectx.description() desc = changectx.description()
match = DIFFERENTIAL_REGEX.search(desc) match = DIFFERENTIAL_REGEX.search(desc)
if match and match.group('id') == diffid: if match and match.group('id') == diffid:
return changectx.rev() return changectx.rev()
return None return None
quarkUnsubmitted
Not Done

It seems we can raise directly here to simplify the code. But that could be a separate diff.

quark: It seems we can `raise` directly here to simplify the code. But that could be a separate diff.
def forksearch(repo, diffid): def forksearch(repo, diffid):
"""Perform a log traversal and Conduit call in parallel """Perform a log traversal and Conduit call in parallel
Returns a (revisions, 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""" None, depending on which process terminated first"""
repo.ui.debug('[diffrev] Starting Conduit call\n') repo.ui.debug('[diffrev] Starting Conduit call\n')
return [node.rev()] return [node.rev()]
# commit is still local, get its hash # commit is still local, get its hash
props = resp['properties'] props = resp['properties']
commits = props['local:commits'] commits = props['local:commits']
# the JSON parser returns Unicode strings, convert to `str` in UTF-8 # the JSON parser returns Unicode strings, convert to `str` in UTF-8
revs = [c['commit'].encode('utf-8') for c in commits.values()] revs = [c['commit'].encode('utf-8') for c in commits.values()]
quarkUnsubmitted
Not Done

Maybe use mercurial.encoding.unitolocal

quark: Maybe use `mercurial.encoding.unitolocal`
# verify all revisions exist in the current repo; if not, try to # verify all revisions exist in the current repo; if not, try to
# find their counterpart by parsing the log # find their counterpart by parsing the log
for idx, rev in enumerate(revs): results = set()
if rev not in repo: for rev in revs:
# TODO: This really should be searching in repo.unfiltered(),
# and then resolving successors if the commit was hidden.
quarkUnsubmitted
Not Done

I think it makes more sense to use filtered repo here. An obsoleted commit may or may not have successors and there could be a split. If there is a successor, the successor would be visible and would be searched.

quark: I think it makes more sense to use filtered repo here. An obsoleted commit may or may not have…
try:
node = repo[rev.encode('utf-8')]
quarkUnsubmitted
Not Done

rev is already encoded in line 243?

quark: `rev` is already encoded in line 243?
results.add(node.rev())
except error.RepoLookupError:
repo.ui.warn(_('Commit not found - doing a linear search\n')) repo.ui.warn(_('Commit not found - doing a linear search\n'))
parsed_rev = finddiff(repo, diffid) parsed_rev = finddiff(repo, diffid)
if not parsed_rev: if not parsed_rev:
raise error.Abort('Could not find diff ' raise error.Abort('Could not find diff '
'D%s in changelog' % diffid) 'D%s in changelog' % diffid)
revs[idx] = parsed_rev results.add(parsed_rev)
if not results:
raise error.Abort('Could not find local commit for D%s' % diffid)
return set(revs) return set(results)
quarkUnsubmitted
Not Done

nit: results is already a set

quark: nit: `results` is already a set
else: else:
if not vcs: if not vcs:
msg = "D%s does not have an associated version control system\n" \ msg = "D%s does not have an associated version control system\n" \
"You can view the diff at http://phabricator.fb.com/D%s\n\n" "You can view the diff at http://phabricator.fb.com/D%s\n\n"
repo.ui.warn(msg % (diffid, diffid)) repo.ui.warn(msg % (diffid, diffid))
return [] return []