From 03c3be7125154a21241fe9fe3d74a7395cae8ad5 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Jul 26 2016 20:20:54 +0000
Subject: [PATCH 1/5] stub


---

diff --git a/koji/util.py b/koji/util.py
index 80e511f..9a73c82 100644
--- a/koji/util.py
+++ b/koji/util.py
@@ -26,6 +26,7 @@ import os
 import os.path
 import re
 import resource
+import shutil
 import stat
 import sys
 import time
@@ -316,6 +317,13 @@ def rmtree(path):
             continue
         os.rmdir(dirpath)
 
+
+def safe_move(src, dst):
+    """Wrapper around shutil.move with additional safety"""
+
+    pass
+
+
 def _relpath(path, start=getattr(os.path, 'curdir', '.')):
     """Backport of os.path.relpath for python<2.6"""
 

From 720c6479594a9a703db1de6c3bc2fafa3d20775f Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Jul 26 2016 20:20:54 +0000
Subject: [PATCH 2/5] just wrap shutil.move for now


---

diff --git a/koji/util.py b/koji/util.py
index 9a73c82..461ccce 100644
--- a/koji/util.py
+++ b/koji/util.py
@@ -318,10 +318,20 @@ def rmtree(path):
         os.rmdir(dirpath)
 
 
-def safe_move(src, dst):
-    """Wrapper around shutil.move with additional safety"""
+def safer_move(src, dst):
+    """Rename if possible, copy+rm otherwise
 
-    pass
+    Behavior is similar to shutil.move
+
+    Unlike move, src is /always/ moved from src to dst. If dst is an existing
+    directory, then an error is raised.
+    """
+    if os.path.exists(dst):
+        raise koji.GenericError, "Destination exists: %s" % dst
+    elif os.path.islink(dst):
+        raise koji.GenericError, "Destination is a symlink: %s" % dst
+    # TODO - use locking to do a better job of catching races
+    shutil.move(src, dst)
 
 
 def _relpath(path, start=getattr(os.path, 'curdir', '.')):

From 8bd4b49822f01fecdef653ce14e8cbb98cc9244a Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Jul 26 2016 20:20:54 +0000
Subject: [PATCH 3/5] use safer_move on hub


---

diff --git a/hub/kojihub.py b/hub/kojihub.py
index c37c445..1e9d759 100644
--- a/hub/kojihub.py
+++ b/hub/kojihub.py
@@ -41,6 +41,7 @@ from koji.util import md5_constructor
 from koji.util import sha1_constructor
 from koji.util import dslice
 from koji.util import multi_fnmatch
+from koji.util import safer_move
 import os
 import re
 import rpm
@@ -5147,7 +5148,7 @@ def import_build_log(fn, buildinfo, subdir=None):
         raise koji.GenericError("Error importing build log. %s already exists." % final_path)
     if os.path.islink(fn) or not os.path.isfile(fn):
         raise koji.GenericError("Error importing build log. %s is not a regular file." % fn)
-    os.rename(fn,final_path)
+    safer_move(fn, final_path)
     os.symlink(final_path,fn)
 
 def import_rpm_file(fn,buildinfo,rpminfo):
@@ -5603,7 +5604,7 @@ def import_old_image(old, name, version):
             raise koji.GenericError("Error importing build log. %s already exists." % final_path)
         if os.path.islink(logsrc) or not os.path.isfile(logsrc):
             raise koji.GenericError("Error importing build log. %s is not a regular file." % logsrc)
-        os.rename(logsrc, final_path)
+        safer_move(logsrc, final_path)
         os.symlink(final_path, logsrc)
 
     return binfo
@@ -5774,7 +5775,7 @@ def _import_archive_file(filepath, destdir):
     if os.path.islink(filepath) or not os.path.isfile(filepath):
         raise koji.GenericError("Error importing archive file, %s is not a regular file" % filepath)
     koji.ensuredir(destdir)
