This is an archive of the discontinued Mercurial Phabricator instance.

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

Authored by pulkit on Aug 12 2017, 4:52 PM.

Download Raw Diff

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

Lint Skipped

Unit

Unit Tests Skipped

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	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

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.

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.

Per suggestions from @quark

ryanmce marked an inline comment as done.Aug 17 2017, 10:32 AM

This doesn't seem to work.

hgext3rd/pushvars.py
7	Doesn't seem to work.

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

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

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
7	Do you mean it does not work even with renamenames disabled?

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
7	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?

@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.

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

hgext3rd/pushvars.py
7	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.

@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.

I have pushed the change. Thanks!

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

quark mentioned this in D473: pushvars: cleanup the extension to just set the server config option.Aug 23 2017, 12:02 AM

Revision Contents
Changeset List

		Path
D	M	hgext3rd/pushvars.py (76 lines)
M		tests/test-pushvars-remotenames.t (4 lines)
D	M	tests/test-pushvars.t (61 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	827		Aug 12 2017, 4:52 PM	★	★
Diff 2	1028	Per suggestions from @quark	Aug 17 2017, 10:32 AM	★	★
Diff 3	1160		Aug 22 2017, 1:56 PM	★	★
Diff 4	1179	rFBHGXbe4cb6f9e1862402a67bf640d8199c301272558a	Aug 22 2017, 11:58 PM	★	★

Status	Author	Revision
Closed	pulkit	D365 pushvars: cleanup the extension to just set the server config option
Closed	pulkit	D364 bundle2hooks: remove the extension from hgext3rd as it is moved to core
Closed	pulkit	D363 commitextras: remove the extension from hgext3rd as it is moved to core

Diff 827

hgext3rd/pushvars.py

This file was deleted.

	# pushvars.py - enable pushing environment variables to the server
	#
	# Copyright 2015 Facebook, Inc.
	#
	# This software may be used and distributed according to the terms of the
	# 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):
	entry = extensions.wrapcommand(commands.table, 'push', push)
	entry[1].append(('', 'pushvars', [],
	"variables that can be sent to the server"))

tests/test-pushvars-remotenames.t

	$ . $TESTDIR/require-ext.sh remotenames			$ . $TESTDIR/require-ext.sh remotenames

	Setup			Setup

	$ PYTHONPATH=$TESTDIR/..:$PYTHONPATH			$ PYTHONPATH=$TESTDIR/..:$PYTHONPATH
	$ export PYTHONPATH			$ export PYTHONPATH

	$ cat > $TESTTMP/pretxnchangegroup.sh << EOF			$ cat > $TESTTMP/pretxnchangegroup.sh << EOF
	> #!/bin/bash			> #!/bin/bash
	> env \| grep -E "^HG_USERVAR"			> env \| grep -E "^HG_USERVAR"
	> exit 0			> exit 0
	> EOF			> EOF
	$ chmod +x $TESTTMP/pretxnchangegroup.sh			$ chmod +x $TESTTMP/pretxnchangegroup.sh
	$ cat >> $HGRCPATH << EOF			$ cat >> $HGRCPATH << EOF
	> [extensions]
	quarkUnsubmitted Done This line shouldn't be removed. Otherwise `remotenames` extension will not be loaded and the test will fail. quark: This line shouldn't be removed. Otherwise `remotenames` extension will not be loaded and the…
	> pushvars=$TESTDIR/../hgext3rd/pushvars.py
	> remotenames=			> remotenames=
	> [hooks]			> [hooks]
	> pretxnchangegroup = $TESTTMP/pretxnchangegroup.sh			> pretxnchangegroup = $TESTTMP/pretxnchangegroup.sh
				> [push]
				> pushvars.server = true
	> EOF			> EOF

	$ hg init server			$ hg init server
	$ cd server			$ cd server

	$ echo x > x			$ echo x > x
	$ hg commit -qAm x			$ hg commit -qAm x
	$ hg book master			$ hg book master

tests/test-pushvars.t

This file was deleted.

	Setup

	$ PYTHONPATH=$TESTDIR/..:$PYTHONPATH
	$ export PYTHONPATH

	$ cat > $TESTTMP/pretxnchangegroup.sh << EOF
	> #!/bin/bash
	> env \| grep -E "^HG_USERVAR_DEBUG"
	> env \| grep -E "^HG_USERVAR_BYPASS_REVIEW"
	> exit 0
	> EOF
	$ chmod +x $TESTTMP/pretxnchangegroup.sh
	$ cat >> $HGRCPATH << EOF
	> [extensions]
	> pushvars=$TESTDIR/../hgext3rd/pushvars.py
	> [hooks]
	> pretxnchangegroup = $TESTTMP/pretxnchangegroup.sh
	> [experimental]
	> bundle2-exp = true
	> EOF

	$ hg init repo
	$ hg clone -q repo child
	$ cd child

	Test pushing vars to repo

	$ echo b > a
	$ hg commit -Aqm a
	$ hg push --pushvars "DEBUG=1" --pushvars "BYPASS_REVIEW=true"
	pushing to $TESTTMP/repo (glob)
	searching for changes
	adding changesets
	adding manifests
	adding file changes
	added 1 changesets with 1 changes to 1 files
	HG_USERVAR_DEBUG=1
	HG_USERVAR_BYPASS_REVIEW=true

	Test pushing var with empty right-hand side

	$ echo b >> a
	$ hg commit -Aqm a
	$ hg push --pushvars "DEBUG="
	pushing to $TESTTMP/repo (glob)
	searching for changes
	adding changesets
	adding manifests
	adding file changes
	added 1 changesets with 1 changes to 1 files
	HG_USERVAR_DEBUG=

	Test pushing bad vars

	$ echo b >> a
	$ hg commit -Aqm b
	$ hg push --pushvars "DEBUG"
	pushing to $TESTTMP/repo (glob)
	searching for changes
	abort: passed in variable needs to be of form var= or var=val. Instead, this was given "DEBUG"
	[255]