This is an archive of the discontinued Mercurial Phabricator instance.

Differential D12623

clone: use better names for temp files
ClosedPublic

Authored by aalekseyev on Tue, May 10, 3:30 PM.

Details

Reviewers
marmoute
Alphare
Group Reviewers
hg-reviewers
Commits
rHG4ff4e23de7df: clone: use better names for temp files
Summary

Before this commit, the file names are /tmp/tmpn8smvlr8
After this commit, they are more like /tmp/hg-clone-n8smvlr8/00manifest.ndb3qj52v6,
which makes it much clearer what these files correspond to.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

aalekseyev created this revision.Tue, May 10, 3:30 PM
marmoute requested changes to this revision.Tue, May 17, 11:12 AM
marmoute added a subscriber: marmoute.

There seems to be three patches in one here :

  1. adding a tryrmdir as a last line of defence
  2. using prefix= in mkstemp`
  3. a comment about hardlink=True (that could probably we a fix to the issue here).

Can you split that patch into three patches?

This revision now requires changes to proceed.Tue, May 17, 11:12 AM
aalekseyev added a comment.Tue, May 17, 11:30 AM

I'll remove a hardlink=True comment (which is not really worthy of a phabsend), but I don't understand the purpose of the split between (1) and (2). The only reason we're removing a directory is that we're creating it and putting temp files there.
Actually, a commit message could be improved...

aalekseyev edited the summary of this revision. (Show Details)Tue, May 17, 1:28 PM
aalekseyev updated this revision to Diff 33404.
marmoute accepted this revision.Thu, May 19, 4:57 AM
In D12623#193297, @aalekseyev wrote:

I'll remove a hardlink=True comment (which is not really worthy of a phabsend), but I don't understand the purpose of the split between (1) and (2). The only reason we're removing a directory is that we're creating it and putting temp files there.
Actually, a commit message could be improved...

Ho right, my mistake.

Alphare accepted this revision.Tue, May 24, 8:47 AM
This revision is now accepted and ready to land.Tue, May 24, 8:47 AM
baymax added a parent revision: D12620: tests: make sure .js files stay in ASCII encoding (issue6559).Tue, May 24, 9:35 AM
baymax updated this revision to Diff 33420.

✅ refresh by Heptapod after a successful CI run (🐙 💚)

aalekseyev added a commit: rHG4ff4e23de7df: clone: use better names for temp files.Tue, May 24, 9:43 AM
Closed by commit rHG4ff4e23de7df: clone: use better names for temp files (authored by aalekseyev). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/streamclone.py (7 lines)
Mmercurial/util.py (8 lines)

Diff 33422

mercurial/streamclone.py

if ftype != _filefull: if ftype != _filefull:
return entry return entry
return (src, name, ftype, copy(vfsmap[src].join(name))) return (src, name, ftype, copy(vfsmap[src].join(name)))
@contextlib.contextmanager @contextlib.contextmanager
def maketempcopies(): def maketempcopies():
"""return a function to temporary copy file""" """return a function to temporary copy file"""
files = [] files = []
dst_dir = pycompat.mkdtemp(prefix=b'hg-clone-')
try: try:
def copy(src): def copy(src):
fd, dst = pycompat.mkstemp() fd, dst = pycompat.mkstemp(
prefix=os.path.basename(src), dir=dst_dir
)
os.close(fd) os.close(fd)
files.append(dst) files.append(dst)
util.copyfiles(src, dst, hardlink=True) util.copyfiles(src, dst, hardlink=True)
return dst return dst
yield copy yield copy
finally: finally:
for tmp in files: for tmp in files:
util.tryunlink(tmp) util.tryunlink(tmp)
util.tryrmdir(dst_dir)
def _makemap(repo): def _makemap(repo):
"""make a (src -> vfs) map for the repo""" """make a (src -> vfs) map for the repo"""
vfsmap = { vfsmap = {
_srcstore: repo.svfs, _srcstore: repo.svfs,
_srccache: repo.cachevfs, _srccache: repo.cachevfs,
} }

mercurial/util.py

def __exit__(self, exctype, excvalue, traceback): def __exit__(self, exctype, excvalue, traceback):
if exctype is not None: if exctype is not None:
self.discard() self.discard()
else: else:
self.close() self.close()
def tryrmdir(f):
try:
removedirs(f)
except OSError as e:
if e.errno != errno.ENOENT and e.errno != errno.ENOTEMPTY:
raise
def unlinkpath(f, ignoremissing=False, rmdir=True): def unlinkpath(f, ignoremissing=False, rmdir=True):
# type: (bytes, bool, bool) -> None # type: (bytes, bool, bool) -> None
"""unlink and remove the directory if it is empty""" """unlink and remove the directory if it is empty"""
if ignoremissing: if ignoremissing:
tryunlink(f) tryunlink(f)
else: else:
unlink(f) unlink(f)
if rmdir: if rmdir: