Re: [Outreachy] [PATCH v4] diff: do not show submodule with untracked files as "-dirty"

From: Sangeeta NB <sangunb09@gmail.com>
Date: Oct 25, 2020, 10:23 UTC
Message-ID: <CAHjREB6fC6j30MH1Lb7enE3KJaGdX--Ft8+hwVeDfwApab5FNQ@mail.gmail.com>
In-Reply-To: <xmqqmu0cfx9a.fsf@gitster.c.googlers.com>

On Sat, Oct 24, 2020 at 2:58 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 40 quoted lines

>
> Sangeeta Jain <sangunb09@gmail.com> 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?

Show 11 quoted lines

> > -     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,