]> git.michaelhowe.org Git - packages/b/bup.git/commitdiff
Fix bugs in new indexing code.
authorAvery Pennarun <apenwarr@gmail.com>
Wed, 3 Feb 2010 23:56:43 +0000 (18:56 -0500)
committerAvery Pennarun <apenwarr@gmail.com>
Thu, 4 Feb 2010 04:49:20 +0000 (23:49 -0500)
The logic was way too screwy, so I've simplified it a lot.  Also extended
the unit tests quite a bit to replicate the weird problems I was having.  It
seems pretty stable - and pretty fast - now.

Iterating through an index of my whole home directory (bup index -p ~) now
takes about 5.1 seconds, vs. 3.5 seconds before the rewrite.  However,
iterating through just a *fraction* of the index can now bypass all the
parts we don't care about, so it's much much faster than before.

Could probably still stand some more optimization eventually, but at least
the file format allows for speed.  The rest is just code :)

cmd-index.py
index.py
t/test.sh

index db97bed94cec3b8ebb04c24d60962e0b2cf93882..d2eeaa7e3aa08a038b1c687ddb1869078cb81211 100755 (executable)
@@ -4,9 +4,27 @@ import options, git, index, drecurse
 from helpers import *
 
 
+def _simplify_iter(iters):
+    l = list([iter(it) for it in iters])
+    l = list([(next(it),it) for it in l])
+    del iters
+    l = filter(lambda x: x[0], l)
+    while l:
+        l.sort()
+        (e,it) = l.pop()
+        if not e:
+            continue
+        #log('merge: %r %r (%d)\n' % (e.ctime, e.name, len(l)))
+        if e.ctime:  # skip auto-generated entries
+            yield e
+        n = next(it)
+        if n:
+            l.append((n,it))
+
+
 def merge_indexes(out, r1, r2):
     log('bup: merging indexes.\n')
-    for e in index._last_writer_wins_iter([r1, r2]):
+    for e in _simplify_iter([r1, r2]):
         #if e.flags & index.IX_EXISTS:
             out.add_ixentry(e)
 
@@ -25,6 +43,31 @@ class IterHelper:
         return self.cur
 
 
+def check_index(reader):
+    try:
+        log('check: checking forward iteration...\n')
+        e = None
+        d = {}
+        for e in reader.forward_iter():
+            if e.children_n:
+                log('%08x+%-4d %r\n' % (e.children_ofs, e.children_n, e.name))
+                assert(e.children_ofs)
+                assert(e.name.endswith('/'))
+                assert(not d.get(e.children_ofs))
+                d[e.children_ofs] = 1
+        assert(not e or e.name == '/')  # last entry is *always* /
+        log('check: checking normal iteration...\n')
+        last = None
+        for e in reader:
+            if last:
+                assert(last > e.name)
+            last = e.name
+    except:
+        log('index error! at %r\n' % e)
+        raise
+    log('check: passed.\n')
+
+
 def update_index(top):
     ri = index.Reader(indexfile)
     wi = index.Writer(indexfile)
@@ -64,8 +107,14 @@ def update_index(top):
         ri.save()
         wi.flush()
         if wi.count:
+            wr = wi.new_reader()
+            if opt.check:
+                log('check: before merging: oldfile\n')
+                check_index(ri)
+                log('check: before merging: newfile\n')
+                check_index(wr)
             mi = index.Writer(indexfile)
-            merge_indexes(mi, ri, wi.new_reader())
+            merge_indexes(mi, ri, wr)
             ri.close()
             mi.close()
         wi.abort()
@@ -80,17 +129,19 @@ p,print    print the index entries for the given names (also works with -u)
 m,modified print only added/deleted/modified files (implies -p)
 s,status   print each filename with a status char (A/M/D) (implies -p)
 H,hash     print the hash for each object next to its name (implies -p)
+l,long     print more information about each file
 u,update   (recursively) update the index entries for the given filenames
 x,xdev,one-file-system  don't cross filesystem boundaries
