This is an archive of the discontinued Mercurial Phabricator instance.

Differential D365

pushvars: cleanup the extension to just set the server config option
ClosedPublic

Authored by pulkit on Aug 12 2017, 4:52 PM.
Tags
None
Subscribers
kulshrax, quark

Details

Reviewers
quark
ryanmce
kulshrax
Group Reviewers
Restricted Project
Commits
rFBHGXbe4cb6f9e186: pushvars: cleanup the extension to just set the server config option
Summary

The --pushvars option to push is moved to core with an extra config named
push.pushvars.server which defualts to False. This config controls whether the
server to unbundle the variables send by pushvars.

To make sure nothing breaks for those with pushvars extension enabled,
this should be added to the configuration file:

[push]
pushvars.server = True

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

pulkit created this revision.Aug 12 2017, 4:52 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 12 2017, 4:52 PM
ryanmce accepted this revision.Aug 14 2017, 4:26 AM
This revision is now accepted and ready to land.Aug 14 2017, 4:26 AM
quark requested changes to this revision.Aug 16 2017, 5:35 PM
quark added a subscriber: quark.
quark added inline comments.
tests/test-pushvars-remotenames.t
15 ↗(On Diff #827)

This line shouldn't be removed. Otherwise remotenames extension will not be loaded and the test will fail.

This revision now requires changes to proceed.Aug 16 2017, 5:35 PM
quark added a comment.EditedAug 16 2017, 5:35 PM

I think we might want to keep the extension for compatibility. It could be simplified to just set the config item:

def uisetup(ui):
    ui.setconfig('push', 'pushvars.server', True)

Also, it seems remotenames does not work with --pushvars. I think we need to change remotenames after D423 to make sure it works.

ryanmce added a comment.Aug 17 2017, 10:23 AM

I'm in I'm just going to make the change that @quark suggests.

ryanmce edited reviewers, added: pulkit; removed: ryanmce.Aug 17 2017, 10:30 AM
ryanmce commandeered this revision.
ryanmce edited edge metadata.Aug 17 2017, 10:32 AM
ryanmce updated this revision to Diff 1028.

Per suggestions from @quark

ryanmce marked an inline comment as done.Aug 17 2017, 10:32 AM
ryanmce planned changes to this revision.Aug 17 2017, 1:59 PM

This doesn't seem to work.

hgext3rd/pushvars.py
16

Doesn't seem to work.

ryanmce added a comment.Aug 18 2017, 6:23 AM

I'm just going to go back to nuking this extension. Sorry for the hash.

pulkit edited edge metadata.Aug 19 2017, 7:21 PM

Sorry for the late reply. @ryanmce did it worked? Otherwise I will try.

quark added a comment.Aug 20 2017, 4:37 PM

I'm just going to go back to nuking this extension. Sorry for the hash.

Do you mean removing the remotenames test too? I think that's risky.

It's expected that the remotenames test won't pass no matter how the extension code is changed. D423 and D424 need to be applied. Or we can find another way to patch remotenames.

hgext3rd/pushvars.py
16

Do you mean it does not work even with renamenames disabled?

ryanmce added a comment.Aug 21 2017, 8:32 AM

Do you mean removing the remotenames test too? I think that's risky.

I agree it's too risky, and no I wouldn't do that. I mean that we can delete the extension instead of replacing it with a "enable a config only" version. As far I I could tell, the config approach didn't work.

It's expected that the remotenames test won't pass no matter how the extension code is changed. D423 and D424 need to be applied. Or we can find another way to patch remotenames.

I agree, this is what is currently blocking our build. I'll follow up with an internal group post about the status.

hgext3rd/pushvars.py
16

Yeah, I couldn't get the test-pushvars.t test to work when I changed the extension to have only this line. Maybe I did something stupid?

ryanmce added a subscriber: kulshrax.Aug 21 2017, 10:34 AM

@pulkit: go ahead and take another shot at it if you'd like. You'll probably want to coordinate with @kulshrax who's running our release this week. I think for now we will need to keep at least tests/test-pushvars-remotenames.t to ensure that remotenames continues to work with pushvars. Or now that I think about it, this test should probably live in the remotenames repo now since the functionality is in core.

pulkit added a comment.Aug 22 2017, 8:06 AM

With https://phab.mercurial-scm.org/D473, test-pushvars.t passes. :)

hgext3rd/pushvars.py
16

I debugged and found two different ui objects going around. So instead I did def reposetup(..) repo.ui.setconfig(...) and it worked. https://phab.mercurial-scm.org/D473

pulkit edited reviewers, added: ryanmce; removed: pulkit.Aug 22 2017, 1:39 PM
pulkit commandeered this revision.
pulkit retitled this revision from pushvars: remove the extension from hgext3rd as it is moved to core to pushvars: cleanup the extension to just set the server config option.Aug 22 2017, 1:56 PM
pulkit updated this revision to Diff 1160.
pulkit added a reviewer: kulshrax.Aug 22 2017, 2:00 PM

@kulshrax please let me know if you are taking in this in the current release and it does not work.

Closed by commit rFBHGXbe4cb6f9e186: pushvars: cleanup the extension to just set the server config option (authored by pulkit). · Explain WhyAug 22 2017, 11:58 PM
This revision was automatically updated to reflect the committed changes.
quark added a comment.Aug 23 2017, 12:01 AM

I have pushed the change. Thanks!

I noticed that the remotenames fix was not enough and updated D424.

Revision Contents
Changeset List

PathPackages
Mhgext3rd/pushvars.py (83 lines)
Mtests/test-pushvars.t (2 lines)

Diff 1179

hgext3rd/pushvars.py

# pushvars.py - enable pushing environment variables to the server # pushvars.py -- extension for setting environment variables on the server-side
# during pushes.
# #
# Copyright 2015 Facebook, Inc. # Copyright 2017 Facebook, Inc.
# #
# This software may be used and distributed according to the terms of the # This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version. # GNU General Public License version 2 or any later version.
#
# When we want to modify how a hook works or to disable it, we
# can send environment variables that the hooks can read. The
# environment variables are prepended with HG_USERVAR_ and are thus
# not a security vulnerability. For example:
#
# hg push --pushvars="BYPASS_REVIEWERS=true" --pushvars="DEBUG=1"
#
# will result in both HG_USERVAR_BYPASS_REVIEWERS and HG_USERVAR_DEBUG
# available to the hook environment.
from mercurial import bundle2, exchange, extensions, registrar
from mercurial import error, commands
from mercurial.i18n import _
cmdtable = {}
command = registrar.command(cmdtable)
@exchange.b2partsgenerator('pushvars')
def _getbundlesendvars(pushop, bundler):
'''send shellvars via bundle2'''
if getattr(pushop.repo, '_shellvars', ()):
part = bundler.newpart('pushvars')
for entry in pushop.repo._shellvars:
try:
key, value = entry.split('=', 1)
except Exception:
raise error.Abort(
_('passed in variable needs to be of form var= or var=val. '
'Instead, this was given "%s"') % entry)
part.addparam(key, value, mandatory=False)
# Ugly hack suggested by pyd to ensure pushvars part comes before
# hook part. Pyd has a fix for this in in he works.
exchange.b2partsgenorder.insert(0, exchange.b2partsgenorder.pop())
# Eventually, this will be used when we update to an Hg that supports this.
#@exchange.b2partsgenerator('pushvars', idx=0)
@bundle2.parthandler('pushvars')
def bundle2getvars(op, part):
'''unbundle a bundle2 containing shellvars on the server'''
hookargs = {}
for key, value in part.advisoryparams:
key = key.upper()
# We want pushed variables to have USERVAR_ prepended so we know
# they came from the pushvar extension.
key = "USERVAR_" + key
hookargs[key] = value
op.addhookargs(hookargs)
def push(orig, ui, repo, *args, **opts):
repo._shellvars = opts['pushvars']
try:
return orig(ui, repo, *args, **opts)
finally:
del repo._shellvars
def uisetup(ui):
# remotenames circumvents the default push implementation entirely, so make
# sure we load after it so that we wrap it.
order = list(extensions._order)
order.remove('pushvars')
order.append('pushvars')
extensions._order = order
def extsetup(ui): def reposetup(ui, repo):
entry = extensions.wrapcommand(commands.table, 'push', push) """
entry[1].append(('', 'pushvars', [], The main pushvars functionality moved into core hg. However, the behavior
"variables that can be sent to the server")) of the core version differs from this extension, which originally would
set the environment variables on the server by default when the extension
was enabled. To keep that behavior, this extension now just sets the option.
This makes the transition painless.
"""
ryanmceUnsubmitted
Not Done

Doesn't seem to work.

ryanmce: Doesn't seem to work.
quarkUnsubmitted
Not Done

Do you mean it does not work even with renamenames disabled?

quark: Do you mean it does not work even with renamenames disabled?
ryanmceUnsubmitted
Not Done

Yeah, I couldn't get the test-pushvars.t test to work when I changed the extension to have only this line. Maybe I did something stupid?

ryanmce: Yeah, I couldn't get the `test-pushvars.t` test to work when I changed the extension to have…
pulkitAuthorUnsubmitted
Not Done

I debugged and found two different ui objects going around. So instead I did def reposetup(..) repo.ui.setconfig(...) and it worked. https://phab.mercurial-scm.org/D473

pulkit: I debugged and found two different ui objects going around. So instead I did `def reposetup(..)…
repo.ui.setconfig('push', 'pushvars.server', True)

tests/test-pushvars.t

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 searching for changes
abort: passed in variable needs to be of form var= or var=val. Instead, this was given "DEBUG" abort: unable to parse variable 'DEBUG', should follow 'KEY=VALUE' or 'KEY=' format
[255] [255]