-    os.rename(filepath, final_path)
+    safer_move(filepath, final_path)
     os.symlink(final_path, filepath)
 
 def _generate_maven_metadata(mavendir):
@@ -7891,7 +7892,7 @@ def importImageInternal(task_id, build_id, imgdata):
             raise koji.GenericError("Error importing build log. %s already exists." % final_path)
         if os.path.islink(logsrc) or not os.path.isfile(logsrc):
             raise koji.GenericError("Error importing build log. %s is not a regular file." % logsrc)
-        os.rename(logsrc, final_path)
+        safer_move(logsrc, final_path)
         os.symlink(final_path, logsrc)
 
     # record all of the RPMs installed in the image(s)
@@ -10977,7 +10978,7 @@ class HostExports(object):
         for relpath in [srpm] + rpms:
             fn = "%s/%s" % (uploadpath,relpath)
             dest = "%s/%s" % (dir,os.path.basename(fn))
-            os.rename(fn,dest)
+            safer_move(fn, dest)
             os.symlink(dest,fn)
         if logs:
             for key, files in logs.iteritems():
@@ -10989,7 +10990,7 @@ class HostExports(object):
                 for relpath in files:
                     fn = "%s/%s" % (uploadpath,relpath)
                     dest = "%s/%s" % (logdir,os.path.basename(fn))
-                    os.rename(fn,dest)
+                    safer_move(fn, dest)
                     os.symlink(dest,fn)
 
     def moveMavenBuildToScratch(self, task_id, results, rpm_results):
@@ -11012,7 +11013,7 @@ class HostExports(object):
                 src = os.path.join(koji.pathinfo.task(results['task_id']), relpath)
                 dest = os.path.join(destdir, relpath)
                 koji.ensuredir(os.path.dirname(dest))
-                os.rename(src, dest)
+                safer_move(src, dest)
                 os.symlink(dest, src)
         if rpm_results:
             for relpath in [rpm_results['srpm']] + rpm_results['rpms'] + \
@@ -11021,7 +11022,7 @@ class HostExports(object):
                                    relpath)
                 dest = os.path.join(destdir, 'rpms', relpath)
                 koji.ensuredir(os.path.dirname(dest))
-                os.rename(src, dest)
+                safer_move(src, dest)
                 os.symlink(dest, src)
 
     def moveWinBuildToScratch(self, task_id, results, rpm_results):
@@ -11039,7 +11040,7 @@ class HostExports(object):
             filename = os.path.join(koji.pathinfo.task(results['task_id']), relpath)
             dest = os.path.join(destdir, relpath)
             koji.ensuredir(os.path.dirname(dest))
-            os.rename(filename, dest)
+            safer_move(filename, dest)
             os.symlink(dest, filename)
         if rpm_results:
             for relpath in [rpm_results['srpm']] + rpm_results['rpms'] + \
@@ -11048,7 +11049,7 @@ class HostExports(object):
                                         relpath)
                 dest = os.path.join(destdir, 'rpms', relpath)
                 koji.ensuredir(os.path.dirname(dest))
-                os.rename(filename, dest)
+                safer_move(filename, dest)
                 os.symlink(dest, filename)
 
     def moveImageBuildToScratch(self, task_id, results):
@@ -11069,7 +11070,7 @@ class HostExports(object):
                 dest = os.path.join(destdir, img)
                 koji.ensuredir(destdir)
                 logger.debug('renaming %s to %s' % (src, dest))
-                os.rename(src, dest)
+                safer_move(src, dest)
                 os.symlink(dest, src)
             if sub_results.has_key('rpmresults'):
                 rpm_results = sub_results['rpmresults']
@@ -11079,7 +11080,7 @@ class HostExports(object):
                         rpm_results['task_id']), relpath)
                     dest = os.path.join(destdir, 'rpms', relpath)
                     koji.ensuredir(os.path.dirname(dest))
