Re: [Monotone-devel] basic_io inventory

[Stephen Leake]

Thomas Keller <address@hidden> writes:

> Stephen Leake schrieb:
>>> For the tests, I think you should implement the actual matching after
>>> you've parsed the basic_io properly. For this there exists a function
>>> named parse_basic_io which takes the basic_io string as input and
>>> returns a lua table. The automate_keys test includes an example how to
>>> cope with that.
>> Hmm. I find that test very hard to understand. Since part of my goal
>> is to understand the basic_io output in order to
>> help implement a parser for Emacs DVC, I'd rather have the monotone
>> test document the actual text format. Unless there's a strong
>> objection to that.
>
> The problem is that grep'ing the output like this makes you assume
> that lines in the stanza are outputted exactly the way they are, i.e.
> aren't reordered, don't get additional spaces, etc. While basic_io
> parsers normally make absolute _no_ assumptions if their stanzas are
> properly indented, your test code shouldn't either.

Ah. That makes sense.

So if the output changes format slightly, but within the bounds a
standard parser will tolerate, the test does not have to be updated.

Is there some documentation on these requirements for basic_io
parsers? I found basic_io.hh and luaext_parse_basic_io.cc; not many
comments :). I searched the wiki for "basic_io" and "basic io"; I
found BasicIoFormalization, which doesn't say much, but hints at
"formal docs" somewhere.

It does point to an IRC session, which says that stanzas are not
actually meaningful. Which is a scary concept; it implies that in the
'automate inventory' output, the 'path' and 'old_node' lines are
_not_ related to each other. Which is of course nonsense!

I also browsed monotone.info for 'basic_io'. The definition of
'parse_basic_io' also makes it clear that stanzas are meaningless. You
said earlier that "lines can be reordered". How then do I associate
'path' with 'old_node'? the "closest line"? the "previous line"?

One IRC comment mentioned "the basic_io grammar in the manual's
appendix"; I don't see that in monotone.info.

Some of the IRC comments imply that tokens _cannot_ be reordered; they
talk about "scan to the token you want, then skip n tokens, read the
next".

Without syntax to define a stanza, there must be some order; 'path'
must start a new set of information, for example. Statements between
'path' statements relate to the previous 'path' statement; they can be
reordered.

Some variation in format could be handled by making the regular
expressions in the test more generic; [:blank:]+ for whitespace. But
the order would be fixed. I used an Emacs macro to write these tests;
that was easy, given the test log.

Using parse_basic_io would provide a level of standardization. I'll
see if I can figure it out, and add comments or something to make the
test more readable.

>>>> //    old_node "<old_node_id>" "<old_node path::status>"
>>>> //    new_node "<new_node_id>" "<new_node path::status>"
>>>> //     fs_type "<current path::status>"
>>> path::status could be simply resolved to "file|directory", and for
>>> fs_type "file|directory|none" (none if the file is missing). This makes
>>> it easier to read.
>> Partly I was documenting what types are used, so I could understand
>> the type structure.
>
> The used types are completly internal. The "user", the program that
> calls inventory, just sees a string.

Yes. I tend to write comments in .hh files for users, and comments in
.cc files for developers. That distinction doesn't apply here. For now
I'll just keep both sets of comments.

The user documentation should be in montone.info.

>> What do you think about outputing 'none' explicitly, rather than
>> leaving out 'old_node' and 'new_node'? It would make the parser more
>> regular. But maybe there's a standard basic_io style?
>
> Well, "basic_io style" is: leave out what is already implicit =)

Ok. Does that extend to "leave out what the user has requested be
ignored" ? :)

>>>> // Error conditions: If no workspace book keeping _MTN directory is found,
>>>> //   prints an error message to stderr, and exits with status 1.
>>>>
>>>> Is this correct so far?
>>> You might want to add a short note about the possible PATH argument and
>>> the options, but this is nit-picking here and could also just go into
>>> monotone.texi.
>> PATH is mentioned as an argument; it seems to be a standard argument,
>> so I didn't think it needed describing.
>> Are there options? the declaration says 'options::opts::none'; so I
>> assumed there were none for 'inventory'; there are standard options
>> for 'automate'.
>
> At least in my head (65dcf7bd70c798d6a8d4216628ad664223e52295) there are
> options::opts::depth | options::opts::exclude mentioned.

Ah. I guess I need to update.

(pause while I figure out how to get Emacs DVC to show me a preview of
the changes, and do the update; DVC needs work)

Ok; I see those options now.

>>> a) its not a good idea to test for specific values here
>> Why not? You seem to be implying that running this test on different
>> machines might produce different results.
>
> Because these node numbers are completly internal. You should not rely
> on them having specific values, because maybe they change if you
> change the platform (32bit <> 64bit) (this is just a guess). Anyways,
> why should you even bother? They're completly uninteresting =)

If they are truly uninteresting, they don't need to be in the output
in the first place. 

Since they are there, I assume they will be used (although I am
unclear when). Thus we need to at least test for them being present.

We should test that they are correct. It would be a severe bug to
output the node_id for file "foo" in the stanza for file "bar". The
test should check for that.

The simplest way to test for that, _if_ the node ids are repeatable,
is to check for the explicit value.

A more complex way would be to use the node_id to retrieve some
information about the file, and check that it matches.

I suggest we check for the explicit value now, with a comment in the
test about how we are not sure that is truly portable, and suggestions
on what to do if it turns out not to be. There's enough work to do
without looking for trouble.

>>> b) the explicit note that this file is added is indeed missing; as
>>> long as there is no other node outputted which has the new_node id
>>> set in its old_node id (which practically makes the add a rename) we
>>> can however assume that this is an add
>> That places the burden of computing "added" on the external tool.
>> Clearly 'automate inventory' could do that same computation; it is
>> already walking the entire tree. Then the computation would be
>> standard; different tools would share a common definition of "added".
>
> Hrm... good points.

Ok. I'll see if I can figure out the tree walking well enough to add
this. 

--
-- Stephe