On Tue, 2020-03-31 at 18:14 +0300, Vitaly Chikunov wrote:
> Mimi,
>
> On Tue, Mar 31, 2020 at 10:25:24AM -0400, Mimi Zohar wrote:
> > > +# For hard errors
> > > +red_always() {
> > > + echo $@ $RED
> >
> > A few functions - "red_always", "red_if_failure", "color_restore" -
> > use "$@", but none of the function callers pass any parameters. Is
> > there a reason for the "$@" or just something left over from
> > debugging?
>
> It was to pass `-n` I think, but it is never needed in the end.
>
> > > + if [ "$chash" ] && [ "$chash" != "$hash" ]; then
> > > + red_always
> >
> > Only when "ima_hash.test" is invoked directly, the output is colored
> > red. Really confusing.
>
> Non-TTY output is never colored to not clutter log files.
> But logic is to color the errors in red.
>
> So it thought as 'always red', _when_ there is colored output (TTY).
>
> And it's "always" in contract to "red if failure" - which make text
> red only when the test is expected to pass (thus, this is real error
> condition), when the test is expected to fail there is no point to
> color it red, because it's not real error (to not confuse user).
>
> Because it is unconditional (in that sense) is it named "red always".
>
> I can rename it to something like `color_red'. And rename
> `red_if_failure' to `color_red_on_failure'.
Sure, and add a short comment - "Non-TTY output is never colored".
>
> > Nice! The code is very concisely written.
> >
> > Reviewing this patch would be a lot easier, if it was broken up into
> > smaller pieces. For example, and this is only an example, the initial
> > patch could define the base ima_hash.test, a subsequent patch could
> > add coloring for the base ima_hash.test, another patch could introduce
> > "make check" and add its coloring. There's all sorts of ways to break
> > up this patch to simplify review.
>
> This would make following commits to change code which is already
> committed in previous commits.
Nothing is committed yet. I pushed it out in order for others to
review and comment, not only your patches, but mine as well.
> This would make editing code extra hard.
> Especially, when tests was reworked a lot.
>
> Also, I don't think splitting coloring into separate patch improves
> review. Instead, we can just remember the rule that (real) errors are
> going to be printed in red.
>
> For example, if we prefix every error message with word "ERROR:" - why
> it would be easier to review if we split adding this prefix to every
> message in a separate commit?
>
> Red color, similarly to uppercase "ERROR", just improves visibility of
> errors. (Which is useful, because there is really a lot of tests).
Trust me, I understand breaking up patches to simplify review is a lot
of work. It's one of the hardest things to teach newbies and explain
to them why it is necessary and why a clean history is worthwhile, but
you know how to do it. These were just suggestions, just as
separating the two tests was just a suggestion.
BTW, in case it got lost along the way, I really do appreciate your
help.
Thanks!
Mimi
