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
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

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 33420

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: