Fix #285: clean command, remove targets in reverse lexical order

Fix #285 by removing targets in reverse lexical order

CHANGES

@@ -20,7 +20,8 @@ Changes
  - Make it possible to use a custom encoder when using config_changed with a dict.
  - Add configuration `DOIT_CONFIG` `action_string_formatting` to control command action formatter.
  - Fix `result_dep`, use result **after** its execution
- - Fix #286: Cannot use functools.partial as title
+ - Fix #286: Support `functools.partial` on tasks' dict metadata `task.title`
+ - Fix #285: `clean` command, remove targets in reverse DFS order
 
 
 0.31.1 (*2018-03-18*)

doc/cmd_other.rst

@@ -121,6 +121,12 @@ include a *clean* attribute. This attribute can be ``True`` to remove all of its
 target files. If there is a folder as a target it will be removed if the folder
 is empty, otherwise it will display a warning message.
 
+.. note::
+
+  The targets' removal order will be the reverse of their lexical ordering.
+  This ensures that the directory structure formed by the targets is correctly
+  removed irrespective of their order in the ``targets`` array.
+
 The *clean* attribute can be a list of actions. An action could be a
 string with a shell command or a tuple with a python callable.
 

doit/task.py

@@ -573,26 +573,20 @@ def dict_to_task(task_dict):
 
 def clean_targets(task, dryrun):
     """remove all targets from a task"""
-    files = [path for path in task.targets if os.path.isfile(path)]
-    dirs = [path for path in task.targets if os.path.isdir(path)]
-
-    # remove all files
-    for file_ in files:
-        print("%s - removing file '%s'" % (task.name, file_))
-        if not dryrun:
-            os.remove(file_)
-
-    # remove all directories (if empty)
-    for dir_ in dirs:
-        if os.listdir(dir_):
-            msg = "%s - cannot remove (it is not empty) '%s'"
-            print(msg % (task.name, dir_))
-        else:
-            msg = "%s - removing dir '%s'"
-            print(msg % (task.name, dir_))
+    for target in sorted(task.targets, reverse=True):
+        if os.path.isfile(target):
+            print("%s - removing file '%s'" % (task.name, target))
             if not dryrun:
-                os.rmdir(dir_)
-
+                os.remove(target)
+        elif os.path.isdir(target):
+            if os.listdir(target):
+                msg = "%s - cannot remove (it is not empty) '%s'"
+                print(msg % (task.name, target))
+            else:
+                msg = "%s - removing dir '%s'"
+                print(msg % (task.name, target))
+                if not dryrun:
+                    os.rmdir(target)
 
 
 # uptodate

tests/test_task.py

@@ -374,8 +374,11 @@ class TestTaskClean(object):
     def tmpdir(self, request):
         tmpdir = {}
         tmpdir['dir'] = tempfile.mkdtemp(prefix='doit-')
+        tmpdir['subdir'] = tempfile.mkdtemp(dir=tmpdir['dir'])
         files = [os.path.join(tmpdir['dir'], fname)
-                 for fname in ['a.txt', 'b.txt']]
+                 for fname in ['a.txt',
+                               'b.txt',
+                               os.path.join(tmpdir['subdir'], 'c.txt')]]
         tmpdir['files'] = files
         # create empty files
         for filename in tmpdir['files']:
@@ -412,14 +415,15 @@ def test_clean_non_existent_targets(self):
 
     def test_clean_empty_dirs(self, tmpdir):
         # Remove empty directories listed in targets
-        targets = tmpdir['files'] + [tmpdir['dir']]
+        targets = tmpdir['files'] + [tmpdir['subdir']]
         t = task.Task("xxx", None, targets=targets, clean=True)
         assert True == t._remove_targets
         assert 0 == len(t.clean_actions)
         t.clean(StringIO(), False)
         for filename in tmpdir['files']:
             assert not os.path.exists(filename)
-        assert not os.path.exists(tmpdir['dir'])
+        assert not os.path.exists(tmpdir['subdir'])
+        assert os.path.exists(tmpdir['dir'])
 
     def test_keep_non_empty_dirs(self, tmpdir):
         # Keep non empty directories listed in targets
@@ -433,6 +437,19 @@ def test_keep_non_empty_dirs(self, tmpdir):
             assert expected == os.path.exists(filename)
         assert os.path.exists(tmpdir['dir'])
 
+    def test_clean_in_reverse_lexical_order(self, tmpdir):
+        # Remove targets in reverse lexical order so that subdirectories' order
+        # in the targets array is irrelevant
+        targets = tmpdir['files'] + [tmpdir['dir'], tmpdir['subdir']]
+        t = task.Task("xxx", None, targets=targets, clean=True)
+        assert True == t._remove_targets
+        assert 0 == len(t.clean_actions)
+        t.clean(StringIO(), False)
+        for filename in tmpdir['files']:
+            assert not os.path.exists(filename)
+        assert not os.path.exists(tmpdir['dir'])
+        assert not os.path.exists(tmpdir['subdir'])
+
     def test_clean_actions(self, tmpdir):
         # a clean action can be anything, it can even not clean anything!
         c_path = tmpdir['files'][0]