mv: Fix regression

Moved Package.todo handling in one place.
Fixed a situation when path to a file that doesn't exist is passed to Package

behave/features/mv.feature

@@ -0,0 +1,21 @@
+Feature: `osc mv` command
+
+
+# common steps for all scenarios
+Background:
+   Given I set working directory to "{context.osc.temp}"
+     And I execute osc with args "checkout test:factory/test-pkgA"
+     And I set working directory to "{context.osc.temp}/test:factory/test-pkgA"
+
+
+Scenario: Run `osc mv <file> <new-name>` in a package checkout
+    When I execute osc with args "mv test-pkgA.changes new-name.changes"
+    Then the exit code is 0
+     And I execute osc with args "status"
+     And stdout is
+     """
+     A    new-name.changes
+     D    test-pkgA.changes
+     """
+     And file "{context.osc.temp}/test:factory/test-pkgA/test-pkgA.changes" does not exist
+     And file "{context.osc.temp}/test:factory/test-pkgA/new-name.changes" exists

osc/commandline.py

@@ -9159,11 +9159,11 @@ Please submit there instead, or use --nodevelproject to force direct submission.
 
         os.rename(source, dest)
         try:
-            tgt_pkg[0].addfile(os.path.basename(dest))
+            tgt_pkg.addfile(os.path.basename(dest))
         except oscerr.PackageFileConflict:
             # file is already tracked
             pass
-        src_pkg[0].delete_file(os.path.basename(source), force=opts.force)
+        src_pkg.delete_file(os.path.basename(source), force=opts.force)
 
     @cmdln.option('-d', '--delete', action='store_true',
                         help='delete option from config or reset option to the default)')

osc/core.py

@@ -1224,7 +1224,14 @@ class Package:
     def __init__(self, workingdir, progress_obj=None, size_limit=None, wc_check=True):
         global store
 
-        self.dir = workingdir
+        self.todo = []
+        if os.path.isfile(workingdir) or not os.path.exists(workingdir):
+            # workingdir is a file
+            # workingdir doesn't exist -> it points to a non-existing file in a working dir (e.g. during mv)
+            workingdir, todo_entry = os.path.split(workingdir)
+            self.todo.append(todo_entry)
+
+        self.dir = workingdir or "."
         self.absdir = os.path.abspath(self.dir)
         self.store = Store(self.dir)
         self.storedir = os.path.join(self.absdir, store)
@@ -1251,8 +1258,6 @@ class Package:
                 'of the working copy afterwards (via \'osc status %s\')' % (self.dir, self.dir, self.dir)
             raise oscerr.WorkingCopyInconsistent(self.prjname, self.name, dirty_files, msg)
 
-        self.todo = []
-
     def __repr__(self):
         return super().__repr__() + f"({self.prjname}/{self.name})"
 
@@ -1272,28 +1277,23 @@ class Package:
         """
         packages = []
         for path in paths:
-            # TODO: match only dirs, remove the code for resolving files into Package objects
-            orig_path = path
-            path_is_file = os.path.isfile(path)
-            if path_is_file:
-                path = os.path.dirname(path) or "."
-
             package = cls(path, progress_obj)
+            seen_package = None
             try:
                 # re-use an existing package
                 seen_package_index = packages.index(package)
-                package = packages[seen_package_index]
-                if os.path.abspath(path) != package.absdir:
-                    raise oscerr.PackageExists(package.prjname, package.name, "Duplicate package")
+                seen_package = packages[seen_package_index]
             except ValueError:
-                # use a new package instance
-                packages.append(package)
+                pass
 
-            if path_is_file:
-                # XXX: modifying 'todo' is an unexpected side-effect
-                todo_entry = os.path.basename(orig_path)
-                if todo_entry not in package.todo:
-                    package.todo.append(todo_entry)
+            if seen_package:
+                # merge package into seen_package
+                if seen_package.absdir != package.absdir:
+                    raise oscerr.PackageExists(package.prjname, package.name, "Duplicate package")
+                seen_package.merge(package)
+            else:
+                # use the new package instance
+                packages.append(package)
 
         return packages
 
@@ -1306,21 +1306,30 @@ class Package:
         packages = []
         failed_to_load = []
         for path in paths:
-            # TODO: match only dirs, remove the code for resolving files into Package objects
-            orig_path = path
-            path_is_file = os.path.isfile(path)
-            if path_is_file:
-                path = os.path.dirname(path) or "."
             try:
                 package = cls(path, progress_obj)
-                if path_is_file:
-                    # XXX: modifying 'todo' is an unexpected side-effect
-                    package.todo = [os.path.basename(orig_path)]
-                if package in packages:
-                    raise oscerr.PackageExists(package.prjname, package.name, "Duplicate package")
-                packages.append(package)
             except oscerr.NoWorkingCopy:
-                failed_to_load.append(orig_path)
+                failed_to_load.append(path)
+                continue
+
+            # the following code is identical to from_paths()
+            seen_package = None
+            try:
+                # re-use an existing package
+                seen_package_index = packages.index(package)
+                seen_package = packages[seen_package_index]
+            except ValueError:
+                pass
+
+            if seen_package:
+                # merge package into seen_package
+                if seen_package.absdir != package.absdir:
+                    raise oscerr.PackageExists(package.prjname, package.name, "Duplicate package")
+                seen_package.merge(package)
+            else:
+                # use the new package instance
+                packages.append(package)
+
         return packages, failed_to_load
 
     def wc_check(self):
@@ -2260,7 +2269,9 @@ class Package:
             yield diff_add_delete(f, False, revision)
 
     def merge(self, otherpac):
-        self.todo += otherpac.todo
+        for todo_entry in otherpac.todo:
+            if todo_entry not in self.todo:
+                self.todo.append(todo_entry)
 
     def __str__(self):
         r = """

tests/test_core_package.py

@@ -69,6 +69,36 @@ class TestPackageFromPaths(OscTestCase):
     def _get_fixtures_dir(self):
         return FIXTURES_DIR
 
+    def test_package_object_dir(self):
+        path = "projectA/pkgA"
+        path = os.path.join(self.tmpdir, 'osctest', path)
+        pac = osc.core.Package(path)
+
+        self.assertEqual(pac.name, "pkgA")
+        self.assertEqual(pac.prjname, "projectA")
+        self.assertEqual(pac.apiurl, "http://localhost")
+        self.assertEqual(pac.todo, [])
+
+    def test_package_object_file(self):
+        path = "projectA/pkgA/pkgA.spec"
+        path = os.path.join(self.tmpdir, 'osctest', path)
+        pac = osc.core.Package(path)
+
+        self.assertEqual(pac.name, "pkgA")
+        self.assertEqual(pac.prjname, "projectA")
+        self.assertEqual(pac.apiurl, "http://localhost")
+        self.assertEqual(pac.todo, ["pkgA.spec"])
+
+    def test_package_object_file_missing(self):
+        path = "projectA/pkgA/missing-file"
+        path = os.path.join(self.tmpdir, 'osctest', path)
+        pac = osc.core.Package(path)
+
+        self.assertEqual(pac.name, "pkgA")
+        self.assertEqual(pac.prjname, "projectA")
+        self.assertEqual(pac.apiurl, "http://localhost")
+        self.assertEqual(pac.todo, ["missing-file"])
+
     def test_single_package(self):
         paths = ["projectA/pkgA"]
         paths = [os.path.join(self.tmpdir, 'osctest', i) for i in paths]