[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Monotone-devel] options for automate inventory
From: |
Stephen Leake |
Subject: |
Re: [Monotone-devel] options for automate inventory |
Date: |
Tue, 15 Jan 2008 04:31:46 -0500 |
User-agent: |
Gnus/5.1006 (Gnus v5.10.6) Emacs/22.1 (windows-nt) |
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
- [Monotone-devel] options for automate inventory, Stephen Leake, 2008/01/13
- Re: [Monotone-devel] options for automate inventory, Thomas Keller, 2008/01/13
- Re: [Monotone-devel] options for automate inventory, William Uther, 2008/01/13
- Re: [Monotone-devel] options for automate inventory, Thomas Keller, 2008/01/14
- Re: [Monotone-devel] options for automate inventory,
Stephen Leake <=
- Re: [Monotone-devel] options for automate inventory, Thomas Keller, 2008/01/15
- [Monotone-devel] Re: options for automate inventory, Lapo Luchini, 2008/01/15
- Re: [Monotone-devel] options for automate inventory, Stephen Leake, 2008/01/15
- Re: [Monotone-devel] options for automate inventory, Nathaniel Smith, 2008/01/15
- Re: [Monotone-devel] options for automate inventory, Stephen Leake, 2008/01/15
Re: [Monotone-devel] options for automate inventory, Thomas Moschny, 2008/01/14