On Tue, May 3, 2016 at 2:42 AM, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote: > On Tue, May 3, 2016 at 10:42 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: >> On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote: >>> 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. 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html