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

From: Jeff King <peff@peff.net>
Date: Sep 27, 2012, 18:43 UTC
Message-ID: <20120927184309.GC2519@sigill.intra.peff.net>
In-Reply-To: <7vr4pnqs8l.fsf@alter.siamese.dyndns.org>

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

Show 21 quoted lines

> > +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