tests/test-rename.t
668	You either need to do $PYTHON or do inline python, eg >>> import os >>> p="mtime/f" >>> t=1234567890 >>> open(p, "w").close() >>> os.utime(p, (t, t))
671	here too

durin42 requested changes to this revision.Mar 12 2018, 5:36 PM

This revision now requires changes to proceed.Mar 12 2018, 5:36 PM

spectral updated this revision to Diff 6964.Mar 12 2018, 5:52 PM

Neat, didn't know about the inline python stuff. That's much nicer.

durin42 requested changes to this revision.Mar 12 2018, 5:55 PM

durin42 added inline comments.

tests/test-rename.t
681	Oh, probably don't do sys.exit here. Instead just always print the boolean result.

This revision now requires changes to proceed.Mar 12 2018, 5:55 PM

spectral updated this revision to Diff 6965.Mar 12 2018, 5:57 PM

spectral marked an inline comment as done.Mar 12 2018, 5:58 PM

durin42 accepted this revision.Mar 12 2018, 5:58 PM

This revision is now accepted and ready to land.Mar 12 2018, 5:58 PM

Closed by commit rHG9e460318ca4b: copyfile: preserve stat info (mtime, etc.) when doing copies/renames (authored by spectral). · Explain WhyMar 12 2018, 6:16 PM

This revision was automatically updated to reflect the committed changes.

I'm sorry, but we cannot ship this as is.

The reason is mtime based build systems, like GNU make.

We can't have version control modifying files without bumping their mtime because this invalidates the target freshness checks of mtime-based build systems.

If we want to preserve mtime on file copy or move, I believe it is safe to do that if and only if the destination file didn't already exist. But if the destination exists, we need to ensure the mtime of the new file is greater than the mtime of the old file.

Reading this patch, I /think/ the previous behavior was buggy in edge cases because we never ensured the mtime of the replacement was newer than the existing file. In 99.99% of cases, it will be because the existing file was created sometime in the past. But if bad clocks or other wonky things are in play, there's no guarantee that *wall clock now* is greater than the mtime of the existing destination file. The correct thing to do in this situation is read the mtime of the existing file and ensure the mtime of the new file is at least 1s greater than the previous mtime (1s because not all filesystems preserve microsecond or millisecond mtime granularity).

In D2729#45664, @indygreg wrote:

I'm sorry, but we cannot ship this as is.
The reason is mtime based build systems, like GNU make.
We can't have version control modifying files without bumping their mtime because this invalidates the target freshness checks of mtime-based build systems.

If the user does an mv in the shell, at least on Linux, it preserves mtime. If they do a cp, it doesn't (the file gets the current timestamp). This includes when overwriting a file. I think I'd be fine with mimicking this behavior (only preserve mtime on hg mv) if that would make this safer or easier to reason about.

If we want to preserve mtime on file copy or move, I believe it is safe to do that if and only if the destination file didn't already exist. But if the destination exists, we need to ensure the mtime of the new file is greater than the mtime of the old file.

hg mv and hg cp require --after if the destination file already exists; in those cases, we don't seem to touch the working directory at all (including not modifying the mtime, even with my patch). With --after, this is purely a VCS operation that afaict "shouldn't" have any effect on build systems, so I think we're safe here for that concern?

Reading this patch, I /think/ the previous behavior was buggy in edge cases because we never ensured the mtime of the replacement was newer than the existing file. In 99.99% of cases, it will be because the existing file was created sometime in the past. But if bad clocks or other wonky things are in play, there's no guarantee that *wall clock now* is greater than the mtime of the existing destination file. The correct thing to do in this situation is read the mtime of the existing file and ensure the mtime of the new file is at least 1s greater than the previous mtime (1s because not all filesystems preserve microsecond or millisecond mtime granularity).

In D2729#45787, @spectral wrote:

In D2729#45664, @indygreg wrote:

