Fix count of backed up files

4 files changed, 87 insertions, 37 deletions

diff --git a/NEWS b/NEWS
index 229d2600..dff7609b 100644
--- a/NEWS
+++ b/NEWS
@@ -3,6 +3,15 @@ Obnam NEWS
 
 This file summarizes changes between releases of Obnam.
 
+Version 1.X, released UNRELEASED
+--------------------------------
+
+Bug fixes:
+
+* Fix count of backed up files. It used to always count directories.
+  Reported by Alberto Fuentes as Debian bug
+  [742384](https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=742384).
+
 Version 1.7.2, released 2014-03-22
 --------------------------------
 
diff --git a/debian/changelog b/debian/changelog
index f8b4a650..e3acdd19 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,11 @@
+obnam (1.7.3-1) UNRELEASED; urgency=low
+
+  * New upstream release.
+    * Fix "several consecutive runs back up unmodified files" 
+      (Closes: #742384)
+
+ -- Lars Wirzenius <liw@liw.fi>  Mon, 24 Mar 2014 21:19:14 +0000
+
 obnam (1.7.2-1) unstable; urgency=low
 
   * New upstream release.
diff --git a/obnamlib/plugins/backup_plugin.py b/obnamlib/plugins/backup_plugin.py
index 517c6015..35661f19 100644
--- a/obnamlib/plugins/backup_plugin.py
+++ b/obnamlib/plugins/backup_plugin.py
@@ -524,12 +524,27 @@ class BackupPlugin(obnamlib.ObnamPlugin):
                 try:
                     self.maybe_simulate_error(pathname)
                     if stat.S_ISDIR(metadata.st_mode):
+                        # Directories should only be counted in the
+                        # progress their metadata has changed. This
+                        # covers the case when files have been deleted
+                        # from them.
+                        gen = self.get_current_generation()
+                        if self.metadata_has_changed(gen, pathname, metadata):
+                            self.progress.backed_up_count += 1
+
                         self.backup_dir_contents(pathname)
-                    elif stat.S_ISREG(metadata.st_mode):
-                        assert metadata.md5 is None
-                        metadata.md5 = self.backup_file_contents(pathname,
-                                                                 metadata)
-                    self.backup_metadata(pathname, metadata)
+                        self.backup_metadata(pathname, metadata)
+                    else:
+                        # Non-directories' progress can be updated
+                        # without further thinking.
+                        self.progress.backed_up_count += 1
+
+                        if stat.S_ISREG(metadata.st_mode):
+                            assert metadata.md5 is None
+                            metadata.md5 = self.backup_file_contents(
+                                pathname, metadata)
+                        self.backup_metadata(pathname, metadata)
+
                 except (IOError, OSError) as e:
 
                     if type(e) is IOError:
@@ -642,7 +657,6 @@ class BackupPlugin(obnamlib.ObnamPlugin):
                 metadata = obnamlib.read_metadata(self.fs, pathname, st=st)
                 self.progress.update_progress_with_file(pathname, metadata)
                 if self.needs_backup(pathname, metadata):
-                    self.progress.backed_up_count += 1
                     yield pathname, metadata
                 else:
                     self.progress.update_progress_with_scanned(
@@ -690,36 +704,39 @@ class BackupPlugin(obnamlib.ObnamPlugin):
         if current.isdir():
             tracing.trace('%s is directory, so needs backup' % pathname)
             return True
+
+        gen = self.get_current_generation()
+        tracing.trace('gen=%s' % repr(gen))
+        return self.metadata_has_changed(gen, pathname, current)
+
+    def get_current_generation(self):
+        '''Return the current generation.
+
+        This handles pretend-mode correctly (in pretend-mode we don't
+        have self.new_generation).
+
+        '''
+
         if self.pretend:
             gens = self.repo.get_client_generation_ids(self.client_name)
-            if not gens:
-                return True
-            gen = gens[-1]
+            assert gens, "Can't handle --pretend without generations"
+            return gens[-1]
         else:
-            gen = self.new_generation
-        tracing.trace('gen=%s' % repr(gen))
+            return self.new_generation
+
+    def metadata_has_changed(self, gen, pathname, current):
+        '''Has the metadata for pathname changed since given generation?
+
+        Treat a file that didn't exist in the generation as changed.
+
+        '''
+
         try:
-            old = obnamlib.Metadata(
-                st_mtime_sec=self.repo.get_file_key(
-                    gen, pathname, obnamlib.REPO_FILE_MTIME_SEC),
-                st_mtime_nsec=self.repo.get_file_key(
-                    gen, pathname, obnamlib.REPO_FILE_MTIME_NSEC),
-                st_mode=self.repo.get_file_key(
-                    gen, pathname, obnamlib.REPO_FILE_MODE),
-                st_nlink=self.repo.get_file_key(
-                    gen, pathname, obnamlib.REPO_FILE_NLINK),
-                st_size=self.repo.get_file_key(
-                    gen, pathname, obnamlib.REPO_FILE_SIZE),
-                st_uid=self.repo.get_file_key(
-                    gen, pathname, obnamlib.REPO_FILE_UID),
-                st_gid=self.repo.get_file_key(
-                    gen, pathname, obnamlib.REPO_FILE_GID),
-                xattr=self.repo.get_file_key(
-                    gen, pathname, obnamlib.REPO_FILE_XATTR_BLOB))
+            old = self.get_metadata_from_generation(gen, pathname)
         except obnamlib.ObnamError as e:
             # File does not exist in the previous generation, so it
             # does need to be backed up.
-            tracing.trace('%s not in previous gen, so needs backup' % pathname)
+            tracing.trace('%s not in previous gen so has changed' % pathname)
             tracing.trace('error: %s' % str(e))
             tracing.trace(traceback.format_exc())
             return True
@@ -740,7 +757,8 @@ class BackupPlugin(obnamlib.ObnamPlugin):
             tracing.trace('current.%s=%r', field, current_value)
             tracing.trace('old.%s=%r', field, old_value)
             if current_value != old_value:
-                tracing.trace('NEED to backup %r', pathname)
+                tracing.trace(
+                    'DIFFERENT metadata %r for %r', field, pathname)
                 return True
 
         # Treat xattr values None (no extended attributes) and ''
@@ -751,12 +769,31 @@ class BackupPlugin(obnamlib.ObnamPlugin):
         tracing.trace('xattr_current=%r', xattr_current)
         tracing.trace('xattr_old=%r', xattr_old)
         if xattr_current != xattr_old:
-            tracing.trace('NEED to backup %r', pathname)
+            tracing.trace('DIFFERENT xattr for %r', pathname)
             return True
 
-        tracing.trace('Do NOT need to backup %r', pathname)
+        tracing.trace('NOT DIFFERENT metadata for %r', pathname)
         return False
 
+    def get_metadata_from_generation(self, gen, pathname):
+        return obnamlib.Metadata(
+            st_mtime_sec=self.repo.get_file_key(
+                gen, pathname, obnamlib.REPO_FILE_MTIME_SEC),
+            st_mtime_nsec=self.repo.get_file_key(
+                gen, pathname, obnamlib.REPO_FILE_MTIME_NSEC),
+            st_mode=self.repo.get_file_key(
+                gen, pathname, obnamlib.REPO_FILE_MODE),
+            st_nlink=self.repo.get_file_key(
+                gen, pathname, obnamlib.REPO_FILE_NLINK),
+            st_size=self.repo.get_file_key(
+                gen, pathname, obnamlib.REPO_FILE_SIZE),
+            st_uid=self.repo.get_file_key(
+                gen, pathname, obnamlib.REPO_FILE_UID),
+            st_gid=self.repo.get_file_key(
+                gen, pathname, obnamlib.REPO_FILE_GID),
+            xattr=self.repo.get_file_key(
+                gen, pathname, obnamlib.REPO_FILE_XATTR_BLOB))
+
     def add_file_to_generation(self, filename, metadata):
         self.repo.add_file(self.new_generation, filename)
 
diff --git a/yarns/0030-basics.yarn b/yarns/0030-basics.yarn
index 7a28720d..936d3be8 100644
--- a/yarns/0030-basics.yarn
+++ b/yarns/0030-basics.yarn
@@ -119,11 +119,7 @@ backup run.
 
     WHEN user U removes file obnam.log
     AND user U backs up directory L to repository R
-
-Note that Obnam always backs up the root directory, so we match
-against one, not zero, below.
-
-    THEN obnam.log matches INFO \* files backed up: 1$
+    THEN obnam.log matches INFO \* files backed up: 0$
     AND L, restored to X, matches manifest M