Re: [PATCH 2/5] t/t5520: explicitly unset rebase.autostash

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 29, 2016, 20:16 UTC
Message-ID: <CAPig+cROGO0kSgTL7OpLGYN+cA7RKWHz0ES=h+FNDREcp65GJA@mail.gmail.com>
In-Reply-To: <1459258200-32444-3-git-send-email-mehul.jain2029@gmail.com>

On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:

> t/t5520: explicitly unset rebase.autostash

As with patch 1/5, this subject is written at too low a level, talking about details of the patch rather than giving a high-level overview. What the patch is really doing is ensuring consistent conditions within the test even if some future change pollutes the global configuration. Maybe:

    t5520: ensure consistent test conditions

or:

    t5520: make test expectations explicit

or something.

> Tests title suggest that tests are done with rebase.autostash unset,
> but doesn not take any action to make sure that it is indeed unset.

This is just paraphrasing my earlier review comment[1], however, "suggest" is a weak argument for why this change is desirable. State instead that this change ensures a consistent condition for tests in which rebase.autostash should not be set and protects against some future change polluting the global configuration.

> Make sure that rebase.autostash is unset by explicitly setting it.

The patch itself looks ok.

[1]: http://article.gmane.org/gmane.comp.version-control.git/289860

Show 27 quoted lines

> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
> ---
>  t/t5520-pull.sh | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 5be39df..9ee2218 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -279,6 +279,7 @@ test_expect_success 'pull --rebase --autostash & rebase.autostash=false' '
>  '
>
>  test_expect_success 'pull --rebase: --autostash & rebase.autostash unset' '
> +       test_unconfig rebase.autostash &&
>         git reset --hard before-rebase &&
>         echo dirty >new_file &&
>         git add new_file &&
> @@ -307,6 +308,7 @@ test_expect_success 'pull --rebase --no-autostash & rebase.autostash=false' '
>  '
>
>  test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
> +       test_unconfig rebase.autostash &&
>         git reset --hard before-rebase &&
>         echo dirty >new_file &&
>         git add new_file &&
> --
> 2.7.1.340.g69eb491.dirty