Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up
On Thu, 2 Aug 2018 11:15:03 -0700 Stefan Beller wrote: > On Thu, Aug 2, 2018 at 9:41 AM SZEDER Gábor wrote: > > [...] > > > > > > Note that test_when_finished is not used here, both to keep the current > > > style > > > and also because it does not work in sub-shells. > > > > That's true, but I think that this: > > > > test_when_finished git -C super reset --hard HEAD~2 > > > > at the very beginning of the test should work. > > Yeah that is a better way to do it. > Even better would be to have 2 of these for both tests 5 and 8, > such that each of them could be skipped individually and any following > tests still work fine. > Test 6 also relies on the error introduced in test 5. So the options would be either to remove one commit at the time in test 6 and 8 (with a comment in test 6 to note that the commit is from the previous test), or to remove both the commits in test 8. I am going to go with the former, using test_when_finished. > > > t/t7411-submodule-config.sh | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh > > > index 0bde5850ac..248da0bc4f 100755 > > > --- a/t/t7411-submodule-config.sh > > > +++ b/t/t7411-submodule-config.sh > > > @@ -135,7 +135,9 @@ test_expect_success 'error in history in > > > fetchrecursesubmodule lets continue' ' > > > HEAD submodule \ > > > >actual && > > > test_cmp expect_error actual && > > > - git reset --hard HEAD^ > > > + # Remove both the commits which add errors to .gitmodules, > > > + # the one from this test and the one from a previous test. > > > + git reset --hard HEAD~2 > > I am a bit hesitant to removing the commits though, as it is expected to have > potentially broken history and submodules still working. > The commits which are removed only affected .gitmoudles, no "submodule init" nor "submoudle update" is ever called after they are added, so I don't know what problems there could be. Would a revert be any different? > The config --unset already fixes the gitmodules file, > so I think we can rather do > > git commit -a -m 'now the .gitmodules file is fixed at HEAD \ > but has a messy history' > > But as I have only read up to here, not knowing what the future tests will > bring this is all speculation at this point. IIUC the "config --unset" is used to cause the error, not to fix it, I am not sure I understand this point. Thanks, Antonio -- Antonio Ospite https://ao2.it https://twitter.com/ao2it A: Because it messes up the order in which people normally read text. See http://en.wikipedia.org/wiki/Posting_style Q: Why is top-posting such a bad thing?
Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up
SZEDER Gábor writes: >> Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with >> invalid lines in .gitmodules but then only the second commit is removed. >> >> This may affect future subsequent tests if they assume that the >> .gitmodules file has no errors. >> >> Since those commits are not needed anymore remove both of them. >> >> The modified line is in the last test of the file, so this does not >> change the current behavior, it only affects future tests. >> >> Signed-off-by: Antonio Ospite >> --- >> >> Note that test_when_finished is not used here, both to keep the current style >> and also because it does not work in sub-shells. > > That's true, but I think that this: > > test_when_finished git -C super reset --hard HEAD~2 > > at the very beginning of the test should work. Assuming that the operations to create these two extra commits always succeed, yes, that would be a more robust and preferrable option. I don't know if that assumption hold true offhand, though. Don't we have a more stable point to anchor that going-back-to commit to?
Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up
On Thu, Aug 2, 2018 at 9:41 AM SZEDER Gábor wrote: > > > Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with > > invalid lines in .gitmodules but then only the second commit is removed. > > > > This may affect future subsequent tests if they assume that the > > .gitmodules file has no errors. > > > > Since those commits are not needed anymore remove both of them. > > > > The modified line is in the last test of the file, so this does not > > change the current behavior, it only affects future tests. > > > > Signed-off-by: Antonio Ospite > > --- > > > > Note that test_when_finished is not used here, both to keep the current > > style > > and also because it does not work in sub-shells. > > That's true, but I think that this: > > test_when_finished git -C super reset --hard HEAD~2 > > at the very beginning of the test should work. Yeah that is a better way to do it. Even better would be to have 2 of these for both tests 5 and 8, such that each of them could be skipped individually and any following tests still work fine. > > t/t7411-submodule-config.sh | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh > > index 0bde5850ac..248da0bc4f 100755 > > --- a/t/t7411-submodule-config.sh > > +++ b/t/t7411-submodule-config.sh > > @@ -135,7 +135,9 @@ test_expect_success 'error in history in > > fetchrecursesubmodule lets continue' ' > > HEAD submodule \ > > >actual && > > test_cmp expect_error actual && > > - git reset --hard HEAD^ > > + # Remove both the commits which add errors to .gitmodules, > > + # the one from this test and the one from a previous test. > > + git reset --hard HEAD~2 I am a bit hesitant to removing the commits though, as it is expected to have potentially broken history and submodules still working. The config --unset already fixes the gitmodules file, so I think we can rather do git commit -a -m 'now the .gitmodules file is fixed at HEAD \ but has a messy history' But as I have only read up to here, not knowing what the future tests will bring this is all speculation at this point.
Re: [RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up
> Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with > invalid lines in .gitmodules but then only the second commit is removed. > > This may affect future subsequent tests if they assume that the > .gitmodules file has no errors. > > Since those commits are not needed anymore remove both of them. > > The modified line is in the last test of the file, so this does not > change the current behavior, it only affects future tests. > > Signed-off-by: Antonio Ospite > --- > > Note that test_when_finished is not used here, both to keep the current style > and also because it does not work in sub-shells. That's true, but I think that this: test_when_finished git -C super reset --hard HEAD~2 at the very beginning of the test should work. > t/t7411-submodule-config.sh | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh > index 0bde5850ac..248da0bc4f 100755 > --- a/t/t7411-submodule-config.sh > +++ b/t/t7411-submodule-config.sh > @@ -135,7 +135,9 @@ test_expect_success 'error in history in > fetchrecursesubmodule lets continue' ' > HEAD submodule \ > >actual && > test_cmp expect_error actual && > - git reset --hard HEAD^ > + # Remove both the commits which add errors to .gitmodules, > + # the one from this test and the one from a previous test. > + git reset --hard HEAD~2 > ) > ' > > -- > 2.18.0 > >
[RFC PATCH v2 03/12] t7411: be nicer to future tests and really clean things up
Tests 5 and 8 in t/t7411-submodule-config.sh add two commits with invalid lines in .gitmodules but then only the second commit is removed. This may affect future subsequent tests if they assume that the .gitmodules file has no errors. Since those commits are not needed anymore remove both of them. The modified line is in the last test of the file, so this does not change the current behavior, it only affects future tests. Signed-off-by: Antonio Ospite --- Note that test_when_finished is not used here, both to keep the current style and also because it does not work in sub-shells. t/t7411-submodule-config.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/t/t7411-submodule-config.sh b/t/t7411-submodule-config.sh index 0bde5850ac..248da0bc4f 100755 --- a/t/t7411-submodule-config.sh +++ b/t/t7411-submodule-config.sh @@ -135,7 +135,9 @@ test_expect_success 'error in history in fetchrecursesubmodule lets continue' ' HEAD submodule \ >actual && test_cmp expect_error actual && - git reset --hard HEAD^ + # Remove both the commits which add errors to .gitmodules, + # the one from this test and the one from a previous test. + git reset --hard HEAD~2 ) ' -- 2.18.0
