Re: [PATCH v2 10/16] tag: change misleading --list <pattern> documentation

From

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

Date

Mar 22, 2017, 23:43 UTC

Message-ID

<CACBZZX7kJ_G8mAYd3mN5WtP0ZLUUOuWs4hu1fhTSspWuW_O=5A@mail.gmail.com>

In-Reply-To

<20170322223600.huyel6j5wl644ddf@sigill.intra.peff.net>

On Wed, Mar 22, 2017 at 11:36 PM, Jeff King <peff@peff.net> wrote:

> On Wed, Mar 22, 2017 at 03:26:21PM -0700, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>> > of things you think we should be putting in the test suite. I.e.
>> > should the tests be:
>> >
>> > a) Only be a collection of invocations of git we'd be comfortable
>> > showing to someone as "this works, and this is how you should do it",
>> > or things that explicitly fail marked with test_must_fail.
>> >
>> > b) or a) && also various surprising combinations of things we don't
>> > necessarily want to encourage or even support in the future, but which
>> > are in there so if we change them, we at least know our change changed
>> > something that worked before.
>>
>> I am strongly inclined to (a).  If we cannot decide when we designed
>> the feature, and we anticipate that we may want to change it later,
>> then documenting the choice in a test or two may be a way to remind
>> the choice we happened to have made, but in general I do not think
>> we want to promise (to ourselves) more than what we are willing to
>> commit to.
>
> I've occasionally[1] added tests that are "what we happen to produce
> now", but I almost always mark them with a comment either in the test
> script or in the commit message.  What I'm _most_ concerned about is a
> developer later breaking the test, but being unsure if they were
> breaking some real-world case (and not being able to find clues in the
> history).
>
> A secondary concern would be people using the test snippets as guidance
> on what is normal or encouraged.
>
> So I could live with these patches, but I'd prefer to see a comment
> somewhere. And I think I'd have a slight inclination to just stick to
> (a) in the first place, unless there is a really good reason to cover
> the test (like that we do not care between behaviors X and Y, but we
> need to check that it does one of them, and not Z).

diff --git a/t/README b/t/README
index 4982d1c521..9e079a360a 100644
--- a/t/README
+++ b/t/README
@@ -379,2 +379,5 @@ Do:

+ - Include tests which assert that the desired & recommended behavior
+   of commands is preserved.
+
  - Put all code inside test_expect_success and other assertions.
@@ -424,2 +427,17 @@ Don't:

+ - Include tests which exhaustively test for various edge cases or
+   unintended emergent behavior which we're not interested in
+   supporting in the future.
+
+   An exception to this is are cases where we don't care about
+   different behaviors X and Y, but we need to check that it does one
+   of them, and not Z.
+
+   Another exception are cases where our documentation might
+   unintentionally stated or implied that something was supported or
+   recommended, but we'd like to discourage its use going forward.
+
+   In both of the above cases please prominently comment the test
+   indicating that you're testing for one of these two cases.
+
  - exit() within a <script> part.
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 0a7ebf5358..35402ad9a0 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -350,2 +350,6 @@ test_expect_success 'tag -l can accept multiple patterns' '

+# Between around v1.7.6.1 & v2.13.0 the documentation unintentionally
+# implied that --list was what took the <pattern>, not that patterns
+# should be clustered at the very end. This test should not imply that
+# this is a sane thing to support.
 test_expect_success 'tag -l can accept multiple patterns interleaved
with -l or --list options' '

Or do you think the "long documented but unintentional" argument isn't
worth a test, in which case squash this:

diff --git a/t/README b/t/README
index 9e079a360a..9f85b8d1cd 100644
--- a/t/README
+++ b/t/README
@@ -433,12 +433,8 @@ Don't:
    different behaviors X and Y, but we need to check that it does one
    of them, and not Z.

-   Another exception are cases where our documentation might
-   unintentionally stated or implied that something was supported or
-   recommended, but we'd like to discourage its use going forward.
-
-   In both of the above cases please prominently comment the test
-   indicating that you're testing for one of these two cases.
+   In that case please prominently comment the test indicating that
+   you're testing for one of these two cases.

  - exit() within a <script> part.

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 35402ad9a0..83772f6003 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -348,19 +348,6 @@ test_expect_success 'tag -l can accept multiple patterns' '
        test_cmp expect actual
 '

-# Between around v1.7.6.1 & v2.13.0 the documentation unintentionally
-# implied that --list was what took the <pattern>, not that patterns
-# should be clustered at the very end. This test should not imply that
-# this is a sane thing to support.
-test_expect_success 'tag -l can accept multiple patterns interleaved
with -l or --list options' '
-       git tag -l "v1*" "v0*" >actual &&
-       test_cmp expect actual &&
-       git tag -l "v1*" --list "v0*" >actual &&
-       test_cmp expect actual &&
-       git tag -l "v1*" "v0*" -l --list >actual &&
-       test_cmp expect actual
-'
-
 test_expect_success 'listing tags in column' '
        COLUMNS=40 git tag -l --column=row >actual &&
        cat >expected <<\EOF &&