> > -test_create_repo sm1 && > > -add_file . foo >/dev/null > > - > > -head1=$(add_file sm1 foo1 foo2) > > -fullhead1=$(cd sm1; git rev-parse --verify HEAD) > > +test_expect_success 'setup' ' > > + test_create_repo sm1 && > > + add_file . foo >/dev/null && > > Now this is inside test_expect_success, redirection to /dev/null is > unnecessary. ack. > > > + head1=$(add_file sm1 foo1 foo2) && > > + fullhead1=$(cd sm1 && git rev-parse --verify HEAD) > > +' > > Or "fullhead1=$(git -C sm1 rev-parse ...)". > > Both of the above can be ignored if we are trying to be a strict > rewrite of the original, but moving code inside test_expect_success > block is a large enough change that there may not be much point in > avoiding such an obvious modernization "while at it". I agree, will fix. > > > -rm sm2 > > -mv sm2-bak sm2 > > - > > test_expect_success 'setup nested submodule' ' > > + rm sm2 && > > + mv sm2-bak sm2 && > > To me, this looks more like something test_when_finished in the test > that wanted not to have sm2 (i.e. "deleted submodule with .git file") > should have done as part of its own clean-up. > > There certainly can be two schools of thought when it comes to how to > arrange the precondition of subsequent tests. More modern tests tend > to clean after themselves by reverting the damage they made to the > environment inside test_when_finished in themselves. This one, as > the posted patch does, goes to the other extreme and forces the > subsequent test to undo the damage done by the previous ones. > > The latter approach has two major downsides. It would not work if > the tester wants to skip an earlier step, or if an earlier step > failed to cause the expected damage this step wants to undo. The > correctness of "what we should see as sm2 here must be in sm2-bak > because we know an earlier step should have moved it there" can > easily be broken. It also makes it harder to update the earlier > step to cause different damage to the environment---the "undoing the > damage done by the previous step(s)" done as early parts of this > step also needs to be updated. > > Whichever approach we pick to use in each script, it would be better > to stick to one philosophy, and if we can make each step revert the > damage it caused when it is done, that would be nice. > > > -mv sm2-bak sm2 > > +test_expect_success 'submodule cleanup' ' > > + mv sm2-bak sm2 > > +' > > > > test_done > > Likewise. ack. Swapping to test_when_finished
