Minimize use of preexec_fn

Python suggests avoiding preexec_fn, and in Python 3, changes to the
PATH no longer affect the subprocess invocation, so replace it with an
env setting where we can, and they've also added start_new_session to
provide setsid(), so we'll use that too when possible.

Signed-off-by: Rob Browning <rlb@defaultvalue.org>
Tested-by: Rob Browning <rlb@defaultvalue.org>

diff --git a/cmd/server-cmd.py b/cmd/server-cmd.py
index 6fa9a67a96d8ac68dd7f748315cf6003a4999184..abb02203c78fb77ab10bc32226c59231b34b012c 100755 (executable)
--- a/cmd/server-cmd.py
+++ b/cmd/server-cmd.py
@@ -219,7 +219,7 @@ def rev_list(conn, _):
     refs = tuple(x[:-1] for x in lines_until_sentinel(conn, '\n', Exception))
     args = git.rev_list_invocation(refs, count=count, format=fmt)
     p = subprocess.Popen(git.rev_list_invocation(refs, count=count, format=fmt),
-                         preexec_fn=git._gitenv(git.repodir),
+                         env=git._gitenv(git.repodir),
                          stdout=subprocess.PIPE)
     while True:
         out = p.stdout.read(64 * 1024)

diff --git a/lib/bup/gc.py b/lib/bup/gc.py
index 7491b5bc31151a9a5db1d78ab609bd2fae8e1341..a70339eb1ebb6af50caf0f469c60ecd79b4a7de1 100644 (file)
--- a/lib/bup/gc.py
+++ b/lib/bup/gc.py
@@ -239,7 +239,7 @@ def bup_gc(threshold=10, compression=1, verbosity=0):
             bloom.clear_bloom(packdir)
             if verbosity: log('clearing reflog\n')
             expirelog_cmd = ['git', 'reflog', 'expire', '--all', '--expire=all']
