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

This is an archive of the discontinued Mercurial Phabricator instance.

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

Authored by spectral on Mar 8 2018, 6:30 PM.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

spectral created this revision.Mar 8 2018, 6:30 PM
durin42 added inline comments.
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
spectral marked 2 inline comments as done.Mar 12 2018, 5:53 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
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).

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

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

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
yuja added a subscriber: yuja.Mar 23 2018, 10:24 AM

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