Re: [PATCH] blame: Add tests for -L/start/,/end/

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Jul 19, 2010 at 21:09, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> git-pickaxe (later git-blame) gained support for the -L/start/,/end/
>> form in 2006 (931233bc66 by Junio C Hamano), but nothing was added to
>> test this functionality. Change that by adding more -L tests to
>> t8003-blame.sh.
>
> If we look at the existing tests carefully, there is no tests for the
> range notation -L<start>,<end> in general, not just the regexp variant.  A
> few existing uses are only to limit the output for testing other features
> and take it granted that -L works correctly (meaning they may detect it if
> you break -L implementation but that is merely a side effect).

I didn't submit tests for -L in general because I'm not happy with how
it behaves (as noted in "[BUG?] blame: Odd -L 1,+0 behavior"). So this
patch only tests the regex portion of parse_loc().

I might add tests for -L in general it later, but that patch will
probably include a behavior change.

>> +for comma in '' ','
>> +do
>> +     # The comma in -L/regex/, is optional
>
> Is it just -L/regex/?  I thought -L<start> regardless of the shape of <start>
> was equivalent to -L<start>,END-OF-FILE.

No, it's not just -L/regex/, but I try to in general to write tests
for thing that *might* break, not just things that are guaranteed not
to break as things are now implemented. If blame.c is rewritten in the
future "-L123", might continue to work but "-L/start/," might be
broken as a result of some code shuffling.

>> +test_expect_success 'blame -LINT,/end/' '
>> +     git blame -L3,/GH/ cow  >current 2>errors &&
>
> This is somewhat an interesting one.  It asks for a range that begins at
> the third line in the file, extending to a line after that line that
> matches the given expression.  Unfortunately the test vector does not have
> a line that contains GH before the third line, nor more than one lines
> that contain GH after the third line (we should stop at the first hit), so
> this test is not as effective as it could be.

Yeah, I didn't test for ambiguities like that. I was going to expand
on that later, but see above about not having patched that yet.

>> +test_expect_success 'blame -L,/end/' '
>> +     git blame -L",/^X/" cow >current 2>errors &&
>
> Missing <start> defaults to the beginning of file; the same as -L,INT and
> not specific to the regex variant.

Yup, but I think it should be tested for anyway.

>> +test_expect_success 'blame -L/invalid regex/' '
>> +     # At least GNU, Solaris and FreeBSD (and by extension, Mac OS X)
>> +     # complain about this
>> +     ! git blame -L/[b-a]/ cow >current 2>errors &&
>> +     grep b-a errors &&
>
> The b-a in the error message comes from us, not the regex library, so this
> test should be portable, I think, as long as the library detects the empty
> range correctly.

Yeah, it's just a test for:

    1. Invalid regexes being detected at all, i.e. git not ignoring
       the return value of regcomp().

    2. That we have an error message at all for it, and that the error
        message includes the invalid regex that the user supplied.