I'm sorry, but we cannot ship this as is.
The reason is mtime based build systems, like GNU make.
We can't have version control modifying files without bumping their mtime because this invalidates the target freshness checks of mtime-based build systems.

If the user does an mv in the shell, at least on Linux, it preserves mtime. If they do a cp, it doesn't (the file gets the current timestamp). This includes when overwriting a file. I think I'd be fine with mimicking this behavior (only preserve mtime on hg mv) if that would make this safer or easier to reason about.

Greg has some concerns about this patch and it sounds like you will make some changes. The patch was queued by Augie, but I'm dropping it for now, so we don't have a slightly controversial patch written by a Googler queued by two other Googlers.

spectral updated this revision to Diff 7190.Mar 21 2018, 4:59 PM

In D2729#46372, @martinvonz wrote:

Greg has some concerns about this patch and it sounds like you will make some changes. The patch was queued by Augie, but I'm dropping it for now, so we don't have a slightly controversial patch written by a Googler queued by two other Googlers.

Made it only do this for hg cp. Not sure if I need to reopen this somehow, or if that happens automatically..

spectral updated this revision to Diff 7191.Mar 21 2018, 5:01 PM

Queued new version, thanks.

FWIW, phabup complains that "You can not accept this revision because it
has already been closed. Only open revisions can be accepted."

			Path	Packages
