Re: [PATCH v3 1/3] t5323: test cases for git-pack-redundant

From: Jiang Xin <worldhello.net@gmail.com>
Date: Jan 10, 2019, 03:28 UTC
Message-ID: <CANYiYbGqLHr-t+f6m6gyY3QiYgxbzbqsmmRw-afKe6NG_mxhPQ@mail.gmail.com>
In-Reply-To: <20190109125628.GG4673@szeder.dev>

SZEDER Gábor <szeder.dev@gmail.com> 于2019年1月9日周三 下午8:56写道：

Show 6 quoted lines

>
> On Wed, Jan 02, 2019 at 12:34:54PM +0800, Jiang Xin wrote:
> > From: Jiang Xin <zhiyou.jx@alibaba-inc.com>
> > +test_description='git redundant test'
>
> s/redundant/pack-redundant/ ?

Yes, will correct it.

Show 19 quoted lines

> > +
> > +. ./test-lib.sh
> > +
> > +create_commits()
> > +{
> > +     set -e
> > +     parent=
> > +     for name in A B C D E F G H I J K L M
> > +     do
> > +             test_tick
> > +             T=$(git write-tree)
> > +             if test -z "$parent"
> > +             then
> > +                     sha1=$(echo $name | git commit-tree $T)
>
> There is a considerable effort going on to switch from SHA-1 to a
> different hash function, so please don't add any new $sha1 variable;
> call it $oid or $commit instead.
>

Will do.

Show 7 quoted lines

> > +
> > +# Create commits and packs
> > +create_commits
> > +create_redundant_packs
>
> Please perform all setup tasks in a test_expect_success block, so we
> get verbose and trace output about what's going on.

Will do like this:

    test_expect_success 'setup' '
            create_commits  &&
            create_redundant_packs
    '

Show 9 quoted lines

> Don't use 'set -e', use an &&-chain instead.  To fail the test if a
> command in the for loop were to fail you could do something like this:
>
>   for ....
>   do
>     do-this &&
>     do-that ||
>     return 1
>   done

Will do.

Show 11 quoted lines

> > +
> > +test_expect_success 'clear loose objects' '
> > +     git prune-packed &&
> > +     test $(find .git/objects -type f | grep -v pack | wc -l) -eq 0
>
> Use something like
>
>   find .git/objects -type f | grep -v pack >out &&
>   test_must_be_empty out
>
> instead, so we get an informative error message on failure.

if `grep -v pack` return empty output, it will return error, so I will use `sed -e "/objects\/pack\//d" >out` instead.

Show 22 quoted lines

>
> > +'
> > +
> > +cat >expected <<EOF
> > +P1:$P1
> > +P4:$P4
> > +P5:$P5
> > +P6:$P6
> > +EOF
> > +
> > +test_expect_success 'git pack-redundant --all' '
> > +     git pack-redundant --all | \
>
> Don't run a git command (especially the particular command the test
> script focuses on) upstream of a pipe, because it hides the command's
> exit code.  Use an intermediate file instead.
>
> > +             sed -e "s#^.*/pack-\(.*\)\.\(idx\|pack\)#\1#g" | \
>
> This sed command doesn't seem to work on macOS (on Travis CI), and
> causes the test to fail with:
>

It works if rewrite as follows:

    git pack-redundant --all >out &&
    sed -E -e "s#.*/pack-(.*)\.(idx|pack)#\1#" out | \

Without `-E`, MasOS has to write two seperate sed commands, such as:

    git pack-redundant --all >out &&
    sed -e "s#.*/pack-\(.*\)\.idx#\1#" out | \
    sed -e "s#.*/pack-\(.*\)\.pack#\1#"

Option '-E' is an alias for -r in GNU sed 4.2 (added in 4.2, not documented unti 4.3), released on May 11 2009. I prefer the `-E` version.

Show 9 quoted lines

>
> Minor nit: 'git pack-redundant' prints one filename per line, so the
> 'g' at the end of the 's###g' is not necessary.
>
> > +             sort -u | \
> > +             while read p; do eval echo "\${P$p}"; done | \
> > +             sort > actual && \
>
> Style nit: no space between redirection operator and filename

Will do.