[Monotone-devel] RFC: Restricting app_state to commands

[Zack Weinberg]

I just pushed a change that (contrary to the impression one might get
from ciabot :) had the primary purpose of removing all inclusions of
app_state.hh from other headers.  Without exception, the uses of that
type in headers need only "class app_state;".  We do have to include
the actual header in a bunch more .cc files now, but that's fine.
This has a nice effect on the size of both preprocessed text and
assembly output; it's not obvious to me exactly where the win is
coming from, but the difference can be huge (e.g. refiner.ii shrank by
15,000 lines and refiner.s by 4000; full statistics at the end of the
message).  The executable itself also shrinks, but only by a few K - I
presume that most of the gain at the assembly level is duplicate
template instantiations and other cruft that the linker throws away
anyway.

I'd like to discuss a more radical change, though: I think we should
stop passing the app_state object down below the level of CMD()
functions.  I've come to this conclusion over the past few weeks by
thinking about what we're going to have to do to make the distinction
between a read-only and read-write database handle in the type system.
I thought there was discussion of this on the wiki, but I can't find
it - basically we want to be able to use non-exclusive database locks
when we're just reading; we want the compiler to catch locking errors;
the way to do that is to make "readonly database handle" and "writable
database handle" distinct types.  But then the database handle needs
to come out of the app_state; every function that currently takes an
app_state in order to get at the database needs to instead receive it
as a separate parameter, so that we can specify in the function
signature whether it wants a writable handle or a readonly one will
do.  (Obviously we will arrange for a writable handle to be an
acceptable argument to readonly functions - that's just a subclassing
exercise.)

So okay, we have to do it to the database, why generalize to
everything else?  For clarity, "everything else" is the lua hooks, the
options, the workspace, the ssh-agent connection, the project, and the
keystore.  For the workspace, keystore, and agent, a similar argument
to the database applies: we would like to be explicit about which
operations use them and which do not, and it may also make sense to be
explicit about whether the operation may write to the object (note in
particular that we have *no* keystore locking right now).  For the
rest of it, I have two rationales: (1) it will enable further
reductions in which headers are exposed in which source files
(currently any number of places need to read app_state.hh just so they
can get at the one or two subcomponents of the structure that they
need - and it's common for this to be just the lua hooks, or just one
or two options), (2) it will be easier to implement a general policy
of "never pass an app_state reference" than to pull individual
sub-objects out of the app_state one at a time.

Thoughts?

zw

-----
file              .ii      .s
annotate           -4      -4
asciik           -190    -700
automate          -36     161
cert                2       0
cmd_automate     -184    -412
cmd_db            -29      -2
cmd_diff_log     -197    -492
cmd_files        -199    -360
cmd_key_cert     -199   -1643
cmd_list         -188   -1250
cmd_merging      -158   -1717
cmd_netsync       -52      -7
cmd_othervcs     -211    -211
cmd_packet       -182    -211
cmd_ws_commit    -172   -1441
commands          -50  -13721 *
database            2     215
database_check      4      -1
diff_patch          5    -307
enumerator          8    -235
key_store           0    -141
keys                0     141
lua_hooks           0       4
merge              24      53
merkle_tree    -14428   -3540
monotone           -1       0
netcmd         -15008   -3535
netsync             4    -225
packet              2       0
project             2      52
rcs_import          2     146
refiner        -15025   -4043
restrictions        2      26
revision          -38    -129
roster             21    6277 *
roster_delta        0     170
roster_merge        0       3
sha1            -3019  -78737
update              4     -77
work           -11816  -28612
work_migration -12351  -32537

total          -73655 -167042

* roster.cc had a function moved to it from commands.cc.