Re: [PATCH v3 4/6] add a test for semantic errors in config files
Tanay Abhra writes: > On 7/28/2014 4:52 PM, Matthieu Moy wrote: >> Tanay Abhra writes: >> >>> +test_expect_success 'check line errors for malformed values' ' >>> + mv .git/config .git/config.old && >>> + test_when_finished "mv .git/config.old .git/config" && >>> + cat >.git/config <<-\EOF && >>> + [alias] >>> + br >>> + EOF >>> + test_expect_code 128 git br 2>result && >>> + grep "fatal: bad config file line 2 in .git/config" result >>> +' >> >> This is PATCH 4, and it tests a bug fixed in PATCH 1. It would have >> eased review to group both patches, either >> >> PATCH 1: introduce test_expect_failure test to demonstrate the failure > > Didn't Junio comment that he wouldn't recommend inserting a > test_expect_failure > for new tests and then flipping them after in the series. No, he even said it was good practice: http://thread.gmane.org/gmane.comp.version-control.git/254101/focus=254104 his point was to avoid breaking something and repairing in another patch (which your initial series was doing because the test patch was coming between "rewrite git_config() to use the config-set API" and "add line number and file name info to `config_set`"). The situation is different when you have a pre-existing bug. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/6] add a test for semantic errors in config files
On 7/28/2014 4:52 PM, Matthieu Moy wrote: > Tanay Abhra writes: > >> +test_expect_success 'check line errors for malformed values' ' >> +mv .git/config .git/config.old && >> +test_when_finished "mv .git/config.old .git/config" && >> +cat >.git/config <<-\EOF && >> +[alias] >> +br >> +EOF >> +test_expect_code 128 git br 2>result && >> +grep "fatal: bad config file line 2 in .git/config" result >> +' > > This is PATCH 4, and it tests a bug fixed in PATCH 1. It would have > eased review to group both patches, either > > PATCH 1: introduce test_expect_failure test to demonstrate the failure Didn't Junio comment that he wouldn't recommend inserting a test_expect_failure for new tests and then flipping them after in the series. > PATCH 2: fix the bug and change test_expect_failure to test_expect_success > > Or putting both in the same patch. > Much better, thanks for the advice. > I think the series is OK like this, my comment is just to be read as > "next time, here's how to do better". > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/6] add a test for semantic errors in config files
Tanay Abhra writes: > +test_expect_success 'check line errors for malformed values' ' > + mv .git/config .git/config.old && > + test_when_finished "mv .git/config.old .git/config" && > + cat >.git/config <<-\EOF && > + [alias] > + br > + EOF > + test_expect_code 128 git br 2>result && > + grep "fatal: bad config file line 2 in .git/config" result > +' This is PATCH 4, and it tests a bug fixed in PATCH 1. It would have eased review to group both patches, either PATCH 1: introduce test_expect_failure test to demonstrate the failure PATCH 2: fix the bug and change test_expect_failure to test_expect_success Or putting both in the same patch. I think the series is OK like this, my comment is just to be read as "next time, here's how to do better". -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
