Re: [PATCH v8 1/2] ima-evm-utils: Add some tests for evmctl

[Mimi Zohar <zohar@linux.ibm.com>]

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