[PATCH v2 0/7] t5520: tests for --[no-]autostash option

[PATCH v2 0/7] t5520: tests for --[no-]autostash option

[Mehul Jain]

The following series is applicable on mj/pull-rebase-autostash.

Thanks Eric and Junio for there comments on previous version[1]

Changes made vs v1:
        * [Patch v1 4/5] is broken into three patches to increase
		  readability of the patches.

		* [Patch 4/5] Factor out code in two functions 
		  test_pull_autostash() and test_pull_autostash_fail()
		  instead of test_rebase_autostash() and 
		  test_rebase_no_autostash(). This leads to further 
		  simplification of code.
		  
		  Also removed two for-loops as they didn't provided
		  the simplicity intended for.
		  
		  For-loop was over-intended. Corrected it.

		* Commit message for patches 1/5, 2/5, 3/5 are improved
		  as suggested by Eric in the previous round.

Here's interdiff with v1:

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 4da9e52..bed75f5 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -9,22 +9,22 @@ modify () {
 	mv "$2.x" "$2"
 }
 
-test_rebase_autostash () {
+test_pull_autostash () {
 	git reset --hard before-rebase &&
 	echo dirty >new_file &&
 	git add new_file &&
-	git pull --rebase --autostash . copy &&
+	git pull $@ . copy &&
 	test_cmp_rev HEAD^ copy &&
 	test "$(cat new_file)" = dirty &&
 	test "$(cat file)" = "modified again"
 }
 
-test_rebase_no_autostash () {
+test_pull_autostash_fail () {
 	git reset --hard before-rebase &&
 	echo dirty >new_file &&
 	git add new_file &&
-	test_must_fail git pull --rebase --no-autostash . copy 2>err &&
-	test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+	test_must_fail git pull $@ . copy 2>err &&
+	test_i18ngrep "uncommitted changes." err
 }
 
 test_expect_success setup '
@@ -265,48 +265,46 @@ test_expect_success '--rebase fails with multiple branches' '
 
 test_expect_success 'pull --rebase succeeds with dirty working directory and rebase.autostash set' '
 	test_config rebase.autostash true &&
-	git reset --hard before-rebase &&
-	echo dirty >new_file &&
-	git add new_file &&
-	git pull --rebase . copy &&
-	test_cmp_rev HEAD^ copy &&
-	test "$(cat new_file)" = dirty &&
-	test "$(cat file)" = "modified again"
+	test_pull_autostash --rebase
+'
+
+test_expect_success 'pull --rebase --autostash & rebase.autostash=true' '
+	test_config rebase.autostash true &&
+	test_pull_autostash --rebase --autostash
 '
 
-for i in true false
-	do
-		test_expect_success "pull --rebase --autostash & rebase.autostash=$i" '
-			test_config rebase.autostash $i &&
-			test_rebase_autostash
-		'
-	done
+test_expect_success 'pull --rebase --autostash & rebase.autostash=false' '
+	test_config rebase.autostash false &&
+	test_pull_autostash --rebase --autostash
+'
 
-test_expect_success 'pull --rebase: --autostash & rebase.autostash unset' '
+test_expect_success 'pull --rebase --autostash & rebase.autostash unset' '
 	test_unconfig rebase.autostash &&
-	test_rebase_autostash
+	test_pull_autostash --rebase --autostash
+'
+
+test_expect_success 'pull --rebase --no-autostash & rebase.autostash=true' '
+	test_config rebase.autostash true &&
+	test_pull_autostash_fail --rebase --no-autostash
 '
 
-for i in true false
-	do
-		test_expect_success "pull --rebase --no-autostash & rebase.autostash=$i" '
-			test_config rebase.autostash $i &&
-			test_rebase_no_autostash
-		'
-	done
+test_expect_success 'pull --rebase --no-autostash & rebase.autostash=false' '
+	test_config rebase.autostash false &&
+	test_pull_autostash_fail --rebase --no-autostash
+'
 
 test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
 	test_unconfig rebase.autostash &&
-	test_rebase_no_autostash
+	test_pull_autostash_fail --rebase --no-autostash
 '
 
 for i in --autostash --no-autostash
-	do
-		test_expect_success "pull $i (without --rebase) is illegal" '
-			test_must_fail git pull $i . copy 2>actual &&
-			test_i18ngrep "only valid with --rebase" actual
-		'
-	done
+do
+	test_expect_success "pull $i (without --rebase) is illegal" '
+		test_must_fail git pull $i . copy 2>err &&
+		test_i18ngrep "only valid with --rebase" err
+	'
+done
 
 test_expect_success 'pull.rebase' '
 	git reset --hard before-rebase &&
@@ -318,22 +316,12 @@ test_expect_success 'pull.rebase' '
 
 test_expect_success 'pull --autostash & pull.rebase=true' '
 	test_config pull.rebase true &&
-	git reset --hard before-rebase &&
-	echo dirty >new_file &&
-	git add new_file &&
-	git pull --autostash . copy &&
-	test_cmp_rev HEAD^ copy &&
-	test "$(cat new_file)" = dirty &&
-	test "$(cat file)" = "modified again"
+	test_pull_autostash --autostash
 '
 
 test_expect_success 'pull --no-autostash & pull.rebase=true' '
 	test_config pull.rebase true &&
-	git reset --hard before-rebase &&
-	echo dirty >new_file &&
-	git add new_file &&
-	test_must_fail git pull --no-autostash . copy 2>err &&
-	test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+	test_pull_autostash_fail --no-autostash
 '
 
 test_expect_success 'branch.to-rebase.rebase' '


Mehul Jain (7):
  t5520: use consistent capitalization in test titles
  t5520: ensure consistent test conditions
  t5520: use better test to check stderr output
  t5520: factor out common code
  t5520: factor out common code
  t5520: reduce commom lines of code
  t5520: test --[no-]autostash with pull.rebase=true

 t/t5520-pull.sh | 102 +++++++++++++++++++++++++-------------------------------
 1 file changed, 46 insertions(+), 56 deletions(-)

-- 
2.7.1.340.g69eb491.dirty

[1]:http://thread.gmane.org/gmane.comp.version-control.git/290134

Thanks,
Mehul

[PATCH v2 1/7] t5520: use consistent capitalization in test titles

[Mehul Jain]

Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
 t/t5520-pull.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 745e59e..5be39df 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -267,7 +267,7 @@ test_expect_success 'pull --rebase --autostash & rebase.autostash=true' '
 	test "$(cat file)" = "modified again"
 '
 
-test_expect_success 'pull --rebase --autostash & rebase.autoStash=false' '
+test_expect_success 'pull --rebase --autostash & rebase.autostash=false' '
 	test_config rebase.autostash false &&
 	git reset --hard before-rebase &&
 	echo dirty >new_file &&
@@ -278,7 +278,7 @@ test_expect_success 'pull --rebase --autostash & rebase.autoStash=false' '
 	test "$(cat file)" = "modified again"
 '
 
-test_expect_success 'pull --rebase: --autostash & rebase.autoStash unset' '
+test_expect_success 'pull --rebase: --autostash & rebase.autostash unset' '
 	git reset --hard before-rebase &&
 	echo dirty >new_file &&
 	git add new_file &&
-- 
2.7.1.340.g69eb491.dirty

[PATCH v2 2/7] t5520: ensure consistent test conditions

[Mehul Jain]

Test title says that tests are done with rebase.autostash unset,
but does not take any action to make sure that it is indeed unset.
This may lead to test failure if future changes somehow pollutes
the configuration globally.

Ensure consistent test conditions by explicitly unsetting
rebase.autostash.

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

[PATCH v2 3/7] t5520: use better test to check stderr output

[Mehul Jain]

Checking stderr output using test_i18ncmp may lead to test failure as
some shells write trace output to stderr when run under 'set -x'.

Use test_i18ngrep instead of test_i18ncmp.

Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
 t/t5520-pull.sh | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 9ee2218..d03cb84 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -317,15 +317,13 @@ test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
 '
 
 test_expect_success 'pull --autostash (without --rebase) should error out' '
-	test_must_fail git pull --autostash . copy 2>actual &&
-	echo "fatal: --[no-]autostash option is only valid with --rebase." >expect &&
-	test_i18ncmp actual expect
+	test_must_fail git pull --autostash . copy 2>err &&
+	test_i18ngrep "only valid with --rebase" err
 '
 
 test_expect_success 'pull --no-autostash (without --rebase) should error out' '
-	test_must_fail git pull --no-autostash . copy 2>actual &&
-	echo "fatal: --[no-]autostash option is only valid with --rebase." >expect &&
-	test_i18ncmp actual expect
+	test_must_fail git pull --no-autostash . copy 2>err &&
+	test_i18ngrep "only valid with --rebase" err
 '
 
 test_expect_success 'pull.rebase' '
-- 
2.7.1.340.g69eb491.dirty

[PATCH v2 4/7] t5520: factor out common code

[Mehul Jain]

Four tests contains repetitive lines of code.

Factor out common code into test_pull_autostash() and then call it in
these tests.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
 t/t5520-pull.sh | 44 +++++++++++++++-----------------------------
 1 file changed, 15 insertions(+), 29 deletions(-)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index d03cb84..ac063c2 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -9,6 +9,16 @@ modify () {
 	mv "$2.x" "$2"
 }
 
+test_pull_autostash () {
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	git pull $@ . copy &&
+	test_cmp_rev HEAD^ copy &&
+	test "$(cat new_file)" = dirty &&
+	test "$(cat file)" = "modified again"
+}
+
 test_expect_success setup '
 	echo file >file &&
 	git add file &&
@@ -247,46 +257,22 @@ test_expect_success '--rebase fails with multiple branches' '
 
 test_expect_success 'pull --rebase succeeds with dirty working directory and rebase.autostash set' '
 	test_config rebase.autostash true &&
-	git reset --hard before-rebase &&
-	echo dirty >new_file &&
-	git add new_file &&
-	git pull --rebase . copy &&
-	test_cmp_rev HEAD^ copy &&
-	test "$(cat new_file)" = dirty &&
-	test "$(cat file)" = "modified again"
+	test_pull_autostash --rebase
 '
 
 test_expect_success 'pull --rebase --autostash & rebase.autostash=true' '
 	test_config rebase.autostash true &&
-	git reset --hard before-rebase &&
-	echo dirty >new_file &&
-	git add new_file &&
-	git pull --rebase --autostash . copy &&
-	test_cmp_rev HEAD^ copy &&
-	test "$(cat new_file)" = dirty &&
-	test "$(cat file)" = "modified again"
+	test_pull_autostash --rebase --autostash
 '
 
 test_expect_success 'pull --rebase --autostash & rebase.autostash=false' '
 	test_config rebase.autostash false &&
-	git reset --hard before-rebase &&
-	echo dirty >new_file &&
-	git add new_file &&
-	git pull --rebase --autostash . copy &&
-	test_cmp_rev HEAD^ copy &&
-	test "$(cat new_file)" = dirty &&
-	test "$(cat file)" = "modified again"
+	test_pull_autostash --rebase --autostash
 '
 
-test_expect_success 'pull --rebase: --autostash & rebase.autostash unset' '
+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 &&
-	git pull --rebase --autostash . copy &&
-	test_cmp_rev HEAD^ copy &&
-	test "$(cat new_file)" = dirty &&
-	test "$(cat file)" = "modified again"
+	test_pull_autostash --rebase --autostash
 '
 
 test_expect_success 'pull --rebase --no-autostash & rebase.autostash=true' '
-- 
2.7.1.340.g69eb491.dirty

[PATCH v2 5/7] t5520: factor out common code

[Mehul Jain]

Three tests contains repetitive lines of code.

Factor out common code into test_pull_autostash_fail() and then call it in
these tests.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
 t/t5520-pull.sh | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index ac063c2..fb9f845 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -19,6 +19,14 @@ test_pull_autostash () {
 	test "$(cat file)" = "modified again"
 }
 
+test_pull_autostash_fail () {
+	git reset --hard before-rebase &&
+	echo dirty >new_file &&
+	git add new_file &&
+	test_must_fail git pull $@ . copy 2>err &&
+	test_i18ngrep "uncommitted changes." err
+}
+
 test_expect_success setup '
 	echo file >file &&
 	git add file &&
@@ -277,29 +285,17 @@ test_expect_success 'pull --rebase --autostash & rebase.autostash unset' '
 
 test_expect_success 'pull --rebase --no-autostash & rebase.autostash=true' '
 	test_config rebase.autostash true &&
-	git reset --hard before-rebase &&
-	echo dirty >new_file &&
-	git add new_file &&
-	test_must_fail git pull --rebase --no-autostash . copy 2>err &&
-	test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+	test_pull_autostash_fail --rebase --no-autostash
 '
 
 test_expect_success 'pull --rebase --no-autostash & rebase.autostash=false' '
 	test_config rebase.autostash false &&
-	git reset --hard before-rebase &&
-	echo dirty >new_file &&
-	git add new_file &&
-	test_must_fail git pull --rebase --no-autostash . copy 2>err &&
-	test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+	test_pull_autostash_fail --rebase --no-autostash
 '
 
 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 &&
-	test_must_fail git pull --rebase --no-autostash . copy 2>err &&
-	test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
+	test_pull_autostash_fail --rebase --no-autostash
 '
 
 test_expect_success 'pull --autostash (without --rebase) should error out' '
-- 
2.7.1.340.g69eb491.dirty

[PATCH v2 6/7] t5520: reduce commom lines of code

[Mehul Jain]

These two tests are almost similar and thus can be folded in a for-loop.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
 t/t5520-pull.sh | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index fb9f845..e12af96 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -298,15 +298,13 @@ test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
 	test_pull_autostash_fail --rebase --no-autostash
 '
 
-test_expect_success 'pull --autostash (without --rebase) should error out' '
-	test_must_fail git pull --autostash . copy 2>err &&
-	test_i18ngrep "only valid with --rebase" err
-'
-
-test_expect_success 'pull --no-autostash (without --rebase) should error out' '
-	test_must_fail git pull --no-autostash . copy 2>err &&
-	test_i18ngrep "only valid with --rebase" err
-'
+for i in --autostash --no-autostash
+do
+	test_expect_success "pull $i (without --rebase) is illegal" '
+		test_must_fail git pull $i . copy 2>err &&
+		test_i18ngrep "only valid with --rebase" err
+	'
+done
 
 test_expect_success 'pull.rebase' '
 	git reset --hard before-rebase &&
-- 
2.7.1.340.g69eb491.dirty

[PATCH v2 7/7] t5520: test --[no-]autostash with pull.rebase=true

[Mehul Jain]

"--[no-]autostash" option for git-pull is only valid in rebase mode(
i.e. either --rebase should be used or pull.rebase=true). Existing
tests already check the cases when --rebase is used but fails to check
for pull.rebase=true case.

Add two new tests to check that --[no-]autostash option works with
pull.rebase=true.

Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
 t/t5520-pull.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index e12af96..bed75f5 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -314,6 +314,16 @@ test_expect_success 'pull.rebase' '
 	test new = "$(git show HEAD:file2)"
 '
 
+test_expect_success 'pull --autostash & pull.rebase=true' '
+	test_config pull.rebase true &&
+	test_pull_autostash --autostash
+'
+
+test_expect_success 'pull --no-autostash & pull.rebase=true' '
+	test_config pull.rebase true &&
+	test_pull_autostash_fail --no-autostash
+'
+
 test_expect_success 'branch.to-rebase.rebase' '
 	git reset --hard before-rebase &&
 	test_config branch.to-rebase.rebase true &&
-- 
2.7.1.340.g69eb491.dirty

Re: [PATCH v2 6/7] t5520: reduce commom lines of code

[Johannes Sixt]

Am 02.04.2016 um 19:58 schrieb Mehul Jain:
> These two tests are almost similar and thus can be folded in a for-loop.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
> ---
>   t/t5520-pull.sh | 16 +++++++---------
>   1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index fb9f845..e12af96 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -298,15 +298,13 @@ test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
>   	test_pull_autostash_fail --rebase --no-autostash
>   '
>
> -test_expect_success 'pull --autostash (without --rebase) should error out' '
> -	test_must_fail git pull --autostash . copy 2>err &&
> -	test_i18ngrep "only valid with --rebase" err
> -'
> -
> -test_expect_success 'pull --no-autostash (without --rebase) should error out' '
> -	test_must_fail git pull --no-autostash . copy 2>err &&
> -	test_i18ngrep "only valid with --rebase" err
> -'
> +for i in --autostash --no-autostash
> +do
> +	test_expect_success "pull $i (without --rebase) is illegal" '
> +		test_must_fail git pull $i . copy 2>err &&
> +		test_i18ngrep "only valid with --rebase" err
> +	'
> +done

Hm. If the implementation of test_expect_success uses the variable, too, 
its value is lost when the test snippet runs. Fortunately, it does not.

You can make this code a bit more robust by using double-quotes around 
the test code so that $i is expanded before test_expect_success is 
evaluated.

You could also change the variable name, but to be sufficiently safe, 
you would have to use an unsightly long name. 'opt' would be just as bad 
as 'i'.

>
>   test_expect_success 'pull.rebase' '
>   	git reset --hard before-rebase &&
>

-- Hannes

Re: [PATCH v2 6/7] t5520: reduce commom lines of code

[Mehul Jain]

On Sun, Apr 3, 2016 at 12:20 AM, Johannes Sixt <j6t@kdbg.org> wrote:
> Am 02.04.2016 um 19:58 schrieb Mehul Jain:
>> +for i in --autostash --no-autostash
>> +do
>> +       test_expect_success "pull $i (without --rebase) is illegal" '
>> +               test_must_fail git pull $i . copy 2>err &&
>> +               test_i18ngrep "only valid with --rebase" err
>> +       '
>> +done
>
>
> Hm. If the implementation of test_expect_success uses the variable, too, its
> value is lost when the test snippet runs. Fortunately, it does not.
>
> You can make this code a bit more robust by using double-quotes around the
> test code so that $i is expanded before test_expect_success is evaluated.

I think that the current format is preferred over the one you suggest.
Here[1] Junio
has given a descriptive explanation.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/283350/focus=284769

Thanks,
Mehul

Re: [PATCH v2 6/7] t5520: reduce commom lines of code

[Johannes Sixt]

Am 03.04.2016 um 08:24 schrieb Mehul Jain:
> On Sun, Apr 3, 2016 at 12:20 AM, Johannes Sixt <j6t@kdbg.org> wrote:
>> Am 02.04.2016 um 19:58 schrieb Mehul Jain:
>>> +for i in --autostash --no-autostash
>>> +do
>>> +       test_expect_success "pull $i (without --rebase) is illegal" '
>>> +               test_must_fail git pull $i . copy 2>err &&
>>> +               test_i18ngrep "only valid with --rebase" err
>>> +       '
>>> +done
>>
>>
>> Hm. If the implementation of test_expect_success uses the variable, too, its
>> value is lost when the test snippet runs. Fortunately, it does not.
>>
>> You can make this code a bit more robust by using double-quotes around the
>> test code so that $i is expanded before test_expect_success is evaluated.
>
> I think that the current format is preferred over the one you suggest.
> Here[1] Junio
> has given a descriptive explanation.
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/283350/focus=284769

Junio has a point there, of course.

In this light, I suggest that you use a more verbose variable name.

-- Hannes

[PATCH v2 6/7] t5520: reduce commom lines of code

[Mehul Jain]

These two tests are almost similar and thus can be folded in a for-loop.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
 t/t5520-pull.sh | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index fb9f845..e12af96 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -298,15 +298,13 @@ test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
 	test_pull_autostash_fail --rebase --no-autostash
 '
 
-test_expect_success 'pull --autostash (without --rebase) should error out' '
-	test_must_fail git pull --autostash . copy 2>err &&
-	test_i18ngrep "only valid with --rebase" err
-'
-
-test_expect_success 'pull --no-autostash (without --rebase) should error out' '
-	test_must_fail git pull --no-autostash . copy 2>err &&
-	test_i18ngrep "only valid with --rebase" err
-'
+for autostash_flag in --autostash --no-autostash
+do
+	test_expect_success "pull $autostash_flag (without --rebase) is illegal" '
+		test_must_fail git pull $autostash_flag . copy 2>err &&
+		test_i18ngrep "only valid with --rebase" err
+	'
+done
 
 test_expect_success 'pull.rebase' '
 	git reset --hard before-rebase &&
-- 
2.7.1.340.g69eb491.dirty

Re: [PATCH v2 4/7] t5520: factor out common code

[Eric Sunshine]

On Sat, Apr 2, 2016 at 1:58 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> t5520: factor out common code

To distinguish this title from that of patch 5/7, you could say:

    t5520: factor out common "successful autostash" code

> Four tests contains repetitive lines of code.
>
> Factor out common code into test_pull_autostash() and then call it in
> these tests.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
> ---
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> @@ -9,6 +9,16 @@ modify () {
> +test_pull_autostash () {
> +       git reset --hard before-rebase &&
> +       echo dirty >new_file &&
> +       git add new_file &&
> +       git pull $@ . copy &&

Nit: This could just as well be $* rather than $@.

> +       test_cmp_rev HEAD^ copy &&
> +       test "$(cat new_file)" = dirty &&
> +       test "$(cat file)" = "modified again"
> +}
> @@ -247,46 +257,22 @@ test_expect_success '--rebase fails with multiple branches' '
>
>  test_expect_success 'pull --rebase succeeds with dirty working directory and rebase.autostash set' '
>         test_config rebase.autostash true &&
> -       git reset --hard before-rebase &&
> -       echo dirty >new_file &&
> -       git add new_file &&
> -       git pull --rebase . copy &&
> -       test_cmp_rev HEAD^ copy &&
> -       test "$(cat new_file)" = dirty &&
> -       test "$(cat file)" = "modified again"
> +       test_pull_autostash --rebase
>  '
>
>  test_expect_success 'pull --rebase --autostash & rebase.autostash=true' '
>         test_config rebase.autostash true &&
> -       git reset --hard before-rebase &&
> -       echo dirty >new_file &&
> -       git add new_file &&
> -       git pull --rebase --autostash . copy &&
> -       test_cmp_rev HEAD^ copy &&
> -       test "$(cat new_file)" = dirty &&
> -       test "$(cat file)" = "modified again"
> +       test_pull_autostash --rebase --autostash
>  '
>
>  test_expect_success 'pull --rebase --autostash & rebase.autostash=false' '
>         test_config rebase.autostash false &&
> -       git reset --hard before-rebase &&
> -       echo dirty >new_file &&
> -       git add new_file &&
> -       git pull --rebase --autostash . copy &&
> -       test_cmp_rev HEAD^ copy &&
> -       test "$(cat new_file)" = dirty &&
> -       test "$(cat file)" = "modified again"
> +       test_pull_autostash --rebase --autostash
>  '
>
> -test_expect_success 'pull --rebase: --autostash & rebase.autostash unset' '
> +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 &&
> -       git pull --rebase --autostash . copy &&
> -       test_cmp_rev HEAD^ copy &&
> -       test "$(cat new_file)" = dirty &&
> -       test "$(cat file)" = "modified again"
> +       test_pull_autostash --rebase --autostash
>  '
>
>  test_expect_success 'pull --rebase --no-autostash & rebase.autostash=true' '
> --
> 2.7.1.340.g69eb491.dirty

Re: [PATCH v2 5/7] t5520: factor out common code

[Eric Sunshine]

On Sat, Apr 2, 2016 at 1:58 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> t5520: factor out common code

To distinguish this title from that of patch 4/7, you could say:

    t5520: factor out common "failing autostash" code

> Three tests contains repetitive lines of code.
>
> Factor out common code into test_pull_autostash_fail() and then call it in
> these tests.
>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
> ---
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> @@ -19,6 +19,14 @@ test_pull_autostash () {
> +test_pull_autostash_fail () {
> +       git reset --hard before-rebase &&
> +       echo dirty >new_file &&
> +       git add new_file &&
> +       test_must_fail git pull $@ . copy 2>err &&

Nit: Same comment as in 4/7: This could just as well be $* rather than $@.

> +       test_i18ngrep "uncommitted changes." err
> +}
> @@ -277,29 +285,17 @@ test_expect_success 'pull --rebase --autostash & rebase.autostash unset' '
>
>  test_expect_success 'pull --rebase --no-autostash & rebase.autostash=true' '
>         test_config rebase.autostash true &&
> -       git reset --hard before-rebase &&
> -       echo dirty >new_file &&
> -       git add new_file &&
> -       test_must_fail git pull --rebase --no-autostash . copy 2>err &&
> -       test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
> +       test_pull_autostash_fail --rebase --no-autostash
>  '
>
>  test_expect_success 'pull --rebase --no-autostash & rebase.autostash=false' '
>         test_config rebase.autostash false &&
> -       git reset --hard before-rebase &&
> -       echo dirty >new_file &&
> -       git add new_file &&
> -       test_must_fail git pull --rebase --no-autostash . copy 2>err &&
> -       test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
> +       test_pull_autostash_fail --rebase --no-autostash
>  '
>
>  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 &&
> -       test_must_fail git pull --rebase --no-autostash . copy 2>err &&
> -       test_i18ngrep "Cannot pull with rebase: Your index contains uncommitted changes." err
> +       test_pull_autostash_fail --rebase --no-autostash
>  '
>
>  test_expect_success 'pull --autostash (without --rebase) should error out' '
> --
> 2.7.1.340.g69eb491.dirty

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

[Eric Sunshine]

On Sat, Apr 2, 2016 at 1:58 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> "--[no-]autostash" option for git-pull is only valid in rebase mode(

s/"--[no-]autostash"/The --[no-]autostash/

Also, move the '(' from the end of the line to the beginning of the next line.

> i.e. either --rebase should be used or pull.rebase=true). Existing
> tests already check the cases when --rebase is used but fails to check
> for pull.rebase=true case.
>
> Add two new tests to check that --[no-]autostash option works with
> pull.rebase=true.

Nicely explained.

> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
> ---
>  t/t5520-pull.sh | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index e12af96..bed75f5 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -314,6 +314,16 @@ test_expect_success 'pull.rebase' '
>         test new = "$(git show HEAD:file2)"
>  '
>
> +test_expect_success 'pull --autostash & pull.rebase=true' '
> +       test_config pull.rebase true &&
> +       test_pull_autostash --autostash
> +'
> +
> +test_expect_success 'pull --no-autostash & pull.rebase=true' '
> +       test_config pull.rebase true &&
> +       test_pull_autostash_fail --no-autostash
> +'
> +
>  test_expect_success 'branch.to-rebase.rebase' '
>         git reset --hard before-rebase &&
>         test_config branch.to-rebase.rebase true &&
> --
> 2.7.1.340.g69eb491.dirty

Re: [PATCH v2 0/7] t5520: tests for --[no-]autostash option

[Eric Sunshine]

On Sat, Apr 2, 2016 at 1:58 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> The following series is applicable on mj/pull-rebase-autostash.
>
> Changes made vs v1:
>         * [Patch v1 4/5] is broken into three patches to increase
>                   readability of the patches.
>
>                 * [Patch 4/5] Factor out code in two functions
>                   test_pull_autostash() and test_pull_autostash_fail()
>                   instead of test_rebase_autostash() and
>                   test_rebase_no_autostash(). This leads to further
>                   simplification of code.
>
>                   Also removed two for-loops as they didn't provided
>                   the simplicity intended for.
>
>                   For-loop was over-intended. Corrected it.
>
>                 * Commit message for patches 1/5, 2/5, 3/5 are improved
>                   as suggested by Eric in the previous round.

Thanks, this version was a pleasant read, much simpler and easier to
digest than the previous round[1]. With or without addressing the few
minor nits in my review (none of which warrant a re-roll), this entire
series is:

    Reviewed-by: Eric Sunshine <sunshine@sunshineco.com>

Mehul, feel free to add my Reviewed-by: if you happen to re-roll (or
Junio can add it if he wants when he picks up the series).

[1]: http://thread.gmane.org/gmane.comp.version-control.git/290134

Re: [PATCH v2 0/7] t5520: tests for --[no-]autostash option

[Matthieu Moy]

Mehul Jain <mehul.jain2029@gmail.com> writes:

> -test_rebase_autostash () {
> +test_pull_autostash () {
>  	git reset --hard before-rebase &&
>  	echo dirty >new_file &&
>  	git add new_file &&
> -	git pull --rebase --autostash . copy &&
> +	git pull $@ . copy &&

Not strictly needed here, but I'd write "$@" (with the double-quotes)
which is the robust way to say "transmit all my arguments without
whitespace interpretation".

I don't mind for this patch since there's no whitespace to interpret,
but some people (sysadmins ;-) ) have the bad habit of writting $@, $*
or "$*" in wrapper scripts and it breaks when you call them with spaces
so it's better to take good habits IHMO.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v2 0/7] t5520: tests for --[no-]autostash option

[Mehul Jain]

On Mon, Apr 4, 2016 at 1:01 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Mehul Jain <mehul.jain2029@gmail.com> writes:
>
>> -test_rebase_autostash () {
>> +test_pull_autostash () {
>>       git reset --hard before-rebase &&
>>       echo dirty >new_file &&
>>       git add new_file &&
>> -     git pull --rebase --autostash . copy &&
>> +     git pull $@ . copy &&
>
> Not strictly needed here, but I'd write "$@" (with the double-quotes)
> which is the robust way to say "transmit all my arguments without
> whitespace interpretation".
>
> I don't mind for this patch since there's no whitespace to interpret,
> but some people (sysadmins ;-) ) have the bad habit of writting $@, $*
> or "$*" in wrapper scripts and it breaks when you call them with spaces
> so it's better to take good habits IHMO.

Thanks for the suggestion, I will remember it. I'm relatively new to
shell and therefore didn't know much about the difference
between "$@" and $@, $*, "$*".

Now that I have read[1][2] about it, it won't be repeated.

[1]: http://unix.stackexchange.com/questions/41571/what-is-the-difference-between-and/94200#94200
[2]: http://unix.stackexchange.com/questions/131766/why-does-my-shell-script-choke-on-whitespace-or-other-special-characters

Thanks,
Mehul

Re: [PATCH v2 0/7] t5520: tests for --[no-]autostash option

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Mehul Jain <mehul.jain2029@gmail.com> writes:
>
>> -test_rebase_autostash () {
>> +test_pull_autostash () {
>>  	git reset --hard before-rebase &&
>>  	echo dirty >new_file &&
>>  	git add new_file &&
>> -	git pull --rebase --autostash . copy &&
>> +	git pull $@ . copy &&
>
> Not strictly needed here, but I'd write "$@" (with the double-quotes)
> which is the robust way to say "transmit all my arguments without
> whitespace interpretation".

Yes, these should be "$@" (with the double-quotes).

> I don't mind for this patch since there's no whitespace to interpret,
> but some people (sysadmins ;-) ) have the bad habit of writting $@, $*
> or "$*" in wrapper scripts and it breaks when you call them with spaces
> so it's better to take good habits IHMO.

Re: [PATCH v2 0/7] t5520: tests for --[no-]autostash option

[Mehul Jain]

On Mon, Apr 4, 2016 at 10:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Mehul Jain <mehul.jain2029@gmail.com> writes:
>>
>>> -test_rebase_autostash () {
>>> +test_pull_autostash () {
>>>      git reset --hard before-rebase &&
>>>      echo dirty >new_file &&
>>>      git add new_file &&
>>> -    git pull --rebase --autostash . copy &&
>>> +    git pull $@ . copy &&
>>
>> Not strictly needed here, but I'd write "$@" (with the double-quotes)
>> which is the robust way to say "transmit all my arguments without
>> whitespace interpretation".
>
> Yes, these should be "$@" (with the double-quotes).

I will do a re-roll then.

Thanks,
Mehul