From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 23 Mar 2017 00:46:11 GMT
Subject: Re: [PATCH v2 10/16] tag: change misleading --list <pattern> documentation
Message-ID: <xmqqd1d8hlbg.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CACBZZX7kJ_G8mAYd3mN5WtP0ZLUUOuWs4hu1fhTSspWuW_O=5A@mail.gmail.com>

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

> Junio would you be fine with just this on top:
>
> 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.

This would probably be part of your other three-patch series for
t/README?  With a quick read-through I spotted nothing questionable,
but I am not 100% sure about the value of going into that level of
details, especially with the latter one about "discourage and wean
off of".

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

Oh, I didn't know I was given two choices and the second one does
address the "I am not 100% sure" above ;-).

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

So for the doc part, I can go with either, but prefer the latter
between the two.

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

I'd actually think "multiple patterns" should be kept, as that is
really what we want to support forever.  

Acceptance of multiple and redundant "--list", e.g.

	git tag -l --list "v1*" "v0*" >actual &&
	test_cmp expect actual &&

immediately after the above may also be something worth protecting,
but that is how OPT_CMDMODE works in general, so it may not be
necessary to test it specifically in a test for "tag -l".

Acceptance of multiple and redundant "--list" that are given after a
<pattern> is already given on the command line is not something we
want to actively deprecate (it is not worth our time or effort) but
it is not something we want to encourage, either.  So I have a
preference for dropping the ones below.

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