On Tue, 14 Aug 2018 13:16:38 -0700 Junio C Hamano <gits...@pobox.com> wrote:
> Antonio Ospite <a...@ao2.it> writes: > [...] > > test_expect_success 'error message contains blob reference' ' > > + # Remove the error introduced in the previous test. > > + # It is not needed in the following tests. > > + test_when_finished "git -C super reset --hard HEAD^" && > > Hmm, that is ugly. Depending on where in the subshell the previous > test failed, you'd still be taking us to an unexpected place. > Imagine if "git commit -m 'add error'" failed, for example, in the > test before this one. > > I am wondering if the proper fix is to merge the previous one and > this one into a single test. The combined test would > > - remember where the HEAD in super is and arrange to come back > to it when test is done > - break .gitmodules and commit it > - run test-tool and check its output > - also check its error output > > in a single test_expect_success. > I will try that. > > @@ -123,6 +126,7 @@ test_expect_success 'using different treeishs works' ' > > ' > > > > test_expect_success 'error in history in fetchrecursesubmodule lets > > continue' ' > > + test_when_finished "git -C super reset --hard HEAD^" && > > (cd super && > > git config -f .gitmodules \ > > submodule.submodule.fetchrecursesubmodules blabla && > > @@ -134,8 +138,7 @@ test_expect_success 'error in history in > > fetchrecursesubmodule lets continue' ' > > HEAD b \ > > HEAD submodule \ > > >actual && > > - test_cmp expect_error actual && > > - git reset --hard HEAD^ > > + test_cmp expect_error actual > > ) > > ' > > If we want to be more robust, you'd probably need to find a better > anchoring point than HEAD, which can be pointing different commit > depending on where in the subshell the process is hit with ^C, > i.e. > > ORIG=$(git -C super rev-parse HEAD) && > test_when_finished "git -C super reset --hard $ORIG" && > ( > cd super && > ... > I see, ORIG is set and evaluated immediately but the value will be used only at a later time. I remember that you raised concerns also in the previous review round but I didn't quite get what you meant, now I think I do. > The patch is still an improvement compared to the current code, > where a broken test-tool that does not produce expected output in > the file 'actual' is guaranteed to leave us at a commit that we do > not expect to be at, but not entirely satisfactory. I can do a v4 with these fixes since there are also some comments about other patches. 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?
