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

On Tue, May 3, 2016 at 12:19 PM, Eric Sunshine [off-list ref] wrote:

On Tue, May 3, 2016 at 2:42 AM, Pranit Bauva [off-list ref] wrote:

quoted

On Tue, May 3, 2016 at 10:42 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva [off-list ref] wrote:

quoted

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.

quoted

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.

This patch should be inserted before 4/7 since it needs to protect
against breakage which might occur when 4/7 changes the behavior of
OPTION_COUNTUP.

I forgot to mention about this earlier. When I was rebasing, this stroked me.
I guess making any changes in ordering the commits will make one of
the test as absurd. One of the test uses a configuration variable
'commit.verbose' will won't be effective before the patch 6/7. So I
guess I will have to only change the commit message to reflect as
"improving test coverage".