[gentoo-portage-dev] [PATCH] bin/doins.py: implement install -p option (bug 642632)

[gentoo-portage-dev] [PATCH] bin/doins.py: implement install -p option (bug 642632)

[Zac Medico]

Bug: https://bugs.gentoo.org/642632
---
 bin/doins.py                        | 13 ++++++++++---
 pym/portage/tests/bin/test_doins.py |  4 ++--
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/bin/doins.py b/bin/doins.py
index 92e450979..dceffee83 100644
--- a/bin/doins.py
+++ b/bin/doins.py
@@ -107,6 +107,7 @@ def _parse_install_options(
 	parser.add_argument('-g', '--group', default=-1, type=_parse_group)
 	parser.add_argument('-o', '--owner', default=-1, type=_parse_user)
 	parser.add_argument('-m', '--mode', default=0o755, type=_parse_mode)
+	parser.add_argument('-p', '--preserve-timestamps', action='store_true')
 	split_options = shlex.split(options)
 	namespace, remaining = parser.parse_known_args(split_options)
 	# Because parsing '--mode' option is partially supported. If unknown
@@ -168,7 +169,8 @@ class _InsInProcessInstallRunner(object):
 			True on success, otherwise False.
 		"""
 		dest = os.path.join(dest_dir, os.path.basename(source))
-		if not self._is_install_allowed(source, dest):
+		sstat = os.stat(source)
+		if not self._is_install_allowed(source, sstat, dest):
 			return False
 
 		# To emulate the `install` command, remove the dest file in
@@ -187,6 +189,11 @@ class _InsInProcessInstallRunner(object):
 				movefile._copyxattr(
 					source, dest,
 					exclude=self._xattr_exclude)
+			if self._parsed_options.preserve_timestamps:
+				if sys.version_info >= (3, 3):
+					os.utime(dest, ns=(sstat.st_mtime_ns, sstat.st_mtime_ns))
+				else:
+					os.utime(dest, (sstat.st_mtime, sstat.st_mtime))
 		except Exception:
 			logging.exception(
 				'Failed to copy file: '
@@ -195,13 +202,14 @@ class _InsInProcessInstallRunner(object):
 			return False
 		return True
 
-	def _is_install_allowed(self, source, dest):
+	def _is_install_allowed(self, source, source_stat, dest):
 		"""Returns if installing source into dest should work.
 
 		This is to keep compatibility with the `install` command.
 
 		Args:
 			source: path to the source file.
+			source_stat: stat result for the source file.
 			dest: path to the dest file.
 
 		Returns:
@@ -210,7 +218,6 @@ class _InsInProcessInstallRunner(object):
 		# To match `install` command, use stat() for source, while
 		# lstat() for dest. Raise an exception if stat(source) fails,
 		# intentionally.
-		source_stat = os.stat(source)
 		try:
 			dest_lstat = os.lstat(dest)
 		except OSError as e:
diff --git a/pym/portage/tests/bin/test_doins.py b/pym/portage/tests/bin/test_doins.py
index 14d7adfa6..dd40abf6e 100644
--- a/pym/portage/tests/bin/test_doins.py
+++ b/pym/portage/tests/bin/test_doins.py
@@ -38,7 +38,7 @@ class DoIns(setup_env.BinTestCase):
 		self.init()
 		try:
 			env = setup_env.env
-			env['INSOPTIONS'] = '-m0644'
+			env['INSOPTIONS'] = '-pm0644'
 			with open(os.path.join(env['S'], 'test'), 'w'):
 				pass
 			doins('test')
@@ -145,7 +145,7 @@ class DoIns(setup_env.BinTestCase):
 			env = setup_env.env
 			# Use an option which doins.py does not know.
 			# Then, fallback to `install` command is expected.
-			env['INSOPTIONS'] = '-p'
+			env['INSOPTIONS'] = '-b'
 			with open(os.path.join(env['S'], 'test'), 'w'):
 				pass
 			doins('test')
-- 
2.13.6

Re: [gentoo-portage-dev] [PATCH] bin/doins.py: implement install -p option (bug 642632)

[Mike Gilbert]

On Fri, Dec 29, 2017 at 3:35 PM, Zac Medico <zmedico@gentoo.org> wrote:
> +                       if self._parsed_options.preserve_timestamps:
> +                               if sys.version_info >= (3, 3):
> +                                       os.utime(dest, ns=(sstat.st_mtime_ns, sstat.st_mtime_ns))
> +                               else:
> +                                       os.utime(dest, (sstat.st_mtime, sstat.st_mtime))

It looks like you are copying mtime into both mtime and atime on the
new file. Is that a mistake?

[gentoo-portage-dev] [PATCH v2] bin/doins.py: implement install -p option (bug 642632)

[Zac Medico]

Bug: https://bugs.gentoo.org/642632
---
[PATCH v2] fix to copy atime, and split out _set_timestamps function

 bin/doins.py                        | 28 +++++++++++++++++++++++++---
 pym/portage/tests/bin/test_doins.py |  6 ++++--
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/bin/doins.py b/bin/doins.py
index 92e450979..0d03d8fb2 100644
--- a/bin/doins.py
+++ b/bin/doins.py
@@ -107,6 +107,7 @@ def _parse_install_options(
 	parser.add_argument('-g', '--group', default=-1, type=_parse_group)
 	parser.add_argument('-o', '--owner', default=-1, type=_parse_user)
 	parser.add_argument('-m', '--mode', default=0o755, type=_parse_mode)
+	parser.add_argument('-p', '--preserve-timestamps', action='store_true')
 	split_options = shlex.split(options)
 	namespace, remaining = parser.parse_known_args(split_options)
 	# Because parsing '--mode' option is partially supported. If unknown
@@ -139,6 +140,24 @@ def _set_attributes(options, path):
 		os.chmod(path, options.mode)
 
 
+def _set_timestamps(source_stat, dest):
+	"""Apply timestamps from source_stat to dest.
+
+	Args:
+		source_stat: stat result for the source file.
+		dest: path to the dest file.
+	"""
+	os.utime(dest, (source_stat.st_atime, source_stat.st_mtime))
+
+
+if sys.version_info >= (3, 3):
+	def _set_timestamps_ns(source_stat, dest):
+		os.utime(dest, ns=(source_stat.st_atime_ns, source_stat.st_mtime_ns))
+
+	_set_timestamps_ns.__doc__ = _set_timestamps.__doc__
+	_set_timestamps = _set_timestamps_ns
+
+
 class _InsInProcessInstallRunner(object):
 	"""Implements `install` command behavior running in a process."""
 
@@ -168,7 +187,8 @@ class _InsInProcessInstallRunner(object):
 			True on success, otherwise False.
 		"""
 		dest = os.path.join(dest_dir, os.path.basename(source))
-		if not self._is_install_allowed(source, dest):
+		sstat = os.stat(source)
+		if not self._is_install_allowed(source, sstat, dest):
 			return False
 
 		# To emulate the `install` command, remove the dest file in
@@ -187,6 +207,8 @@ class _InsInProcessInstallRunner(object):
 				movefile._copyxattr(
 					source, dest,
 					exclude=self._xattr_exclude)
+			if self._parsed_options.preserve_timestamps:
+				_set_timestamps(sstat, dest)
 		except Exception:
 			logging.exception(
 				'Failed to copy file: '
@@ -195,13 +217,14 @@ class _InsInProcessInstallRunner(object):
 			return False
 		return True
 
-	def _is_install_allowed(self, source, dest):
+	def _is_install_allowed(self, source, source_stat, dest):
 		"""Returns if installing source into dest should work.
 
 		This is to keep compatibility with the `install` command.
 
 		Args:
 			source: path to the source file.
+			source_stat: stat result for the source file.
 			dest: path to the dest file.
 
 		Returns:
@@ -210,7 +233,6 @@ class _InsInProcessInstallRunner(object):
 		# To match `install` command, use stat() for source, while
 		# lstat() for dest. Raise an exception if stat(source) fails,
 		# intentionally.
-		source_stat = os.stat(source)
 		try:
 			dest_lstat = os.lstat(dest)
 		except OSError as e:
diff --git a/pym/portage/tests/bin/test_doins.py b/pym/portage/tests/bin/test_doins.py
index 14d7adfa6..9b6c55d85 100644
--- a/pym/portage/tests/bin/test_doins.py
+++ b/pym/portage/tests/bin/test_doins.py
@@ -38,13 +38,15 @@ class DoIns(setup_env.BinTestCase):
 		self.init()
 		try:
 			env = setup_env.env
-			env['INSOPTIONS'] = '-m0644'
+			env['INSOPTIONS'] = '-pm0644'
 			with open(os.path.join(env['S'], 'test'), 'w'):
 				pass
 			doins('test')
 			st = os.lstat(env['D'] + '/test')
 			if stat.S_IMODE(st.st_mode) != 0o644:
 				raise tests.TestCase.failureException
+			if os.stat(os.path.join(env['S'], 'test')).st_mtime != st.st_mtime:
+				raise tests.TestCase.failureException
 		finally:
 			self.cleanup()
 
@@ -145,7 +147,7 @@ class DoIns(setup_env.BinTestCase):
 			env = setup_env.env
 			# Use an option which doins.py does not know.
 			# Then, fallback to `install` command is expected.
-			env['INSOPTIONS'] = '-p'
+			env['INSOPTIONS'] = '-b'
 			with open(os.path.join(env['S'], 'test'), 'w'):
 				pass
 			doins('test')
-- 
2.13.6

Re: [gentoo-portage-dev] [PATCH] bin/doins.py: implement install -p option (bug 642632)

[Zac Medico]

On 12/29/2017 02:13 PM, Mike Gilbert wrote:
> On Fri, Dec 29, 2017 at 3:35 PM, Zac Medico <zmedico@gentoo.org> wrote:
>> +                       if self._parsed_options.preserve_timestamps:
>> +                               if sys.version_info >= (3, 3):
>> +                                       os.utime(dest, ns=(sstat.st_mtime_ns, sstat.st_mtime_ns))
>> +                               else:
>> +                                       os.utime(dest, (sstat.st_mtime, sstat.st_mtime))
> 
> It looks like you are copying mtime into both mtime and atime on the
> new file. Is that a mistake?
> 

Yeah, that's fixed in v2.
-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH v2] bin/doins.py: implement install -p option (bug 642632)

[Mike Gilbert]

On Fri, Dec 29, 2017 at 7:19 PM, Zac Medico <zmedico@gentoo.org> wrote:
> +def _set_timestamps(source_stat, dest):
> +       """Apply timestamps from source_stat to dest.
> +
> +       Args:
> +               source_stat: stat result for the source file.
> +               dest: path to the dest file.
> +       """
> +       os.utime(dest, (source_stat.st_atime, source_stat.st_mtime))
> +
> +
> +if sys.version_info >= (3, 3):
> +       def _set_timestamps_ns(source_stat, dest):
> +               os.utime(dest, ns=(source_stat.st_atime_ns, source_stat.st_mtime_ns))
> +
> +       _set_timestamps_ns.__doc__ = _set_timestamps.__doc__
> +       _set_timestamps = _set_timestamps_ns
> +
> +

This seems weirdly complex. I guess the goal was to reduce the
sys.version_info check to once per import?

The __doc__ trick is nifty, but I'm not sure I would ever want to use it myself.

Anyway, just my thoughts as an unpracticed python programmer. It looks
like this code will get the job done.

Re: [gentoo-portage-dev] [PATCH v2] bin/doins.py: implement install -p option (bug 642632)

[Zac Medico]

On 12/30/2017 01:45 PM, Mike Gilbert wrote:
> On Fri, Dec 29, 2017 at 7:19 PM, Zac Medico <zmedico@gentoo.org> wrote:
>> +def _set_timestamps(source_stat, dest):
>> +       """Apply timestamps from source_stat to dest.
>> +
>> +       Args:
>> +               source_stat: stat result for the source file.
>> +               dest: path to the dest file.
>> +       """
>> +       os.utime(dest, (source_stat.st_atime, source_stat.st_mtime))
>> +
>> +
>> +if sys.version_info >= (3, 3):
>> +       def _set_timestamps_ns(source_stat, dest):
>> +               os.utime(dest, ns=(source_stat.st_atime_ns, source_stat.st_mtime_ns))
>> +
>> +       _set_timestamps_ns.__doc__ = _set_timestamps.__doc__
>> +       _set_timestamps = _set_timestamps_ns
>> +
>> +
> 
> This seems weirdly complex. I guess the goal was to reduce the
> sys.version_info check to once per import?

Yeah, and generally it's cleanest to create compatibility shims in cases
like this.

> The __doc__ trick is nifty, but I'm not sure I would ever want to use it myself.

Yeah, the alternative of writing the docstring twice wasn't very appealing.

> Anyway, just my thoughts as an unpracticed python programmer. It looks
> like this code will get the job done.

-- 
Thanks,
Zac

Re: [gentoo-portage-dev] [PATCH v2] bin/doins.py: implement install -p option (bug 642632)

[Alec Warner]

[-- Attachment #1: Type: text/plain, Size: 6082 bytes --]

On Fri, Dec 29, 2017 at 7:19 PM, Zac Medico <zmedico@gentoo.org> wrote:

> Bug: https://bugs.gentoo.org/642632
> ---
> [PATCH v2] fix to copy atime, and split out _set_timestamps function
>
>  bin/doins.py                        | 28 +++++++++++++++++++++++++---
>  pym/portage/tests/bin/test_doins.py |  6 ++++--
>  2 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/bin/doins.py b/bin/doins.py
> index 92e450979..0d03d8fb2 100644
> --- a/bin/doins.py
> +++ b/bin/doins.py
> @@ -107,6 +107,7 @@ def _parse_install_options(
>         parser.add_argument('-g', '--group', default=-1, type=_parse_group)
>         parser.add_argument('-o', '--owner', default=-1, type=_parse_user)
>         parser.add_argument('-m', '--mode', default=0o755,
> type=_parse_mode)
> +       parser.add_argument('-p', '--preserve-timestamps',
> action='store_true')
>         split_options = shlex.split(options)
>         namespace, remaining = parser.parse_known_args(split_options)
>         # Because parsing '--mode' option is partially supported. If
> unknown
> @@ -139,6 +140,24 @@ def _set_attributes(options, path):
>                 os.chmod(path, options.mode)
>
>
> +def _set_timestamps(source_stat, dest):
> +       """Apply timestamps from source_stat to dest.
> +
> +       Args:
> +               source_stat: stat result for the source file.
> +               dest: path to the dest file.
> +       """
> +       os.utime(dest, (source_stat.st_atime, source_stat.st_mtime))
> +
> +
> +if sys.version_info >= (3, 3):
> +       def _set_timestamps_ns(source_stat, dest):
> +               os.utime(dest, ns=(source_stat.st_atime_ns,
> source_stat.st_mtime_ns))
> +
> +       _set_timestamps_ns.__doc__ = _set_timestamps.__doc__
> +       _set_timestamps = _set_timestamps_ns
> +
> +
>  class _InsInProcessInstallRunner(object):
>         """Implements `install` command behavior running in a process."""
>
> @@ -168,7 +187,8 @@ class _InsInProcessInstallRunner(object):
>                         True on success, otherwise False.
>                 """
>                 dest = os.path.join(dest_dir, os.path.basename(source))
> -               if not self._is_install_allowed(source, dest):
>

I'm going to be api picky here (so forgive me).

Previously is_install_allowed seemed to take great care with which stat was
used. But now callers have to just know to use os.stat (as opposed to
lstat, or other stats)?

Should we update the is_install_allowed comment (perhaps to say that
source_stat prefers os.stat?)

Part of me just wishes the stat was cached at a different layer (so that
code like this wasn't required just to save the syscall ;(

-A

+               sstat = os.stat(source)
> +               if not self._is_install_allowed(source, sstat, dest):
>                         return False
>
>                 # To emulate the `install` command, remove the dest file in
> @@ -187,6 +207,8 @@ class _InsInProcessInstallRunner(object):
>                                 movefile._copyxattr(
>                                         source, dest,
>                                         exclude=self._xattr_exclude)
> +                       if self._parsed_options.preserve_timestamps:
> +                               _set_timestamps(sstat, dest)
>                 except Exception:
>                         logging.exception(
>                                 'Failed to copy file: '
> @@ -195,13 +217,14 @@ class _InsInProcessInstallRunner(object):
>                         return False
>                 return True
>
> -       def _is_install_allowed(self, source, dest):
> +       def _is_install_allowed(self, source, source_stat, dest):
>                 """Returns if installing source into dest should work.
>
>                 This is to keep compatibility with the `install` command.
>
>                 Args:
>                         source: path to the source file.
> +                       source_stat: stat result for the source file.
>                         dest: path to the dest file.
>
>                 Returns:
> @@ -210,7 +233,6 @@ class _InsInProcessInstallRunner(object):
>                 # To match `install` command, use stat() for source, while
>                 # lstat() for dest. Raise an exception if stat(source)
> fails,
>                 # intentionally.
> -               source_stat = os.stat(source)
>                 try:
>                         dest_lstat = os.lstat(dest)
>                 except OSError as e:
> diff --git a/pym/portage/tests/bin/test_doins.py
> b/pym/portage/tests/bin/test_doins.py
> index 14d7adfa6..9b6c55d85 100644
> --- a/pym/portage/tests/bin/test_doins.py
> +++ b/pym/portage/tests/bin/test_doins.py
> @@ -38,13 +38,15 @@ class DoIns(setup_env.BinTestCase):
>                 self.init()
>                 try:
>                         env = setup_env.env
> -                       env['INSOPTIONS'] = '-m0644'
> +                       env['INSOPTIONS'] = '-pm0644'
>                         with open(os.path.join(env['S'], 'test'), 'w'):
>                                 pass
>                         doins('test')
>                         st = os.lstat(env['D'] + '/test')
>                         if stat.S_IMODE(st.st_mode) != 0o644:
>                                 raise tests.TestCase.failureException
> +                       if os.stat(os.path.join(env['S'],
> 'test')).st_mtime != st.st_mtime:
> +                               raise tests.TestCase.failureException
>                 finally:
>                         self.cleanup()
>
> @@ -145,7 +147,7 @@ class DoIns(setup_env.BinTestCase):
>                         env = setup_env.env
>                         # Use an option which doins.py does not know.
>                         # Then, fallback to `install` command is expected.
> -                       env['INSOPTIONS'] = '-p'
> +                       env['INSOPTIONS'] = '-b'
>                         with open(os.path.join(env['S'], 'test'), 'w'):
>                                 pass
>                         doins('test')
> --
> 2.13.6
>
>
>

[-- Attachment #2: Type: text/html, Size: 8480 bytes --]

[gentoo-portage-dev] [PATCH v3] bin/doins.py: implement install -p option (bug 642632)

[Zac Medico]

Bug: https://bugs.gentoo.org/642632
---
[PATCH v3] update _is_install_allowed docstring to specify that source_stat
    should be obtained with stat() rather than lstat()

 bin/doins.py                        | 34 +++++++++++++++++++++++++++++-----
 pym/portage/tests/bin/test_doins.py |  6 ++++--
 2 files changed, 33 insertions(+), 7 deletions(-)

diff --git a/bin/doins.py b/bin/doins.py
index 92e450979..9e6566097 100644
--- a/bin/doins.py
+++ b/bin/doins.py
@@ -107,6 +107,7 @@ def _parse_install_options(
 	parser.add_argument('-g', '--group', default=-1, type=_parse_group)
 	parser.add_argument('-o', '--owner', default=-1, type=_parse_user)
 	parser.add_argument('-m', '--mode', default=0o755, type=_parse_mode)
+	parser.add_argument('-p', '--preserve-timestamps', action='store_true')
 	split_options = shlex.split(options)
 	namespace, remaining = parser.parse_known_args(split_options)
 	# Because parsing '--mode' option is partially supported. If unknown
@@ -139,6 +140,24 @@ def _set_attributes(options, path):
 		os.chmod(path, options.mode)
 
 
+def _set_timestamps(source_stat, dest):
+	"""Apply timestamps from source_stat to dest.
+
+	Args:
+		source_stat: stat result for the source file.
+		dest: path to the dest file.
+	"""
+	os.utime(dest, (source_stat.st_atime, source_stat.st_mtime))
+
+
+if sys.version_info >= (3, 3):
+	def _set_timestamps_ns(source_stat, dest):
+		os.utime(dest, ns=(source_stat.st_atime_ns, source_stat.st_mtime_ns))
+
+	_set_timestamps_ns.__doc__ = _set_timestamps.__doc__
+	_set_timestamps = _set_timestamps_ns
+
+
 class _InsInProcessInstallRunner(object):
 	"""Implements `install` command behavior running in a process."""
 
@@ -168,7 +187,9 @@ class _InsInProcessInstallRunner(object):
 			True on success, otherwise False.
 		"""
 		dest = os.path.join(dest_dir, os.path.basename(source))
-		if not self._is_install_allowed(source, dest):
+		# Raise an exception if stat(source) fails, intentionally.
+		sstat = os.stat(source)
+		if not self._is_install_allowed(source, sstat, dest):
 			return False
 
 		# To emulate the `install` command, remove the dest file in
@@ -187,6 +208,8 @@ class _InsInProcessInstallRunner(object):
 				movefile._copyxattr(
 					source, dest,
 					exclude=self._xattr_exclude)
+			if self._parsed_options.preserve_timestamps:
+				_set_timestamps(sstat, dest)
 		except Exception:
 			logging.exception(
 				'Failed to copy file: '
@@ -195,22 +218,23 @@ class _InsInProcessInstallRunner(object):
 			return False
 		return True
 
-	def _is_install_allowed(self, source, dest):
+	def _is_install_allowed(self, source, source_stat, dest):
 		"""Returns if installing source into dest should work.
 
 		This is to keep compatibility with the `install` command.
 
 		Args:
 			source: path to the source file.
+			source_stat: stat result for the source file, using stat()
+				rather than lstat(), in order to match the `install`
+				command
 			dest: path to the dest file.
 
 		Returns:
 			True if it should succeed.
 		"""
 		# To match `install` command, use stat() for source, while
-		# lstat() for dest. Raise an exception if stat(source) fails,
-		# intentionally.
-		source_stat = os.stat(source)
+		# lstat() for dest.
 		try:
 			dest_lstat = os.lstat(dest)
 		except OSError as e:
diff --git a/pym/portage/tests/bin/test_doins.py b/pym/portage/tests/bin/test_doins.py
index 14d7adfa6..9b6c55d85 100644
--- a/pym/portage/tests/bin/test_doins.py
+++ b/pym/portage/tests/bin/test_doins.py
@@ -38,13 +38,15 @@ class DoIns(setup_env.BinTestCase):
 		self.init()
 		try:
 			env = setup_env.env
-			env['INSOPTIONS'] = '-m0644'
+			env['INSOPTIONS'] = '-pm0644'
 			with open(os.path.join(env['S'], 'test'), 'w'):
 				pass
 			doins('test')
 			st = os.lstat(env['D'] + '/test')
 			if stat.S_IMODE(st.st_mode) != 0o644:
 				raise tests.TestCase.failureException
+			if os.stat(os.path.join(env['S'], 'test')).st_mtime != st.st_mtime:
+				raise tests.TestCase.failureException
 		finally:
 			self.cleanup()
 
@@ -145,7 +147,7 @@ class DoIns(setup_env.BinTestCase):
 			env = setup_env.env
 			# Use an option which doins.py does not know.
 			# Then, fallback to `install` command is expected.
-			env['INSOPTIONS'] = '-p'
+			env['INSOPTIONS'] = '-b'
 			with open(os.path.join(env['S'], 'test'), 'w'):
 				pass
 			doins('test')
-- 
2.13.6

Re: [gentoo-portage-dev] [PATCH v2] bin/doins.py: implement install -p option (bug 642632)

[Zac Medico]

On 12/31/2017 11:55 AM, Alec Warner wrote:
> On Fri, Dec 29, 2017 at 7:19 PM, Zac Medico <zmedico@gentoo.org
> <mailto:zmedico@gentoo.org>> wrote:
> 
>     Bug: https://bugs.gentoo.org/642632
>     ---
>     [PATCH v2] fix to copy atime, and split out _set_timestamps function
> 
>      bin/doins.py                        | 28 +++++++++++++++++++++++++---
>      pym/portage/tests/bin/test_doins.py |  6 ++++--
>      2 files changed, 29 insertions(+), 5 deletions(-)
> 
>     diff --git a/bin/doins.py b/bin/doins.py
>     index 92e450979..0d03d8fb2 100644
>     --- a/bin/doins.py
>     +++ b/bin/doins.py
>     @@ -107,6 +107,7 @@ def _parse_install_options(
>             parser.add_argument('-g', '--group', default=-1,
>     type=_parse_group)
>             parser.add_argument('-o', '--owner', default=-1,
>     type=_parse_user)
>             parser.add_argument('-m', '--mode', default=0o755,
>     type=_parse_mode)
>     +       parser.add_argument('-p', '--preserve-timestamps',
>     action='store_true')
>             split_options = shlex.split(options)
>             namespace, remaining = parser.parse_known_args(split_options)
>             # Because parsing '--mode' option is partially supported. If
>     unknown
>     @@ -139,6 +140,24 @@ def _set_attributes(options, path):
>                     os.chmod(path, options.mode)
> 
> 
>     +def _set_timestamps(source_stat, dest):
>     +       """Apply timestamps from source_stat to dest.
>     +
>     +       Args:
>     +               source_stat: stat result for the source file.
>     +               dest: path to the dest file.
>     +       """
>     +       os.utime(dest, (source_stat.st_atime, source_stat.st_mtime))
>     +
>     +
>     +if sys.version_info >= (3, 3):
>     +       def _set_timestamps_ns(source_stat, dest):
>     +               os.utime(dest, ns=(source_stat.st_atime_ns,
>     source_stat.st_mtime_ns))
>     +
>     +       _set_timestamps_ns.__doc__ = _set_timestamps.__doc__
>     +       _set_timestamps = _set_timestamps_ns
>     +
>     +
>      class _InsInProcessInstallRunner(object):
>             """Implements `install` command behavior running in a
>     process."""
> 
>     @@ -168,7 +187,8 @@ class _InsInProcessInstallRunner(object):
>                             True on success, otherwise False.
>                     """
>                     dest = os.path.join(dest_dir, os.path.basename(source))
>     -               if not self._is_install_allowed(source, dest):
> 
> 
> I'm going to be api picky here (so forgive me).
> 
> Previously is_install_allowed seemed to take great care with which stat
> was used. But now callers have to just know to use os.stat (as opposed
> to lstat, or other stats)?
> 
> Should we update the is_install_allowed comment (perhaps to say that
> source_stat prefers os.stat?)

Updated the is_install_allowed docstring in v3.

> Part of me just wishes the stat was cached at a different layer (so that
> code like this wasn't required just to save the syscall ;(

The _doins function has an earlier stat call here:

    if os.path.islink(source):

We can implement more caching at some point, but it's beyond the scope
of this patch.
-- 
Thanks,
Zac