( )⚙ D423 pushvars: do not mangle repo state

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D423

pushvars: do not mangle repo state
ClosedPublic

Authored by quark on Aug 16 2017, 6:54 PM.

Details

Reviewers
dsp
ryanmce
durin42
Group Reviewers
hg-reviewers
Commits
rHG800bb35d891e: pushvars: do not mangle repo state
Summary

Setting repo._shellvars works but is not a clean way to pass the pushvars
information from the push command to the exchange operation. Therefore
change it to actually pass pushvars as a push operation argument instead.

This makes third party extension like remotenames easier to support pushvars
cleanly. The key value parsing and verification code has been moved to a
lower level so it's harder to be bypassed and easier to be used in
remotenames which could replace push command entirely.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Aug 16 2017, 6:54 PM
dsp added a subscriber: dsp.Aug 16 2017, 11:28 PM

this looks good to me. One consideration that users would get warnings about pushvars parsing failing fairly late now, compared to directly before we attempt a push. This can be annoying if searching for changes takes a long time. We could attempt to test if they are parsable in commands.py to avoid this.

dsp accepted this revision.Aug 16 2017, 11:28 PM
ryanmce accepted this revision.Aug 21 2017, 10:35 AM
ryanmce added a subscriber: ryanmce.

It would be really nice to get this change in.

durin42 accepted this revision.Aug 23 2017, 11:50 AM
This revision is now accepted and ready to land.Aug 23 2017, 11:50 AM
Closed by commit rHG800bb35d891e: pushvars: do not mangle repo state (authored by quark). · Explain WhyAug 23 2017, 12:00 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/commands.py (19 lines)
Mmercurial/exchange.py (18 lines)
Mtests/test-pushvars.t (1 line)

Diff 1019

mercurial/commands.py

subs = c.substate # only repos that are committed subs = c.substate # only repos that are committed
for s in sorted(subs): for s in sorted(subs):
result = c.sub(s).push(opts) result = c.sub(s).push(opts)
if result == 0: if result == 0:
return not result return not result
finally: finally:
del repo._subtoppath del repo._subtoppath
pushvars = opts.get('pushvars') opargs = dict(opts.get('opargs', {})) # copy opargs since we may mutate it
if pushvars: opargs.setdefault('pushvars', []).extend(opts.get('pushvars', []))
shellvars = {}
for raw in pushvars:
if '=' not in raw:
msg = ("unable to parse variable '%s', should follow "
"'KEY=VALUE' or 'KEY=' format")
raise error.Abort(msg % raw)
k, v = raw.split('=', 1)
shellvars[k] = v
repo._shellvars = shellvars
pushop = exchange.push(repo, other, opts.get('force'), revs=revs, pushop = exchange.push(repo, other, opts.get('force'), revs=revs,
newbranch=opts.get('new_branch'), newbranch=opts.get('new_branch'),
bookmarks=opts.get('bookmark', ()), bookmarks=opts.get('bookmark', ()),
opargs=opts.get('opargs')) opargs=opargs)
if pushvars:
del repo._shellvars
result = not pushop.cgresult result = not pushop.cgresult
if pushop.bkresult is not None: if pushop.bkresult is not None:
if pushop.bkresult == 2: if pushop.bkresult == 2:
result = 2 result = 2
elif not result and pushop.bkresult: elif not result and pushop.bkresult:
result = 2 result = 2

mercurial/exchange.py

Its purpose is to carry push related state and very common operations. Its purpose is to carry push related state and very common operations.
A new pushoperation should be created at the beginning of each push and A new pushoperation should be created at the beginning of each push and
discarded afterward. discarded afterward.
""" """
def __init__(self, repo, remote, force=False, revs=None, newbranch=False, def __init__(self, repo, remote, force=False, revs=None, newbranch=False,
bookmarks=()): bookmarks=(), pushvars=None):
# repo we push from # repo we push from
self.repo = repo self.repo = repo
self.ui = repo.ui self.ui = repo.ui
# repo we push to # repo we push to
self.remote = remote self.remote = remote
# force option provided # force option provided
self.force = force self.force = force
# revs to be pushed (None is "all") # revs to be pushed (None is "all")
self.outobsmarkers = set() self.outobsmarkers = set()
# outgoing bookmarks # outgoing bookmarks
self.outbookmarks = [] self.outbookmarks = []
# transaction manager # transaction manager
self.trmanager = None self.trmanager = None
# map { pushkey partid -> callback handling failure} # map { pushkey partid -> callback handling failure}
# used to handle exception from mandatory pushkey part failure # used to handle exception from mandatory pushkey part failure
self.pkfailcb = {} self.pkfailcb = {}
# an iterable of pushvars or None
self.pushvars = pushvars
@util.propertycache @util.propertycache
def futureheads(self): def futureheads(self):
"""future remote heads if the changeset push succeeds""" """future remote heads if the changeset push succeeds"""
return self.outgoing.missingheads return self.outgoing.missingheads
@util.propertycache @util.propertycache
def fallbackheads(self): def fallbackheads(self):
ui.warn(bookmsgmap[action][1] % book) ui.warn(bookmsgmap[action][1] % book)
if pushop.bkresult is not None: if pushop.bkresult is not None:
pushop.bkresult = 1 pushop.bkresult = 1
return handlereply return handlereply
@b2partsgenerator('pushvars', idx=0) @b2partsgenerator('pushvars', idx=0)
def _getbundlesendvars(pushop, bundler): def _getbundlesendvars(pushop, bundler):
'''send shellvars via bundle2''' '''send shellvars via bundle2'''
if getattr(pushop.repo, '_shellvars', ()): pushvars = pushop.pushvars
if pushvars:
shellvars = {}
for raw in pushvars:
if '=' not in raw:
msg = ("unable to parse variable '%s', should follow "
"'KEY=VALUE' or 'KEY=' format")
raise error.Abort(msg % raw)
k, v = raw.split('=', 1)
shellvars[k] = v
part = bundler.newpart('pushvars') part = bundler.newpart('pushvars')
for key, value in pushop.repo._shellvars.iteritems(): for key, value in shellvars.iteritems():
part.addparam(key, value, mandatory=False) part.addparam(key, value, mandatory=False)
def _pushbundle2(pushop): def _pushbundle2(pushop):
"""push data to the remote using bundle2 """push data to the remote using bundle2
The only currently supported type of data is changegroup but this will The only currently supported type of data is changegroup but this will
evolve in the future.""" evolve in the future."""
bundler = bundle2.bundle20(pushop.ui, bundle2.bundle2caps(pushop.remote)) bundler = bundle2.bundle20(pushop.ui, bundle2.bundle2caps(pushop.remote))

tests/test-pushvars.t

HG_USERVAR_DEBUG= HG_USERVAR_DEBUG=
Test pushing bad vars Test pushing bad vars
$ echo b >> a $ echo b >> a
$ hg commit -Aqm b $ hg commit -Aqm b
$ hg push --pushvars "DEBUG" $ hg push --pushvars "DEBUG"
pushing to $TESTTMP/repo (glob) pushing to $TESTTMP/repo (glob)
searching for changes
abort: unable to parse variable 'DEBUG', should follow 'KEY=VALUE' or 'KEY=' format abort: unable to parse variable 'DEBUG', should follow 'KEY=VALUE' or 'KEY=' format
[255] [255]