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: >> On Tue, May 3, 2016 at 4:37 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: >>> Pranit Bauva <pranit.bauva@xxxxxxxxx> 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@xxxxxxxxx> >>> >>> 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 -- 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