-                    os.rename(src, dest)
+                    safer_move(src, dest)
                     os.symlink(dest, src)
 
     def initBuild(self,data):
@@ -11694,7 +11695,7 @@ class HostExports(object):
                     raise koji.GenericError, "File already in repo: %s" % dst
                 #otherwise the desired hardlink already exists
             else:
-                os.link(filepath, dst)
+                safer_move(filepath, dst)
 
     def repoDone(self, repo_id, data, expire=False):
         """Move repo data into place, mark as ready, and expire earlier repos
@@ -11724,7 +11725,7 @@ class HostExports(object):
                 dst = "%s/%s" % (datadir, fn)
                 if not os.path.exists(src):
                     raise koji.GenericError, "uploaded file missing: %s" % src
-                os.link(src, dst)
+                safer_move(src, dst)
                 os.unlink(src)
         if expire:
             repo_expire(repo_id)

From 0e7c45a544aaacb78b6dc07c310b27cf41c51951 Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Jul 26 2016 20:20:54 +0000
Subject: [PATCH 4/5] update raise syntax in safer_move()


---

diff --git a/koji/util.py b/koji/util.py
index 461ccce..7903979 100644
--- a/koji/util.py
+++ b/koji/util.py
@@ -327,9 +327,9 @@ def safer_move(src, dst):
     directory, then an error is raised.
     """
     if os.path.exists(dst):
-        raise koji.GenericError, "Destination exists: %s" % dst
+        raise koji.GenericError("Destination exists: %s" % dst)
     elif os.path.islink(dst):
-        raise koji.GenericError, "Destination is a symlink: %s" % dst
+        raise koji.GenericError("Destination is a symlink: %s" % dst)
     # TODO - use locking to do a better job of catching races
     shutil.move(src, dst)
 

From 952891ea2f3da28635eeacf45fc6d2f8384434ce Mon Sep 17 00:00:00 2001
From: Mike McLean <mikem@redhat.com>
Date: Jul 26 2016 20:20:54 +0000
Subject: [PATCH 5/5] unit test for safer_move()


---

diff --git a/tests/test_utils.py b/tests/test_utils.py
index 6be5326..9f5b604 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -1,6 +1,8 @@
+import mock
 import unittest
 
 import koji
+import koji.util
 
 
 class EnumTestCase(unittest.TestCase):
@@ -31,3 +33,46 @@ class EnumTestCase(unittest.TestCase):
         """ Test slice access. """
         test = koji.Enum(('one', 'two', 'three'))
         self.assertEquals(test[1:], ('two', 'three'))
+
+
+class MiscFunctionTestCase(unittest.TestCase):
+
+    @mock.patch('os.path.exists')
+    @mock.patch('os.path.islink')
+    @mock.patch('shutil.move')
+    def test_safer_move(self, move, islink, exists):
+        """Test safer_move function"""
+        src = '/FAKEPATH/SRC'
+        dst = '/FAKEPATH/DST'
+
+        # good args
+        exists.return_value = False
+        islink.return_value = False
+        koji.util.safer_move(src, dst)
+        exists.assert_called_once_with(dst)
+        islink.assert_called_once_with(dst)
+        move.assert_called_once_with(src, dst)
+
+        move.reset_mock()
+        islink.reset_mock()
+        exists.reset_mock()
+
+        # existing dst
+        exists.return_value = True
+        with self.assertRaises(koji.GenericError):
+            koji.util.safer_move(src, dst)
+        exists.assert_called_once_with(dst)
+        move.assert_not_called()
+
+        move.reset_mock()
+        islink.reset_mock()
+        exists.reset_mock()
+
+        # symlink dst
+        exists.return_value = False
+        islink.return_value = True
+        with self.assertRaises(koji.GenericError):
+            koji.util.safer_move(src, dst)
+        exists.assert_called_once_with(dst)
+        islink.assert_called_once_with(dst)
+        move.assert_not_called()

