Re: [PATCH 5/5] t/t5520: test --[no-]autostash with pull.rebase=true

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 3, 2016, 19:28 UTC
Message-ID: <CAPig+cSR9Um5FUWzkzHGAM5RanaKssAysA5hGOP4+E5oA0Y5oA@mail.gmail.com>
In-Reply-To: <CA+DCAeT1DQvHnRpJeApcm2vO6KhXaMaRXZg9HCUmiiBv=hfxzw@mail.gmail.com>

On Fri, Apr 1, 2016 at 6:27 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:

Show 54 quoted lines

> I tried out this method also. Below is the script that I wrote for this:
>
> test_autostash () {
>     OLDIFS=$IFS
>     IFS=',    ='
>     while read -r expect cmd config_variable value
>     do
>         test_expect_success "$cmd, $config_variable=$value" '
>             if [ "$value" = "" ]; then
>                 test_unconfig $config_variable
>             else
>                 test_config $config_variable $value
>             fi &&
>
>             git reset --hard before-rebase &&
>             echo dirty >new_file &&
>             git add new_file &&
>
>             if [ $expect = "ok" ]; then
>                 git pull '$cmd' . copy &&
>                 echo test_cmp_rev HEAD^ copy &&
>                 test "$(cat new_file)" = dirty &&
>                 test "$(cat file)" = "modified again"
>             else
>                 test_must_fail git pull '$cmd' . copy 2>err &&
>                 test_i18ngrep "uncommitted changes." err
>             fi
>         '
>     done
>     IFS=$OLDIFS
> }
>
> test_autostash <<-\EOF
>     ok,--rebase,rebase.autostash=true
>     [...]
>     err,--no-autostash,pull.rebase=true
>     EOF
>
> Things worked out perfectly.
>
> Unfortunately there was a strange behaviour that I noticed
> and frankly I don't understand why it happened.
>
> In test_autostash() there's a line
>
>     echo test_cmp_rev HEAD^ copy &&
>
> Originally it should have been
>
>     test_cmp_rev HEAD^ copy &&
>
> but this raise following error while testing
>
>     ./t5520-pull.sh: 684: eval: diff -u: not found

This is caused by the custom IFS=',\t=' which is still in effect when test_cmp_rev() is invoked. You need to restore IFS within the loop itself.

> I'm not able to understand why putting an "echo" before
> test_cmp didn't raise the above error. This looks quite
> strange. Any thoughts?

With 'echo', test_cmp_rev() is never even invoked (the command is just printed by 'echo'), and 'echo' succeeds (returns 0), so the test succeeds, but isn't actually doing an revision verification.

The test also behaves incorrectly on these lines:

    git pull '$cmd' . copy &&

and:

    test_must_fail git pull '$cmd' . copy 2>err &&

Those single quotes around $cmd are within the second argument to test_expect_success(), which itself is a single quoted string. So, '$cmd' is actually ending and re-starting the "outer" quoted string. This isn't a problem when $cmd has a single token (such as "--rebase"), but it causes test_expect_success() to complain about incorrect number of arguments when $cmd is composed of multiple tokens (such as "--rebase --autostash").

Moreover, $cmd shouldn't be quoted at all. When $cmd is "--rebase --autostash", you want git-pull to see the --rebase and --autostash as separate arguments, but the quoting causes them to be treated as a single argument, which git-pull doesn't recognize. So, dropping the quotes around $cmd is the correct thing to do.

Show 15 quoted lines

> Though the above code works perfectly and can be used in
> place of previous tests. Only problem remains is tests titles.
> Currently with this script, test titles will be:
>
> ok 21 - --rebase, rebase.autostash=true
> ok 22 - --rebase --autostash, rebase.autostash=true
> ok 23 - --rebase --autostash, rebase.autostash=false
> ok 24 - --rebase --autostash, rebase.autostash=
> ok 25 - --rebase --no-autostash, rebase.autostash=true
> ok 26 - --rebase --no-autostash, rebase.autostash=false
> ok 27 - --rebase --no-autostash, rebase.autostash=
> ok 28 - --autostash, pull.rebase=true
> ok 29 - --no-autostash, pull.rebase=true
>
> Any thoughts/suggestions on them?

I don't see a particular problem with the titles. You could prefix them with "pull " if that's what you mean.