Re: [Monotone-devel] automate log

[Stephen Leake]

Thomas Keller <address@hidden> writes:

> A few objections:
>
> 1) in monotone.texi / cmd_diff_log.cc:
>
>   address@hidden address@hidden address@hidden [...]]
>   address@hidden [...]] [--no-merges] [--no-files]
>
>   If only revision ids are outputted, the --no-files option is bogus

Ah. From the description, I assumed it was a rev filter, the opposite of
--no-merges. But looking at the code, I see it means "no revision
summary".

> 2) in monotone.texi:
>
>   +These messages are in the 'm' stream in automate stdio.
>
>   All the normal output for every automate command goes to this stream.
>   Since we do not mention it anywhere else explicitely, we shouldn't do
>   that here either.

Ok.

> 3) in cmd_diff_log.cc:
>
>   +void
>   +log_print_rev (app_state &      app,
>   +               database &       db,
>   +               project_t &      project,
>   +               revision_id      rid,
>   +               revision_t &     rev,
>   +               string           date_fmt,
>   +               node_restriction mask,
>   +               bool             automate,
>                 ^^^^
>
>   Please use an enum for this and later uses.

I prefer a bool, for a couple reasons:

- I'm not clear what the other name should be (note your example below
  doesn't give one). "automate" and "not automate" are clear in this
  context.

- When there's an enum, I have to wonder how many choices there are, so
  case statements are required. That seems overkill for two choices.
  Your example uses an 'if', so a bool is more appropriate.

There are certainly many other places in mtn that use bool for similar
choices.

>   Also, because log_print_rev is only used twice and the use is
>   determined with this
>
>   if (automate)
>     log_print_rev (app, db, project, rid, rev, date_fmt, mask, \
>                    automate, output);
>   else
>     ostringstream out;
>     log_print_rev (app, db, project, rid, rev, date_fmt, mask, \
>                    automate, out);
>
>   The code could be simplified like this:
>
>   if (target == automate_log)
>     out << rid << "\n";
>   else
>     {
>        // the former code which prints everything else ...
>     }

Yes, that is cleaner. But I'll keep log_print_rev for the else branch;
that makes it easier to see the logic flow. Comparing either version to
main requires telling ediff to focus on regions.

> 4) You have quite a lot whitespace changes

The only way to fully eliminate whitespace changes is for every file to
follow a standard convention. I have emacs set to enforce what I
perceive to be the monotone convention, so it fixes files that I edit.

It's easy to ignore whitespace changes in a good diff tool, so I don't
see this as a problem.

> / unrelated changes e.g. in testlib.lua and a couple of others. Some
> of them seem to be needed for your current work,

That was to help with debugging this test. The commit log gives a clear
explanation.

> but in general please try to keep the patch at a minimum and commit
> unrelated things directly to mainline next time.

Ok, I'll keep that in mind.

Thanks for your review. Changes synced.

--
-- Stephe