Re: [Monotone-devel] basic_io inventory

[Thomas Keller]

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.

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

> I guess it depends on who is reading this doc; for developers, the
> types probably make more sense. If there is a tool that pulls this
> comment into the help output, then your suggestions are definitely
> better.

The docs are not actually generated from that yet, but there is some 
work in the nvm.help-rewrite branch which makes that possible in general.

> 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 =)

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

> > 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 =)

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

> > > -- not in original output (why not?)
> > > check(grep('^   path "inventory_hooks.lua"$', "stdout"), 0, false, false)
> > > check(grep('^fs_type "file"$', "stdout"), 0, false, false)
> > > check(grep('^ status "unknown"$', "stdout"), 0, false, false)
> > > check(grep('^$', "stdout"), 0, false, false)
> > You don't need to check things more than once, right? The complete test
> > suite already takes a long time to finish, no need to make it take even
> > longer =)
>
> I'm not clear what you are saying.
>
> What if a bug causes the text I skip to change in a bad way? I
> prefer to explicitly check all output.

Well, I was heading towards "check each possible state once" =)


Thomas.

--
ICQ: 85945241 | SIP: 1-747-027-0392 | http://www.thomaskeller.biz
> Guitone, a frontend for monotone: http://guitone.thomaskeller.biz
> Music lyrics and more: http://musicmademe.com