Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

[Pranit Bauva <pranit.bauva@gmail.com>]

On Tue, May 3, 2016 at 10:42 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:
>> On Tue, May 3, 2016 at 4:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Pranit Bauva <pranit.bauva@gmail.com> writes:
>>>> Variable named 'verbose' in builtin/commit.c is consumed by git-status
>>>> and git-commit so if a new verbose related behavior is introduced in
>>>> git-commit, then it should not affect the behavior of git-status.
>>>>
>>>> One previous commit (title: commit: add a commit.verbose config
>>>> variable) introduced a new config variable named commit.verbose,
>>>> so care should be taken that it would not affect the behavior of
>>>> status.
>>>>
>>>> Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
>>>> respect "unspecified" values") changes the initial value of verbose
>>>> from 0 to -1. This can cause git-status to display a verbose output even
>>>> when it isn't supposed to.
>>>>
>>>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>>
>>> If these are documenting what your previous patches broke, then
>>> there test body should describe what should happen, and then if it
>>> is broken, use test_expect_failure, no?
>>>
>>> Your first test does "run status with commit.verbose is set, and
>>> make sure the "diff --git" does not appear", which is correct, so if
>>> it does not work, test_expect_failure would be the right thing to
>>> use.
>>>
>>> These, especially the latter, look rather unpleasant regressions to
>>> me, and the main commit.verbose change would need to be held back
>>> before they are fixed.
>>
>> I agree that using test_expect_failure would be a better way of going
>> with this thing. Thanks. Will send an updated patch for this.
>
> Please don't. test_expect_failure() is not warranted.

I got confused between test_must_fail and test_expect_failure. I
thought Junio mentioned to use test_must_fail and remove the " ! "
sign.

> Step back a moment and recall why these tests were added. Earlier
> rounds of this series were buggy and caused regressions in git-status.
> As a consequence, reviewers suggested[1,2] that you improve test
> coverage to ensure that such breakage is caught early.
>
> The problems which caused the regressions were addressed in later
> versions of the series, thus using test_expect_success() is indeed
> correct, whereas test_expect_failure(), which illustrates broken
> behavior, would be the wrong choice.
>
> The point of these new tests is to prevent regressions caused by
> *subsequent* changes, which is why it was suggested that these tests
> be added early (as a "preparatory patch"[3]), not at the very end of
> the series as done here in v15.
>
> This patch's commit message is perhaps a bit too detailed about what
> could have gone wrong in earlier patches in this series; indeed, it
> misled Junio into thinking that patches in this series did break
> behavior, when in fact, it was instead previous rounds of this series
> which were buggy. If you instead make this a preparatory patch[3],
> then you can sell it more simply by explaining that git-commit and
> git-status share implementation (without necessarily going into detail
> about exactly what is shared), and that you're improving test coverage
> to ensure that changes specific to git-commit don't accidentally
> impact git-status, as well.

Sure! I just wanted the commit message to be detailed as per the
guidelines given by SubmittingPatches. I will swap the patch 6/7 and
patch 7/7 changing the commit message. Also I will make the commit
message less detailed.

>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/288634/focus=288648
> [2]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289730
> [3]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=291468