[Monotone-devel] Updated Issue 130 - database-optional commands require a database now (monotone)

[code]

Hello,

The following issue has been updated:

130 - database-optional commands require a database now
Project: monotone
Status: Verified
Reported by: Timothy Brownawell
URL: https://code.monotone.ca/p/monotone/issues/130/
Labels:
 Type:Incorrect Behavior
 Priority:Medium

Comments (last first):

# By Richard Levitte, Jan 23, 2011:



 Status: Verified

# By Stephen Leake, Jan 23, 2011:

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

# 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

# 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.

# 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

# 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.

# By Thomas Keller, Jan 11, 2011:



 Status: Accepted
 Owner: tommyd

# By Timothy Brownawell, Jan 10, 2011:

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

1. mtn privkey $KEY --no-workspace
2. mtn genkey address@hidden --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.



--
Issue: https://code.monotone.ca/p/monotone/issues/130/