Re: [PATCH] diff: diff.context configuration gives default to -U

[Jeff King <peff@peff.net>]

On Thu, Sep 27, 2012 at 10:40:26AM -0700, Junio C Hamano wrote:

> > +test_expect_success 'diff.context affects log' '
> > +	git log -1 -p | grep -q -v firstline
> > +	git config diff.context 8 &&
> > +	git log -1 -p | grep -q firstline
> > +'
> 
> Three points:
> 
>  - Please avoid "grep -q", which does not help people who ran tests
>    (the output is hidden by default) and hurts people who want to
>    debug tests.
> 
>  - Your test will ignore breakage from the first "log 1" output and
>    goes on running "git config".  Make sure you got your && cascades
>    right.
> 
>  - Because an error from the command on the upstream side of the
>    pipe is ignored, we tend to prefer writing things like this:
> 
> 	git log -n 1 -p >output &&
>         grep -v firstline output &&

I agree with all of that. But also, is "grep -v" the right thing? I
think the intent of the test is "firstline does not appear". But that is
not what  "grep -v" will tell you. It will tell you whether any line
that did not have "firstline" in it was shown (which it would be, since
there are a bunch of other lines shown).

I think "! grep firstline" is what is needed here. Or even just
explicitly matching the diff that we expect via test_cmp. I like the
latter much better anyway, as a failure will show exactly what is wrong.
Whereas if the grep ends up not matching, there is no helpful output for
somebody reading the test.

We already produce nice messages from things like test_must_fail. Maybe
it would be nice to have:

  test_contains () {
          if ! grep "$1" "$2"; then
	          echo >&2 "File '$2' does not contain a line with '$1'"
		  return 1
	  fi
  }

and likewise a "test_not_contains" or something to negate it. That makes
both the tests and their failure output readable.

-Peff