On Tue, May 3, 2016 at 12:19 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > 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. 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". -- 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