On Sat, Oct 24, 2020 at 2:58 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > Sangeeta Jain <sangunb09@xxxxxxxxx> writes: > > > diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh > > ... > > +test_expect_success 'git diff HEAD with dirty submodule (untracked) (none ignored)' ' > > + git config diff.ignoreSubmodules none && > > + git diff HEAD >actual && > > + sed -e "1,/^@@/d" actual >actual.body && > > + expect_from_to >expect.body $subtip $subprev-dirty && > > + test_cmp expect.body actual.body && > > + git config --unset diff.ignoreSubmodules > > +' > > If any step concatenated by && in the above sequence fails, the test > repository will have diff.ignoreSubmodules set to none (without > getting it reset, since &&-chain prevents the last "config --unset" > step from running). Unless the test is run under the --immediate > option, that would affect the environment in which subsequent tests > are run. > > Instead, our test usually do this: > > test_expect_success 'test title' ' > test_config diff.ignoresubmodules none && > git diff HEAD >actual && > ... > test_cmp expect.body actual.body > ' > > "test_config" sets up a trigger, which will _always_ fire, whether > the test fails at any intermediate steps or runs to the end and > succeeds, to remove the configuration it just added. It is a > short-hand for writing > > test_when_finished test_unconfig diff.ignoresubmodules && > git config diff.ignoresubmodules none > > i.e. to use test_when_finished to set up the trigger. > Oh, okay! I would make these changes. Thanks for the explanation. There are also other places where `git config` is used in test scripts. Should I change that too? Or maybe as they are not relevant to this patch, we can create an issue or even a microproject for other first-timer contributors? > > - git diff HEAD >actual && > > + git diff --ignore-submodules=none HEAD >actual && > > sed -e "1,/^@@/d" actual >actual.body && > > expect_from_to >expect.body $subprev $subprev-dirty && > > test_cmp expect.body actual.body && > > git diff --ignore-submodules=all HEAD >actual2 && > > test_must_be_empty actual2 && > > - git diff --ignore-submodules=untracked HEAD >actual3 && > > + git diff HEAD >actual3 && > > This line can be left as-is, no? Ya, they can be left as it is but I removed the --ignore-submodules=untracked because we also have to check the default git behavior and adding that doesn't change anything in the code so I removed that. In general, in all the tests I followed this practice: 1. I added --ignore-submodules=none where there were no options specified(as this was the earlier default) 2. I removed --ignore-submodules=untracked(as this should even work if no options are specified) I would put that back if you still want it? Thanks and Regards,
