Re: [PATCH v7 1/6] t5323: test cases for git-pack-redundant

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 1, 2019, 06:11 UTC
Message-ID: <CAPig+cS84BOPdrZ+e1Z9ystx6SDyON0MJxVySQyxYE=LYcNRdg@mail.gmail.com>
In-Reply-To: <CANYiYbG5vbV1bqbHu-u19XFy7JTpcopg0s=JWAhJBRNGvP7j3g@mail.gmail.com>

On Fri, Feb 1, 2019 at 12:44 AM Jiang Xin <worldhello.net@gmail.com> wrote:

Show 12 quoted lines

>> Junio C Hamano <gitster@pobox.com> 于2019年2月1日周五 上午5:44写道：
> > Move this outside loop, not for efficiency but for clarity. This
> > helper function creates a single empty tree and bunch of commits
> > that hold the same empty tree, arranged as a single strand of
> > pearls.
>
> Will rewrite as:
>
>     create_commits () {
>             parent=
>             T=$(git write-tree) &&
>             for name in A B C D E F G H I J K L M N O P Q R

Don't forget the && at the end of the 'parent=' line to protect against someone later adding code above that line. So:

    create_commits () {
        parent= &&
        T=$(git write-tree) &&
        ...

Show 20 quoted lines

> Nice chart, will edit test_description as follows:
>
>     test_description='git pack-redundant test
>
>     In order to test git-pack-redundant, we will create a number of
> redundant
>     packs in the repository `master.git`. The relationship between
> packs (P1-P8)
>     and objects (T,A-R) is show in the following chart:
>
>            | T A B C D E F G H I J K L M N O P Q R
>         ---+--------------------------------------
>         P1 | x x x x x x x                       x
>         P2 |     x x x x   x x x
>         P3 |             x     x x x x x
>         P4 |                     x x x x     x
>         P5 |               x x           x x
>         P6 |                             x x   x
>         P7 |                                 x x
>         P8 |   x

test_description should be a meaningful one-liner; it should not contain this other information, but this information should appear as comments in the test script.

>     Another repoisitory `shared.git` has unique objects (X-Z), while
> share others

s/repoisitory/repository/

Show 13 quoted lines

> > > +test_expect_success 'setup master.git' '
> > > +     git init --bare master.git &&
> > > +     cd master.git &&
> > > +     create_commits
> > > +'
> >
> > Everything below will be done inside master.git?  Avoid cd'ing
> > around in random places in the test script, as a failure in any of
> > the steps that does cd would start later tests in an unexpected
> > place, if you can.
>
> The first 10 test cases will run inside master.git, and others will
> run inside shared.git.  Only run cd inside the two `setup` test cases.

That's not what Junio meant. It's okay for tests to 'cd', but each test which does so _must_ ensure that the 'cd' is undone at the end of the test, even if the test fails. The correct way to do this within each test is by using 'cd' in a subhsell, like this:

    test_expect_success 'setup master.git' '
        git init --bare master.git &&
        (
            cd master.git &&
            create_commits
        )
    '

Then, each test which needs to use "master.git" would 'cd' itself, like this:

    test_expect_success 'some test' '
        (
            cd master.git &&
            ...
        )
    '

Show 7 quoted lines

> > > +test_expect_success 'setup shared.git' '
> > > +     cd "$TRASH_DIRECTORY" &&
> > > +     git clone -q --mirror master.git shared.git &&
> >
> > Why "-q"?
>
> To make verbose output cleaner.

What Junio really meant by asking that question was that you should not do this. When something goes wrong with a test, we want as much output as possible to help diagnose the problem, so suppressing output is undesirable. To summarize, don't use -q, --no-progress, or any other such option and don't redirect to /dev/null.