From 4bb695bcdbada9c8153442688e8494199f015f04 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sun, 26 Dec 2021 18:01:17 -0800 Subject: [PATCH 1/5] Fix copying for atomic file moves Fixes #4168. Also closes #4192, which it supersedes. The original problem is that this implementation used bytestrings incorrectly to invoke `mktemp`. However, `mktemp` is deprecated, so this PR just avoids it altogether. Fortunately, the non-deprecated APIs in `tempfile` support all-bytes arguments. --- beets/util/__init__.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/beets/util/__init__.py b/beets/util/__init__.py index d58bb28e4..9f96d4561 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -495,13 +495,20 @@ def move(path, dest, replace=False): try: os.replace(path, dest) except OSError: - tmp = tempfile.mktemp(suffix='.beets', - prefix=py3_path(b'.' + os.path.basename(dest)), - dir=py3_path(os.path.dirname(dest))) - tmp = syspath(tmp) + # Copy the file to a temporary destination. + tmp = tempfile.NamedTemporaryFile(suffix=b'.beets', + prefix=b'.' + os.path.basename(dest), + dir=os.path.dirname(dest), + delete=False) try: - shutil.copyfile(path, tmp) - os.replace(tmp, dest) + with open(path, 'rb') as f: + shutil.copyfileobj(f, tmp) + finally: + tmp.close() + + # Move the copied file into place. + try: + os.replace(tmp.name, dest) tmp = None os.remove(path) except OSError as exc: From 592c3fa3561edfa0921b37edbd1cec910e47336f Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sun, 26 Dec 2021 18:05:56 -0800 Subject: [PATCH 2/5] Changelog for #4168 fix --- docs/changelog.rst | 3 +++ 1 file changed, 3 insertions(+) diff --git a/docs/changelog.rst b/docs/changelog.rst index 3fbe5f1fc..51fbadb5e 100644 --- a/docs/changelog.rst +++ b/docs/changelog.rst @@ -28,6 +28,9 @@ Bug fixes: ``r128_album_gain`` fields was changed from integer to float to fix loss of precision due to truncation. :bug:`4169` +* Fix a regression in the previous release that caused a `TypeError` when + moving files across filesystems. + :bug:`4168` For packagers: From de3eedc033ed07be000bba446cd3c3c5dfcd282f Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Mon, 27 Dec 2021 13:51:42 -0800 Subject: [PATCH 3/5] Use bytes for destination base name This is mostly "defensive programming": clients *should* only call this on bytestring paths, but just in case this gets called on a Unicode string path, we should now not crash. --- beets/util/__init__.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/beets/util/__init__.py b/beets/util/__init__.py index 9f96d4561..8fd196359 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -496,8 +496,9 @@ def move(path, dest, replace=False): os.replace(path, dest) except OSError: # Copy the file to a temporary destination. + base = os.path.basename(bytestring_path(dest)) tmp = tempfile.NamedTemporaryFile(suffix=b'.beets', - prefix=b'.' + os.path.basename(dest), + prefix=b'.' + base, dir=os.path.dirname(dest), delete=False) try: From bb13f37e59ff4dde134f511f1999bb5b59829a6c Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Mon, 3 Jan 2022 10:16:39 -0800 Subject: [PATCH 4/5] Provide consistent types to NamedTemporaryFile --- beets/util/__init__.py | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/beets/util/__init__.py b/beets/util/__init__.py index 8fd196359..dc7edd0ff 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -486,32 +486,33 @@ def move(path, dest, replace=False): (path, dest)) if samefile(path, dest): return - path = syspath(path) - dest = syspath(dest) - if os.path.exists(dest) and not replace: + if os.path.exists(syspath(dest)) and not replace: raise FilesystemError('file exists', 'rename', (path, dest)) # First, try renaming the file. try: - os.replace(path, dest) + os.replace(syspath(path), syspath(dest)) except OSError: # Copy the file to a temporary destination. - base = os.path.basename(bytestring_path(dest)) - tmp = tempfile.NamedTemporaryFile(suffix=b'.beets', - prefix=b'.' + base, - dir=os.path.dirname(dest), - delete=False) + basename = os.path.basename(bytestring_path(dest)) + dirname = os.path.dirname(bytestring_path(dest)) + tmp = tempfile.NamedTemporaryFile( + suffix=syspath(b'.beets', prefix=False), + prefix=syspath(b'.' + basename, prefix=False), + dir=syspath(dirname), + delete=False, + ) try: - with open(path, 'rb') as f: + with open(syspath(path), 'rb') as f: shutil.copyfileobj(f, tmp) finally: tmp.close() # Move the copied file into place. try: - os.replace(tmp.name, dest) + os.replace(tmp.name, syspath(dest)) tmp = None - os.remove(path) + os.remove(syspath(path)) except OSError as exc: raise FilesystemError(exc, 'move', (path, dest), traceback.format_exc()) From 686838856a756cb1f5684adc9a385c3cedbc3f36 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Wed, 5 Jan 2022 16:15:39 -0800 Subject: [PATCH 5/5] Two more syspath calls --- beets/util/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/beets/util/__init__.py b/beets/util/__init__.py index dc7edd0ff..720ca311a 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -479,9 +479,9 @@ def move(path, dest, replace=False): instead, in which case metadata will *not* be preserved. Paths are translated to system paths. """ - if os.path.isdir(path): + if os.path.isdir(syspath(path)): raise FilesystemError(u'source is directory', 'move', (path, dest)) - if os.path.isdir(dest): + if os.path.isdir(syspath(dest)): raise FilesystemError(u'destination is directory', 'move', (path, dest)) if samefile(path, dest):