From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Fri, 13 Aug 2010 20:55:35 GMT
Subject: Re: [RFC/PATCH] t5525: test the tagopt variable and that it can be overridden
Message-ID: <AANLkTimYm+GmJ4BmZKOmcZkJf_wgeUiKhTs06qxex+0q@mail.gmail.com>
In-Reply-To: <1281730410-20009-1-git-send-email-ComputerDruid@gmail.com>

On Fri, Aug 13, 2010 at 20:13, Daniel Johnson <computerdruid@gmail.com> wrote:
> ---
>> The current behaviour seems to me a bug introduced while git-fetch was
>> rewritten in C (the original found in contrib/examples reads from the
>> config only when no --tags/--no-tags option is given from the command
>> line).
>>
>> Is this something we can protect with a test script from future breakages?
> This should test that behavior. I'd appreciate feedback on how to improve this
> test. I'm not sure if this is the right name/number either.

Thanks for tackling this.

>  t/t5525-fetch-tagopt.sh |   44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 44 insertions(+), 0 deletions(-)
>  create mode 100755 t/t5525-fetch-tagopt.sh
>
> diff --git a/t/t5525-fetch-tagopt.sh b/t/t5525-fetch-tagopt.sh

The test name looks fine.

(The t55* test names are a bit of a mess with mixed pull/fetch, but
that's not something that should be dealt with here)

> new file mode 100755
> index 0000000..17bd407
> --- /dev/null
> +++ b/t/t5525-fetch-tagopt.sh
> @@ -0,0 +1,44 @@
> +
> +#!/bin/sh

Is that an empty line before the test begins? The shebang should be on
the first line.

> +test_description='tagopt variable affects "git fetch" and is overridden by commandline.'
> +
> +. ./test-lib.sh
> +
> +setup_clone () {
> +       (git clone --mirror . $1 &&
> +       git remote add remote_$1 $1 &&
> +       cd $1 &&
> +       git tag tag_$1)
> +}

Maybe only put the "cd $1 ..." inside a subshell for clarity.

> +test_expect_success setup '
> +       echo >file original &&
> +       git add file &&
> +       git commit -a -m original &&

Maybe this can use test_commit if you don't mind it creating a tag
too.

> +       setup_clone one &&
> +       git config remote.remote_one.tagopt --no-tags &&
> +       setup_clone two &&
> +       git config remote.remote_two.tagopt --tags
> +       '
> +
> +test_expect_success "fetch with tagopt=--no-tags does not get tag" '
> +       git fetch remote_one &&
> +       ! (git show-ref tag_one)
> +       '

Doesn't need a subshell? You should also use:

    test_must_fail git show-ref ...

> +test_expect_success "fetch --tags with tagopt=--no-tags gets tag" '
> +       git fetch --tags remote_one &&
> +       (git show-ref tag_one)
> +       '

Doesn't need a subshell?

> +test_expect_success "fetch --no-tags with tagopt=--tags does not get tag" '
> +       git fetch --no-tags remote_two &&
> +       ! (git show-ref tag_two)
> +       '
> +
> +test_expect_success "fetch with tagopt=--tags gets tag" '
> +       git fetch remote_two &&
> +       (git show-ref tag_two)
> +       '
> +test_done

test_must_fail etc etc.

Otherwise it looks good.