-            expirelog = subprocess.Popen(expirelog_cmd, preexec_fn = git._gitenv())
+            expirelog = subprocess.Popen(expirelog_cmd, env=git._gitenv())
             git._git_wait(' '.join(expirelog_cmd), expirelog)
             if verbosity: log('removing unreachable data\n')
             sweep(live_objects, existing_count, cat_pipe,

diff --git a/lib/bup/git.py b/lib/bup/git.py
index 2cf1c911daeb625f450c737cef3ad819f312c4ef..b33d964cfccb11f12d9ac7f876876c859f4902e2 100644 (file)
--- a/lib/bup/git.py
+++ b/lib/bup/git.py
@@ -13,7 +13,9 @@ from bup import _helpers, compat, hashsplit, path, midx, bloom, xstat
 from bup.compat import range
 from bup.helpers import (Sha1, add_error, chunkyreader, debug1, debug2,
                          fdatasync,
-                         hostname, localtime, log, merge_iter,
+                         hostname, localtime, log,
+                         merge_dict,
+                         merge_iter,
                          mmap_read, mmap_readwrite,
                          parse_num,
                          progress, qprogress, shstr, stat_if_exists,
@@ -35,13 +37,18 @@ class GitError(Exception):
     pass
 
 
+def _gitenv(repo_dir=None):
+    if not repo_dir:
+        repo_dir = repo()
+    return merge_dict(os.environ, {'GIT_DIR': os.path.abspath(repo_dir)})
+
 def _git_wait(cmd, p):
     rv = p.wait()
     if rv != 0:
         raise GitError('%s returned %d' % (shstr(cmd), rv))
 
 def _git_capture(argv):
-    p = subprocess.Popen(argv, stdout=subprocess.PIPE, preexec_fn = _gitenv())
+    p = subprocess.Popen(argv, stdout=subprocess.PIPE, env=_gitenv())
     r = p.stdout.read()
     _git_wait(repr(argv), p)
     return r
@@ -49,7 +56,7 @@ def _git_capture(argv):
 def git_config_get(option, repo_dir=None):
     cmd = ('git', 'config', '--get', option)
     p = subprocess.Popen(cmd, stdout=subprocess.PIPE,
-                         preexec_fn=_gitenv(repo_dir=repo_dir))
+                         env=_gitenv(repo_dir=repo_dir))
     r = p.stdout.read()
     rc = p.wait()
     if rc == 0:
@@ -887,14 +894,6 @@ class PackWriter:
             idx_f.close()
 
 
-def _gitenv(repo_dir = None):
-    if not repo_dir:
-        repo_dir = repo()
-    def env():
-        os.environ['GIT_DIR'] = os.path.abspath(repo_dir)
-    return env
-
-
 def list_refs(patterns=None, repo_dir=None,
               limit_to_heads=False, limit_to_tags=False):
     """Yield (refname, hash) tuples for all repository refs unless
@@ -912,9 +911,7 @@ def list_refs(patterns=None, repo_dir=None,
     argv.append('--')
     if patterns:
         argv.extend(patterns)
-    p = subprocess.Popen(argv,
-                         preexec_fn = _gitenv(repo_dir),
-                         stdout = subprocess.PIPE)
+    p = subprocess.Popen(argv, env=_gitenv(repo_dir), stdout=subprocess.PIPE)
     out = p.stdout.read().strip()
     rv = p.wait()  # not fatal
     if rv:
@@ -968,7 +965,7 @@ def rev_list(ref_or_refs, count=None, parse=None, format=None, repo_dir=None):
     assert bool(parse) == bool(format)
     p = subprocess.Popen(rev_list_invocation(ref_or_refs, count=count,
                                              format=format),
-                         preexec_fn = _gitenv(repo_dir),
+                         env=_gitenv(repo_dir),
                          stdout = subprocess.PIPE)
     if not format:
         for line in p.stdout:
@@ -1035,7 +1032,7 @@ def update_ref(refname, newval, oldval, repo_dir=None):
            or refname.startswith('refs/tags/'))
     p = subprocess.Popen(['git', 'update-ref', refname,
                           newval.encode('hex'), oldval.encode('hex')],
-                         preexec_fn = _gitenv(repo_dir))
+                         env=_gitenv(repo_dir))
     _git_wait('git update-ref', p)
 
 
@@ -1044,7 +1041,7 @@ def delete_ref(refname, oldvalue=None):
     assert(refname.startswith('refs/'))
     oldvalue = [] if not oldvalue else [oldvalue]
     p = subprocess.Popen(['git', 'update-ref', '-d', refname] + oldvalue,
-                         preexec_fn = _gitenv())
+                         env=_gitenv())
     _git_wait('git update-ref', p)
 
 
@@ -1074,16 +1071,16 @@ def init_repo(path=None):
     if os.path.exists(d) and not os.path.isdir(os.path.join(d, '.')):
         raise GitError('"%s" exists but is not a directory\n' % d)
     p = subprocess.Popen(['git', '--bare', 'init'], stdout=sys.stderr,
-                         preexec_fn = _gitenv())
+                         env=_gitenv())
     _git_wait('git init', p)
     # Force the index version configuration in order to ensure bup works
     # regardless of the version of the installed Git binary.
     p = subprocess.Popen(['git', 'config', 'pack.indexVersion', '2'],
-                         stdout=sys.stderr, preexec_fn = _gitenv())
+                         stdout=sys.stderr, env=_gitenv())
     _git_wait('git config', p)
     # Enable the reflog
     p = subprocess.Popen(['git', 'config', 'core.logAllRefUpdates', 'true'],
-                         stdout=sys.stderr, preexec_fn = _gitenv())
+                         stdout=sys.stderr, env=_gitenv())
     _git_wait('git config', p)
 
 
@@ -1187,7 +1184,7 @@ class CatPipe:
                                   stdout=subprocess.PIPE,
                                   close_fds = True,
                                   bufsize = 4096,
-                                  preexec_fn = _gitenv(self.repo_dir))
+                                  env=_gitenv(self.repo_dir))
 
     def get(self, ref):
         """Yield (oidx, type, size), followed by the data referred to by ref.

diff --git a/lib/bup/helpers.py b/lib/bup/helpers.py
index 9a1218b9f8f4030c29a41af8163b1b6dd611e01a..333a491efef1413299a1f926ab2fab78049d705d 100644 (file)
--- a/lib/bup/helpers.py
+++ b/lib/bup/helpers.py
@@ -99,6 +99,13 @@ def partition(predicate, stream):
     return (leading_matches(), rest())
 
 
+def merge_dict(*xs):
+    result = {}
+    for x in xs:
+        result.update(x)
+    return result
+
+
 def lines_until_sentinel(f, sentinel, ex_type):
     # sentinel must end with \n and must contain only one \n
     while True:

diff --git a/lib/bup/ssh.py b/lib/bup/ssh.py
index 97ab3121a7177b9a0e0f24f4b39e795767e9fe8f..5602921b2d77e32152fc5a21b594d4ee381973c9 100644 (file)
--- a/lib/bup/ssh.py
+++ b/lib/bup/ssh.py
@@ -2,7 +2,7 @@
 Connect to a remote host via SSH and execute a command on the host.
 """
 
-from __future__ import absolute_import
+from __future__ import absolute_import, print_function
 import sys, os, re, subprocess
 from bup import helpers, path
 
@@ -34,12 +34,21 @@ def connect(rhost, port, subcmd, stderr=None):
             argv.extend(('-p', port))
         argv.extend((rhost, '--', cmd.strip()))
         #helpers.log('argv is: %r\n' % argv)
-    def setup():
-        # runs in the child process
-        if not rhost:
-            os.environ['PATH'] = ':'.join([nicedir,
-                                           os.environ.get('PATH', '')])
-        os.setsid()
-    return subprocess.Popen(argv, stdin=subprocess.PIPE, stdout=subprocess.PIPE,
-                            stderr=stderr,
-                            preexec_fn=setup)
+    if rhost:
+        env = os.environ
+    else:
+        envpath = os.environ.get('PATH')
+        env = os.environ.copy()
+        env['PATH'] = nicedir if not envpath else nicedir + ':' + envpath
+    if sys.version_info[0] < 3:
+        return subprocess.Popen(argv,
+                                stdin=subprocess.PIPE, stdout=subprocess.PIPE,
+                                stderr=stderr,
+                                env=env,
+                                preexec_fn=lambda: os.setsid())
+    else:
+        return subprocess.Popen(argv,
+                                stdin=subprocess.PIPE, stdout=subprocess.PIPE,
+                                stderr=stderr,
+                                env=env,
+                                start_new_session=True)

diff --git a/main.py b/main.py
index ec86b2ac8376c7b4055d36db674b2edd544a531e..78da11de049689c7b81736a179d0e4c1a63463f3 100755 (executable)
--- a/main.py
+++ b/main.py
@@ -148,10 +148,12 @@ if subcmd_name in ['mux', 'ftp', 'help']:
 fix_stdout = not already_fixed and os.isatty(1)
 fix_stderr = not already_fixed and os.isatty(2)
 
-def force_tty():
-    if fix_stdout or fix_stderr:
-        amt = (fix_stdout and 1 or 0) + (fix_stderr and 2 or 0)
-        os.environ['BUP_FORCE_TTY'] = str(amt)
+if fix_stdout or fix_stderr:
+    tty_env = merge_dict(os.environ,
+                         {'BUP_FORCE_TTY': str((fix_stdout and 1 or 0)
+                                               + (fix_stderr and 2 or 0))})
+else:
+    tty_env = os.environ
 
 
 sep_rx = re.compile(br'([\r\n])')
@@ -240,9 +242,7 @@ def run_subcmd(subcmd):
         p = subprocess.Popen(c,
                              stdout=PIPE if fix_stdout else sys.stdout,
                              stderr=PIPE if fix_stderr else sys.stderr,
-                             preexec_fn=force_tty,
-                             bufsize=4096,
-                             close_fds=True)
+                             env=tty_env, bufsize=4096, close_fds=True)
         # Assume p will receive these signals and quit, which will
         # then cause us to quit.
         for sig in (signal.SIGINT, signal.SIGTERM, signal.SIGQUIT):

diff --git a/t/test-get b/t/test-get
index 41656ef7b0f7da15e5fb48fd6b5691878d8d4c82..7f5c7d0e326c0243f49e4d89b20e7dfdc50430e2 100755 (executable)
--- a/t/test-get
+++ b/t/test-get
@@ -18,7 +18,7 @@ script_home = abspath(dirname(sys.argv[0] or '.'))
 sys.path[:0] = [abspath(script_home + '/../lib'), abspath(script_home + '/..')]
 
 from bup import compat
-from bup.helpers import unlink
+from bup.helpers import merge_dict, unlink
 from buptest import ex, exo, test_tempdir
 from wvtest import wvcheck, wvfail, wvmsg, wvpass, wvpasseq, wvpassne, wvstart
 
@@ -84,21 +84,20 @@ def validate_blob(src_id, dest_id):
 
 def validate_tree(src_id, dest_id):
 
-    def set_committer_date():
-        environ['GIT_COMMITTER_DATE'] = "2014-01-01 01:01"
-
     rmrf('restore-src')
     rmrf('restore-dest')
     mkdir('restore-src')
     mkdir('restore-dest')
     
+    commit_env = merge_dict(environ, {'GIT_COMMITTER_DATE': '2014-01-01 01:01'})
+
     # Create a commit so the archive contents will have matching timestamps.
     src_c = exo(('git', '--git-dir', 'get-src',
                  'commit-tree', '-m', 'foo', src_id),
-                preexec_fn=set_committer_date).out.strip()
+                env=commit_env).out.strip()
     dest_c = exo(('git', '--git-dir', 'get-dest',
                   'commit-tree', '-m', 'foo', dest_id),
-                 preexec_fn=set_committer_date).out.strip()
+                 env=commit_env).out.strip()
     exr = verify_rcz('git --git-dir get-src archive %s | tar xvf - -C restore-src'
                      % quote(src_c),
                      shell=True)