Re: [Monotone-devel] options for automate inventory

[Stephen Leake]

Thomas Keller <address@hidden> writes:

> Stephen Leake schrieb:
>> As others noticed, 0: does work in automate stdio for flag options.
>> 
>> So I've committed new automate inventory options --no-ignored,
>> --no-unknown, --no-unchanged, with appropriate tests, manual update,
>> and NEWS entry.
>
> A few comments:
>
> 1) Please edit the comment in front of
> tests/automate_inventory_options/__driver__.lua
> which still lists the old true/false options

Done.

> 2) From what I can see in the code you determine if a file stanza should
> be omitted from the inventory_print_states and inventory_print_changes
> methods. 

Right. But since each only knows part of the information, they can't
just output 'print_this' directly.

> If you really go that route, the three boolean values should at
> least be set to false _outside_ of the first method (currently
> they're initialized in the body), 

Ah. That's a difference between Ada (my everyday language) and C++; in
Ada, you can declare a subprogram parameter to be "in", "in out" or
"out", so it is clear whether you can rely on the initial value. In
C++, it is not clear whether a "&" parameter is "in out" or just
"out"; I was intending these to be "out" for inventory_print_states,
since it doesn't use the initial value, and "in out" for
inventory_print_changes, since it does.

I've added comments saying something like that.

> but I really think the actual filtering should be done beforehand
> and not be mixed into these two methods. I know this involves a
> litte code duplication, but its far easier to understand later on.

But not to maintain; it would be too easy to change one routine
without changing the other.

If these procedure names where changed to "inventory_compute_state",
"inventory_compute_changes", would that make it clearer?

Do the comments I've added help?

> 3) Finally, its easier to continue; immediately as soon as its clear
> some code path cannot be executed than waiting / evaluating up to three
> additional conditions ;)

Ada doesn't have 'continue', so I tend not to use it in C++ either :).

Done.

> 4) A minor thing: the interface_version does not neccessarily need to be
> bumped to 7.1, since mtn 0.38 is 6.0 anyways... and I *think* there was
> a previously used rule that only command additions get a minor raise,
> however format / output changes of existing commands get a major raise.
> So either stick with 7.0 (which incorporates the attributes stuff) or
> raise it to 8.0.

The comment on interface_version in cmd_automate.cc says:

// Purpose: Prints version of automation interface.  Major number increments
//   whenever a backwards incompatible change is made; minor number increments
//   whenever any change is made (but is reset when major number increments).

This doesn't say anything about "only one increment per mtn version";
perhaps it should?

This counts as "any change", but it is backwards compatible - if you
don't use the options, you don't notice they got added.

Hmm. "don't recurse into ignored directories" is not backwards
compatible. But I see that as bug fix; no one should have been relying
on that behavior in the first place.

And I need the interface_version change in my current Emacs DVC code,
since the monotone version has _not_ changed yet; I need to know
whether I can use --no-ignored or not. I suppose that's a minor point;
if I'm using the head of the development branch, I should know what
I'm doing :).

Still, it makes more sense to decide whether to use --no-ignored based
on automate interface_version rather than mtn version.

> 5) The monotone.texi patch contains a lot of unrelated changes - seems
> as some formatter went over this file? 

Yes, that was inadvertent. I was reviewing the ediff, and realized I
had not put in the doc for automate stdio options that don't take
values. So I added that, and hit F5 to recompile. But I have that
bound to a 'reformat and run makeinfo' function, and then I did _not_
rerun ediff.

I've added a whitelist feature to that reformat function, so I don't do
this accidently on the next project.

> It may be ok to do that (I haven't compiled anything from the new
> texi file, so I can't say for sure everything is ok), 

I did compile it; no errors. 

> but it would be great if that would happen in a separate patch /
> revision ;)

I'm not clear; should I back out this change and just do the doc
update without the reformat? 

Or leave it as is?

-- 
-- Stephe