On Mon, Nov 11 2013, Tomi Ollila <tomi.ollila@iki.fi> wrote: > Hi > > I think these changes would be good to have in use before notmuch compact > is in wider usage. > > All tests pass. I plan to test all of these code paths manually tomorrow. > If anyone comes up with good plan how to add automatic tests I'll add > those too (I also think myself, haven't got any good ones yet). I made this change to the code: diff --git a/lib/database.cc b/lib/database.cc index 40272dc..ad7002b 100644 --- a/lib/database.cc +++ b/lib/database.cc @@ -933,2 +933,2 @@ notmuch_database_compact (const char* path, - } catch (Xapian::InvalidArgumentError e) { - fprintf (stderr, "Error while compacting: %s\n", e.get_msg().c_str()); + } catch (Xapian::Error &error) { + fprintf (stderr, "Error while compacting: %s\n", error.get_msg().c_str()); Now it is consistent with other code and also doesn't crash on other errors I did the following tests: $ pwd /path/to/.notmuch $ mkdir xapian.old $ notmuch compact Compacting database... Backup path already exists: /path/to/.notmuch/xapian.old Compaction failed: Something went wrong trying to read or write a file zsh: exit 1 ./notmuch compact $ : tried to make compact fail by doing: $ : mkdir xapian.compact / touch xapian.compact + chmod 000 xapian.compact $ : notmuch compact worked OK after the above. $ chmod 555 . $ notmuch compact Compacting database... Error while compacting: /path/to/.notmuch/xapian.compact: cannot create directory Compaction failed: A Xapian exception occurred zsh: exit 1 ./notmuch compact $ chmod 755 . $ notmuch compact --backup=/tmp/exdev Compacting database... ... ... ... Error moving old database out of the way: Old database: /path/to/.notmuch/xapian Backup database: /tmp/exdev Error: Invalid cross-device link Compaction failed: Something went wrong trying to read or write a file zsh: exit 1 ./notmuch compact --backup=/tmp/exdev $ : here I tried all I could think of (that doesn't include modifying $ : permissions on the fly (perhaps in lib/database.cc code!), but $ : could not get if (rename(compact_xapian_path, xapian_path)) { $ : fail. i.e. it is possible although highly unlikely (which is good). $ chmod 555 xapian Compacting database... ... ... ... Error removing backup database: Permission denied Please remove the backup database with rm -rf '/path/to/.notmuch/xapian.old' Compaction failed: Something went wrong trying to read or write a file zsh: exit 1 ./notmuch compact $ chmod 755 xapian $ notmuch compact --backup=xapian.backup Compacting database... ... ... ... The old database has been moved to xapian.backup. Done. $ ls xapian.backup flintlock postlist.baseB record.baseB termlist.baseB iamchert postlist.DB record.DB termlist.DB postlist.baseA record.baseA termlist.baseA --- Unless there are other comments I'll make new patch series where } catch (Xapian::Error &error) { and submit it for inclusion to 0.17. we'd better have somewhat decent 'notmuch compact' from day one. Tomi