M			hgext/largefiles/overrides.py (4 lines)
M			mercurial/cmdutil.py (4 lines)
M			tests/test-rename.t (33 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	6735		Mar 8 2018, 6:30 PM	★	★
Diff 2	6964		Mar 12 2018, 5:52 PM	★	★
Diff 3	6965		Mar 12 2018, 5:57 PM	★	★
Diff 4	6967	rHG9e460318ca4b7bf8558a9e830b69c7142904dd34	Mar 8 2018, 2:44 PM	★	★
Diff 5	7190		Mar 21 2018, 4:59 PM	★	★
Diff 6	7191		Mar 21 2018, 5:01 PM	★	★

	if matchmod.patkind(pat) is not None:			if matchmod.patkind(pat) is not None:
	listpats.append(pat)			listpats.append(pat)
	else:			else:
	listpats.append(makestandin(pat))			listpats.append(makestandin(pat))

	try:			try:
	origcopyfile = util.copyfile			origcopyfile = util.copyfile
	copiedfiles = []			copiedfiles = []
	def overridecopyfile(src, dest):			def overridecopyfile(src, dest, args, *kwargs):
	if (lfutil.shortname in src and			if (lfutil.shortname in src and
	dest.startswith(repo.wjoin(lfutil.shortname))):			dest.startswith(repo.wjoin(lfutil.shortname))):
	destlfile = dest.replace(lfutil.shortname, '')			destlfile = dest.replace(lfutil.shortname, '')
	if not opts['force'] and os.path.exists(destlfile):			if not opts['force'] and os.path.exists(destlfile):
	raise IOError('',			raise IOError('',
	_('destination largefile already exists'))			_('destination largefile already exists'))
	copiedfiles.append((src, dest))			copiedfiles.append((src, dest))
	origcopyfile(src, dest)			origcopyfile(src, dest, args, *kwargs)

	util.copyfile = overridecopyfile			util.copyfile = overridecopyfile
	result += orig(ui, repo, listpats, opts, rename)			result += orig(ui, repo, listpats, opts, rename)
	finally:			finally:
	util.copyfile = origcopyfile			util.copyfile = origcopyfile

	lfdirstate = lfutil.openlfdirstate(ui, repo)			lfdirstate = lfutil.openlfdirstate(ui, repo)
	for (src, dest) in copiedfiles:			for (src, dest) in copiedfiles:

	targetdir = os.path.dirname(target) or '.'			targetdir = os.path.dirname(target) or '.'
	if not os.path.isdir(targetdir):			if not os.path.isdir(targetdir):
	os.makedirs(targetdir)			os.makedirs(targetdir)
	if samefile:			if samefile:
	tmp = target + "~hgrename"			tmp = target + "~hgrename"
	os.rename(src, tmp)			os.rename(src, tmp)
	os.rename(tmp, target)			os.rename(tmp, target)
	else:			else:
	util.copyfile(src, target)			# Preserve stat info on renames, not on copies; this matches
				# Linux CLI behavior.
				util.copyfile(src, target, copystat=rename)
	srcexists = True			srcexists = True
	except IOError as inst:			except IOError as inst:
	if inst.errno == errno.ENOENT:			if inst.errno == errno.ENOENT:
	ui.warn(_('%s: deleted in working directory\n') % relsrc)			ui.warn(_('%s: deleted in working directory\n') % relsrc)
	srcexists = False			srcexists = False
	else:			else:
	ui.warn(_('%s: cannot copy - %s\n') %			ui.warn(_('%s: cannot copy - %s\n') %
	(relsrc, encoding.strtolocal(inst.strerror)))			(relsrc, encoding.strtolocal(inst.strerror)))

	abort: path contains illegal component: .hg/foo			abort: path contains illegal component: .hg/foo
	[255]			[255]
	$ hg status -C			$ hg status -C
	$ (cd d1/d11; hg rename ../../d2/b ../../../foo)			$ (cd d1/d11; hg rename ../../d2/b ../../../foo)
	abort: ../../../foo not under root '$TESTTMP'			abort: ../../../foo not under root '$TESTTMP'
	[255]			[255]
	$ hg status -C			$ hg status -C

				check that stat information such as mtime is preserved on rename - it's unclear
				whether the `touch` and `stat` commands are portable, so we mimic them using
				python. Not all platforms support precision of even one-second granularity, so
				we allow a rather generous fudge factor here; 1234567890 is 2009, and the
				primary thing we care about is that it's not the machine's current time;
				hopefully it's really unlikely for a machine to have such a broken clock that
				this test fails. :)

				$ mkdir mtime
				durin42Unsubmitted Done You either need to do $PYTHON or do inline python, eg >>> import os >>> p="mtime/f" >>> t=1234567890 >>> open(p, "w").close() >>> os.utime(p, (t, t)) durin42: You either need to do $PYTHON or do inline python, eg ``` >>> import os >>> p="mtime/f"…
				Create the file (as empty), then update its mtime and atime to be 1234567890.
				>>> import os
				>>> filename = "mtime/f"
				durin42Unsubmitted Done here too durin42: here too
				>>> mtime = 1234567890
				>>> open(filename, "w").close()
				>>> os.utime(filename, (mtime, mtime))
				$ hg ci -qAm 'add mtime dir'
				"hg cp" does not preserve the mtime, so it should be newer than the 2009
				timestamp.
				$ hg cp -q mtime mtime_cp
				>>> from __future__ import print_function
				>>> import os
				>>> filename = "mtime_cp/f"
				durin42Unsubmitted Done Oh, probably don't do sys.exit here. Instead just always print the boolean result. durin42: Oh, probably don't do sys.exit here. Instead just always print the boolean result.
				>>> print(os.stat(filename).st_mtime < 1234567999)
				False
				"hg mv" preserves the mtime, so it should be ~equal to the 2009 timestamp
				(modulo some fudge factor due to not every system supporting 1s-level
				precision).
				$ hg mv -q mtime mtime_mv
				>>> from __future__ import print_function
				>>> import os
				>>> filename = "mtime_mv/f"
				>>> print(os.stat(filename).st_mtime < 1234567999)
				True

This is an archive of the discontinued Mercurial Phabricator instance.

copyfile: preserve stat info (mtime, etc.) when doing copies/renames
ClosedPublic

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 7191

hgext/largefiles/overrides.py

mercurial/cmdutil.py

tests/test-rename.t