monotone

Issue 130: database-optional commands require a database now

Reported by Timothy Brownawell, Jan 10, 2011

Steps to reproduce the problem:
-------------------------------

1. mtn privkey $KEY --no-workspace
2. mtn genkey foo@bar.com --no-workspace
3. mtn ls keys --no-workspace

Expected result:
----------------
Those should all work.

Actual results:
---------------
mtn: misuse: no database specified

Output of `mtn version --full`:
-------------------------------
3903ea47e7b1a656e0695f10d57bc0e9deca205b (current head; probably 
broken by the "don't assume :memory:" fix)

There is a database::database_specified() that a few places use to 
check whether there really is a db. Previously constructing a 
database object without an actual database would work, and it would 
only error out when you tried to use it. Possibly instead we want 
something like

class maybe_database {
  // we own it
  shared_ptr<database> const _db;
  // we don't own it
  database * const __db;
public:
  explicit maybe_database(app_state & app)
    : _db(app.opts.dbname_given ? new database(app) : 0),
      __db(0) { }
  /*explicit*/ maybe_database(database & db)
    : __db(&db) { }
  bool exists() const { return _db || __db; }
  database & get() const {
    if (_db) return *_db;
    if (__db) return *__db;
    I(false);
  }
};

and then things that currently call database::database_specified() 
would instead accept one of these.

Comment 1 by Thomas Keller, Jan 11, 2011

Status: Accepted
Owner: tommyd

Comment 2 by Richard Levitte, Jan 16, 2011

I experimented a bit with Timmothy's idea, but it became a bit 
clumsy...  I would rather think that an option argument for the 
constructor database::database(app_state&), making it 
database::database(app_state&, 
database::options=database::options::none).  database::options would 
be an enum:

enum { none, maybe_unspecified } options;

All that's needed is to have a proper test for it in 
database_path_helper::get_database_path.

Comment 3 by Richard Levitte, Jan 17, 2011

Have a look at revision 1680b6fb5779e66e0d1b6cea993e711bd9e01e66, I 
believe it answers quite a lot of the issue.

It still needs a test for commands with --no-workspace.  It's 
obvious most key commands fit in there, but are there other commands 
that should be checked as well?

(stealing this ticket from tommyd ;-))
Owner: levitte

Comment 4 by Timothy Brownawell, Jan 18, 2011

No other commands come to mind as needing to check.

I'd prefer if dboptions was declared inside the database class (or a 
dboptions namespace), to not put a "none" value in the 
global namespace.

Comment 5 by Richard Levitte, Jan 18, 2011

Good suggestion, thank you.

Latest changes:
- dboptions moved into class database
- tests/key_management_without_a_database corrected to really work 
without a database, and had a few more commands added for test.

e3116ec35bd77e9419683c331cea14fcb1d2f3cb

I think it's ready to get propagated back into net.venge.monotone, 
but would like a review by someone first.
Status: Fixed
Summary: database-optional commands require a database now

Comment 6 by Stephen Leake, Jan 23, 2011

looks good to me. the only failing tests (on MinGW) are fixed in 
main.

Comment 7 by Richard Levitte, Jan 23, 2011

Status: Verified

Comment 8 by Richard Levitte, Jan 23, 2011

Thanks.
I've now propagated this back to net.venge.monotone.

Created: 13 years 2 months ago by Timothy Brownawell

Updated: 13 years 2 months ago

Status: Verified

Owner: Richard Levitte

Followed by: 2 persons

Labels:
Type:Incorrect Behavior
Priority:Medium

Quick Links:     www.monotone.ca    -     Downloads    -     Documentation    -     Wiki    -     Code Forge    -     Build Status