This is an archive of the discontinued Mercurial Phabricator instance.

Differential D79

phabstatus: clean up showsyncstatus()
ClosedPublic

Authored by simpkins on Jul 13 2017, 10:29 PM.
Tags
None
Subscribers
ryanmce

Details

Reviewers
ryanmce
Group Reviewers
Restricted Project
Commits
rFBHGXb2de23e301d2: phabstatus: clean up showsyncstatus()
Summary

Refactor the showsyncstatus function to clean up the code a little bit.

  • Don't bother to call populateresponseforphab() if the current commit does not contain a differential revision ID.
  • Only call getdiffstatus() once instead of 4 times. getdiffstatus() does cache the result instead of hitting phabricator each time, but it is still better to just re-use the return value ourselves instead of hitting the memoization code.
  • Refactor the error handling to avoid having deeply nested conditional blocks.
Test Plan

Confirmed existing tests still pass, and manually tested using the template.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

simpkins created this revision.Jul 13 2017, 10:29 PM
ryanmce accepted this revision.EditedJul 19 2017, 9:54 AM
ryanmce added a subscriber: ryanmce.

Easier to review by ignoring all ws: https://phab.mercurial-scm.org/D79?whitespace=ignore-all

This revision is now accepted and ready to land.Jul 19 2017, 9:54 AM
Closed by commit rFBHGXb2de23e301d2: phabstatus: clean up showsyncstatus() (authored by simpkins). · Explain WhyJul 19 2017, 10:22 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext3rd/phabstatus.py (57 lines)

Diff 328

hgext3rd/phabstatus.py

don't say anything. All good. don't say anything. All good.
3) If this is a middle revision: Then we compare the hashes as regular. 3) If this is a middle revision: Then we compare the hashes as regular.
""" """
@templatekeyword('syncstatus') @templatekeyword('syncstatus')
def showsyncstatus(repo, ctx, templ, **args): def showsyncstatus(repo, ctx, templ, **args):
"""String. Return whether the local revision is in sync """String. Return whether the local revision is in sync
with the remote (phabricator) revision with the remote (phabricator) revision
""" """
populateresponseforphab(repo, ctx)
diffnum = getdiffnum(repo, ctx) diffnum = getdiffnum(repo, ctx)
local = ctx.hex() if diffnum is None:
if diffnum is not None: return None
result = getdiffstatus(repo, diffnum)[0]
if isinstance(result, dict) and "hash" in result \ populateresponseforphab(repo, ctx)
and "status" in result and "count" in result: results = getdiffstatus(repo, diffnum)
remote = getdiffstatus(repo, diffnum)[0].get("hash") try:
status = getdiffstatus(repo, diffnum)[0].get("status") result = results[0]
count = int(getdiffstatus(repo, diffnum)[0].get("count")) remote = result["hash"]
status = result["status"]
count = int(result["count"])
except (IndexError, KeyError, ValueError, TypeError):
# We got no result back, or it did not contain all required fields
return "Error"
local = ctx.hex()
if local == remote: if local == remote:
return "sync" return "sync"
elif count == 1: elif count == 1:
precursors = list(obsutil.allprecursors(repo.obsstore, precursors = list(obsutil.allprecursors(repo.obsstore,
[ctx.node()])) [ctx.node()]))
hashes = [repo.unfiltered()[h].hex() for h in precursors] hashes = [repo.unfiltered()[h].hex() for h in precursors]
# hashes[0] is the current # hashes[0] is the current
# hashes[1] is the previous # hashes[1] is the previous
if len(hashes) > 1 and hashes[1] == remote: if len(hashes) > 1 and hashes[1] == remote:
return "sync" return "sync"
else: else:
return "unsync" return "unsync"
elif status == "Committed": elif status == "Committed":
return "committed" return "committed"
else: else:
return "unsync" return "unsync"
else:
return "Error"
def getdiffnum(repo, ctx): def getdiffnum(repo, ctx):
return diffprops.parserevfromcommitmsg(ctx.description()) return diffprops.parserevfromcommitmsg(ctx.description())
def _getdag(orig, *args): def _getdag(orig, *args):
repo = args[1] repo = args[1]
# We retain the smartlogrevision, this way showphabstatus knows that there # We retain the smartlogrevision, this way showphabstatus knows that there
# are multiple revisions to resolve # are multiple revisions to resolve