-fake-valid    mark all index entries as up-to-date even if they aren't
+fake-valid mark all index entries as up-to-date even if they aren't
+check      carefully check index file integrity
 f,indexfile=  the name of the index file (default 'index')
 v,verbose  increase log output (can be used more than once)
 """
 o = options.Options('bup index', optspec)
 (opt, flags, extra) = o.parse(sys.argv[1:])
 
-if not (opt.modified or opt['print'] or opt.status or opt.update):
-    log('bup index: you must supply one or more of -p, -s, -m, or -u\n')
+if not (opt.modified or opt['print'] or opt.status or opt.update or opt.check):
+    log('bup index: supply one or more of -p, -s, -m, -u, or --check\n')
     o.usage()
 if opt.fake_valid and not opt.update:
     log('bup index: --fake-valid is meaningless without -u\n')
@@ -99,6 +150,10 @@ if opt.fake_valid and not opt.update:
 git.check_repo_or_die()
 indexfile = opt.indexfile or git.repo('bupindex')
 
+if opt.check:
+    log('check: starting initial check.\n')
+    check_index(index.Reader(indexfile))
+
 paths = index.reduce_paths(extra)
 
 if opt.update:
@@ -110,8 +165,10 @@ if opt.update:
 
 if opt['print'] or opt.status or opt.modified:
     for (name, ent) in index.Reader(indexfile).filter(extra or ['']):
-        if opt.modified and (ent.flags & index.IX_HASHVALID
-                             or stat.S_ISDIR(ent.mode)):
+        if (opt.modified 
+            and (ent.flags & index.IX_HASHVALID
+                 or not ent.mode
+                 or stat.S_ISDIR(ent.mode))):
             continue
         line = ''
         if opt.status:
@@ -124,11 +181,17 @@ if opt['print'] or opt.status or opt.modified:
                     line += 'M '
             else:
                 line += '  '
+        if opt.long:
+            line += "%7s " % oct(ent.mode)
         if opt.hash:
             line += ent.sha.encode('hex') + ' '
         print line + (name or './')
         #print repr(ent)
 
+if opt.check:
+    log('check: starting final check.\n')
+    check_index(index.Reader(indexfile))
+
 if saved_errors:
     log('WARNING: %d errors encountered.\n' % len(saved_errors))
     sys.exit(1)
index 3af039df1999f1bccf7db49e8aebdef51ff3f955..5ba57cf3c670d2cff7fefb59eea16341e288b228 100644 (file)
--- a/index.py
+++ b/index.py
@@ -14,22 +14,62 @@ class Error(Exception):
     pass
 
 
-def _encode(dev, ctime, mtime, uid, gi8d, size, mode, gitmode, sha, flags):
-    return struct.pack(INDEX_SIG,
-                       dev, ctime, mtime, uid, gid, size, mode,
-                       gitmode, sha, flags)
+class Level:
+    def __init__(self, ename, parent):
+        self.parent = parent
+        self.ename = ename
+        self.list = []
+        self.count = 0
+
+    def write(self, f):
+        (ofs,n) = (f.tell(), len(self.list))
+        if self.list:
+            count = len(self.list)
+            #log('popping %r with %d entries\n' 
+            #    % (''.join(self.ename), count))
+            for e in self.list:
+                e.write(f)
+            if self.parent:
+                self.parent.count += count + self.count
+        return (ofs,n)
+
+
+def _golevel(level, f, ename, newentry):
+    # close nodes back up the tree
+    assert(level)
+    while ename[:len(level.ename)] != level.ename:
+        n = BlankNewEntry(level.ename[-1])
+        (n.children_ofs,n.children_n) = level.write(f)
+        level.parent.list.append(n)
+        level = level.parent
+
+    # create nodes down the tree
+    while len(level.ename) < len(ename):
+        level = Level(ename[:len(level.ename)+1], level)
+
+    # are we in precisely the right place?
+    assert(ename == level.ename)
+    n = newentry or BlankNewEntry(ename and level.ename[-1] or None)
+    (n.children_ofs,n.children_n) = level.write(f)
+    if level.parent:
+        level.parent.list.append(n)
+    level = level.parent
+
+    return level
+
 
 class Entry:
-    def __init__(self, name):
+    def __init__(self, basename, name):
+        self.basename = str(basename)
         self.name = str(name)
         self.children_ofs = 0
         self.children_n = 0
 
     def __repr__(self):
-        return ("(%s,0x%04x,%d,%d,%d,%d,%d,0x%04x)" 
+        return ("(%s,0x%04x,%d,%d,%d,%d,%d,0x%04x,0x%08x/%d)" 
                 % (self.name, self.dev,
                    self.ctime, self.mtime, self.uid, self.gid,
-                   self.size, self.flags))
+                   self.size, self.flags, self.children_ofs, self.children_n))
 
     def packed(self):
         return struct.pack(INDEX_SIG,
@@ -70,11 +110,14 @@ class Entry:
     def __cmp__(a, b):
         return cmp(a.name, b.name)
 
+    def write(self, f):
+        f.write(self.basename + '\0' + self.packed())
+
 
 class NewEntry(Entry):
-    def __init__(self, name, dev, ctime, mtime, uid, gid,
+    def __init__(self, basename, name, dev, ctime, mtime, uid, gid,
                  size, mode, gitmode, sha, flags, children_ofs, children_n):
-        Entry.__init__(self, name)
+        Entry.__init__(self, basename, name)
         (self.dev, self.ctime, self.mtime, self.uid, self.gid,
          self.size, self.mode, self.gitmode, self.sha,
          self.flags, self.children_ofs, self.children_n
@@ -82,9 +125,16 @@ class NewEntry(Entry):
               size, mode, gitmode, sha, flags, children_ofs, children_n)
 
 
+class BlankNewEntry(NewEntry):
+    def __init__(self, basename):
+        NewEntry.__init__(self, basename, basename,
+                          0, 0, 0, 0, 0, 0, 0,
+                          0, EMPTY_SHA, 0, 0, 0)
+
+
 class ExistingEntry(Entry):
-    def __init__(self, name, m, ofs):
-        Entry.__init__(self, name)
+    def __init__(self, basename, name, m, ofs):
+        Entry.__init__(self, basename, name)
         self._m = m
         self._ofs = ofs
         (self.dev, self.ctime, self.mtime, self.uid, self.gid,
@@ -100,15 +150,14 @@ class ExistingEntry(Entry):
         if dname and not dname.endswith('/'):
             dname += '/'
         ofs = self.children_ofs
-        #log('myname=%r\n' % self.name)
         assert(ofs <= len(self._m))
         for i in range(self.children_n):
             eon = self._m.find('\0', ofs)
-            #log('eon=0x%x ofs=0x%x i=%d cn=%d\n' % (eon, ofs, i, self.children_n))
             assert(eon >= 0)
             assert(eon >= ofs)
             assert(eon > ofs)
-            child = ExistingEntry(self.name + str(buffer(self._m, ofs, eon-ofs)),
+            basename = str(buffer(self._m, ofs, eon-ofs))
+            child = ExistingEntry(basename, self.name + basename,
                                   self._m, eon+1)
             if (not dname
                  or child.name.startswith(dname)
@@ -150,12 +199,23 @@ class Reader:
     def __del__(self):
         self.close()
 
+    def forward_iter(self):
+        ofs = len(INDEX_HDR)
+        while ofs+ENTLEN <= len(self.m):
+            eon = self.m.find('\0', ofs)
+            assert(eon >= 0)
+            assert(eon >= ofs)
+            assert(eon > ofs)
+            basename = str(buffer(self.m, ofs, eon-ofs))
+            yield ExistingEntry(basename, basename, self.m, eon+1)
+            ofs = eon + 1 + ENTLEN
+
     def iter(self, name=None):
         if len(self.m) > len(INDEX_HDR)+ENTLEN:
             dname = name
             if dname and not dname.endswith('/'):
                 dname += '/'
-            root = ExistingEntry('/', self.m, len(self.m)-ENTLEN)
+            root = ExistingEntry('/', '/', self.m, len(self.m)-ENTLEN)
             for sub in root.iter(name=name):
                 yield sub
             if not dname or dname == root.name:
@@ -185,41 +245,9 @@ class Reader:
                 yield (name, e)
 
 
-# Read all the iters in order; when more than one iter has the same entry,
-# the *later* iter in the list wins.  (ie. more recent iter entries replace
-# older ones)
-def _last_writer_wins_iter(iters):
-    l = []
-    for e in iters:
-        it = iter(e)
-        try:
-            l.append([it.next(), it])
-        except StopIteration:
-            pass
-    del iters  # to avoid accidents
-    while l:
-        l.sort()
-        mv = l[0][0]
-        mi = []
-        for (i,(v,it)) in enumerate(l):
-            #log('(%d) considering %d: %r\n' % (len(l), i, v))
-            if v > mv:
-                mv = v
-                mi = [i]
-            elif v == mv:
-                mi.append(i)
-        yield mv
-        for i in mi:
-            try:
-                l[i][0] = l[i][1].next()
-            except StopIteration:
-                l[i] = None
-        l = filter(None, l)
-
-
 class Writer:
     def __init__(self, filename):
-        self.stack = []
+        self.rootlevel = self.level = Level([], None)
         self.f = None
         self.count = 0
         self.lastfile = None
@@ -241,9 +269,10 @@ class Writer:
             os.unlink(self.tmpname)
 
     def flush(self):
-        while self.stack:
-            self.add(''.join(self.stack[-1][0]), None)
-        self._pop_to(None, [])
+        if self.level:
+            self.level = _golevel(self.level, self.f, [], None)
+        assert(self.level == None)
+        self.count = self.rootlevel.count
         self.f.flush()
 
     def close(self):
@@ -254,41 +283,18 @@ class Writer:
             f.close()
             os.rename(self.tmpname, self.filename)
 
-    # FIXME: this function modifies 'entry' and can only pop a single level.
-    # That means its semantics are basically crazy.
-    def _pop_to(self, entry, edir):
-        assert(len(self.stack) - len(edir) <= 1)
-        while self.stack and self.stack[-1][0] > edir:
-            #log('popping %r with %d entries (%d)\n' 
-            #    % (''.join(self.stack[-1][0]), len(self.stack[-1][1]),
-            #       len(self.stack)))
-            p = self.stack.pop()
-            entry.children_ofs = self.f.tell()
-            entry.children_n = len(p[1])
-            for e in p[1]:
-                self._write(e)
-
-    def _write(self, entry):
-        #log('        writing %r\n' % entry.name)
-        es = pathsplit(entry.name)
-        self.f.write(es[-1] + '\0' + entry.packed())
-        self.count += 1
-
-    def _add(self, entry):
-        es = pathsplit(entry.name)
-        edir = es[:-1]
-        self._pop_to(entry, edir)
-        while len(self.stack) < len(edir):
-            self.stack.append([es[:len(self.stack)+1], [], ()])
-        if entry.name != '/':
-            self.stack[-1][1].append(entry)
-        else:
-            self._write(entry)
+    def _add(self, ename, entry):
+        if self.lastfile and self.lastfile <= ename:
+            raise Error('%r must come before %r' 
+                             % (''.join(e.name), ''.join(self.lastfile)))
+            self.lastfile = e.name
+        self.level = _golevel(self.level, self.f, ename, entry)
 
     def add(self, name, st, hashgen = None):
-        if self.lastfile:
-            assert(cmp(self.lastfile, name) > 0) # reverse order only
         endswith = name.endswith('/')
+        ename = pathsplit(name)
+        basename = ename[-1]
+        #log('add: %r %r\n' % (basename, name))
         flags = IX_EXISTS
         sha = None
         if hashgen:
@@ -299,22 +305,21 @@ class Writer:
         if st:
             isdir = stat.S_ISDIR(st.st_mode)
             assert(isdir == endswith)
-            e = NewEntry(name, st.st_dev, int(st.st_ctime),
+            e = NewEntry(basename, name, st.st_dev, int(st.st_ctime),
                          int(st.st_mtime), st.st_uid, st.st_gid,
                          st.st_size, st.st_mode, gitmode, sha, flags,
                          0, 0)
         else:
             assert(endswith)
-            e = NewEntry(name, 0, 0, 0, 0, 0, 0, 0, gitmode, sha, flags, 0, 0)
-        self.lastfile = name
-        self._add(e)
+            e = BlankNewEntry(basename)
+            e.gitmode = gitmode
+            e.sha = sha
+            e.flags = flags
+        self._add(ename, e)
 
     def add_ixentry(self, e):
-        if self.lastfile and self.lastfile <= e.name:
-            raise Error('%r must come before %r' 
-                             % (e.name, self.lastfile))
-        self.lastfile = e.name
-        self._add(e)
+        e.children_ofs = e.children_n = 0
+        self._add(pathsplit(e.name), e)
 
     def new_reader(self):
         self.flush()
index a5a54eb4093852ce8e2d35cb007737cec4d2aef6..893f63aaf43ed413fcf53663c6cbe9dcc1958f94 100755 (executable)
--- a/t/test.sh
+++ b/t/test.sh
@@ -20,32 +20,41 @@ WVSTART "index"
 D=bupdata.tmp
 rm -rf $D
 mkdir $D
-WVPASSEQ "$(bup index -p)" ""
-WVPASSEQ "$(bup index -p $D)" ""
+WVPASSEQ "$(bup index --check -p)" ""
+WVPASSEQ "$(bup index --check -p $D)" ""
 WVFAIL [ -e $D.fake ]
-WVFAIL bup index -u $D.fake
-WVPASS bup index -u $D
-WVPASSEQ "$(bup index -p $D)" "$D/"
-touch $D/a $D/b
+WVFAIL bup index --check -u $D.fake
+WVPASS bup index --check -u $D
+WVPASSEQ "$(bup index --check -p $D)" "$D/"
+touch $D/a $D/b $D/f
 mkdir $D/d $D/d/e
 WVPASSEQ "$(bup index -s $D/)" "A $D/"
 WVPASSEQ "$(bup index -s $D/b)" ""
 bup tick
-WVPASSEQ "$(bup index -us $D/b)" "A $D/b"
-WVPASSEQ "$(bup index -usx $D)" \
+WVPASSEQ "$(bup index --check -us $D/b)" "A $D/b"
+WVPASSEQ "$(bup index --check -us $D/b $D/d)" \
 "A $D/d/e/
 A $D/d/
+A $D/b"
+touch $D/d/z
+WVPASSEQ "$(bup index --check -usx $D)" \
+"A $D/f
+A $D/d/z
+A $D/d/e/
+A $D/d/
 A $D/b
 A $D/a
 A $D/"
-WVPASSEQ "$(bup index -us $D/a $D/b --fake-valid)" \
+WVPASSEQ "$(bup index --check -us $D/a $D/b --fake-valid)" \
 "  $D/b
   $D/a"
-WVPASSEQ "$(bup index -us $D/a)" "  $D/a"  # stays unmodified
+WVPASSEQ "$(bup index --check -us $D/a)" "  $D/a"  # stays unmodified
 touch $D/a
 WVPASS bup index -u $D/a  # becomes modified
 WVPASSEQ "$(bup index -s $D/a $D $D/b)" \
-"A $D/d/e/
+"A $D/f
+A $D/d/z
+A $D/d/e/
 A $D/d/
   $D/b
 M $D/a
@@ -56,9 +65,13 @@ A $D/"
 # directories, at which time the output of -m will change, so we'll have to
 # change this test too.
 WVPASSEQ "$(cd $D && bup index -m .)" \
-"./a"
+"./f
+./d/z
+./a"
 WVPASSEQ "$(cd $D && bup index -m)" \
-"a"
+"f
+d/z
+a"
 WVPASSEQ "$(cd $D && bup index -s .)" "$(cd $D && bup index -s .)"