Thomas Keller <[EMAIL PROTECTED]> 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 mailing list [email protected] http://lists.nongnu.org/mailman/listinfo/monotone-devel
