threads / patch / 41904

v2, 7 partst5520: tests for --[no-]autostash option

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

## tl;dr

20 messages between Apr 2, 2016 and Apr 4, 2016. Diffs are folded; open one to read it.

replies: 19people: 5as markdown or json

Mehul Jain· Apr 2, 2016, 17:58 UTC · lore
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:
Show changes to t/t5520-pull.sh +35 −47
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
Mehul Jain· Apr 2, 2016, 17:58 UTC · re: Mehul Jain · lore

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

Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
---
 t/t5520-pull.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to t/t5520-pull.sh +2 −2
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
Mehul Jain· Apr 2, 2016, 17:58 UTC · re: Mehul Jain · lore

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

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(+)
Show changes to t/t5520-pull.sh +2 −0
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
Mehul Jain· Apr 2, 2016, 17:58 UTC · re: Mehul Jain · lore

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

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(-)
Show changes to t/t5520-pull.sh +4 −6
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
Mehul Jain· Apr 2, 2016, 17:58 UTC · re: Mehul Jain · lore

[PATCH v2 4/7] t5520: factor out common 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>
---
 t/t5520-pull.sh | 44 +++++++++++++++-----------------------------
 1 file changed, 15 insertions(+), 29 deletions(-)
Show changes to t/t5520-pull.sh +15 −29
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
Eric Sunshine· Apr 3, 2016, 20:03 UTC · re: Mehul Jain · lore

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

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
Show 15 quoted lines
> 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 $@.
Show 58 quoted lines
> +       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
Mehul Jain· Apr 2, 2016, 17:58 UTC · re: Mehul Jain · lore

[PATCH v2 5/7] t5520: factor out common 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>
---
 t/t5520-pull.sh | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)
Show changes to t/t5520-pull.sh +11 −15
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
Eric Sunshine· Apr 3, 2016, 20:05 UTC · re: Mehul Jain · lore

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

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
Show 15 quoted lines
> 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 $@.
Show 37 quoted lines
> +       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
Mehul Jain· Apr 2, 2016, 17:58 UTC · re: Mehul Jain · lore

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

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(-)
Show changes to t/t5520-pull.sh +7 −9
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
Johannes Sixt· Apr 2, 2016, 18:50 UTC · re: Mehul Jain · lore

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

Am 02.04.2016 um 19:58 schrieb Mehul Jain:
Show 32 quoted lines
> 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
Mehul Jain· Apr 3, 2016, 06:24 UTC · re: Johannes Sixt · lore

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

On Sun, Apr 3, 2016 at 12:20 AM, Johannes Sixt <j6t@kdbg.org> wrote:
Show 15 quoted lines
> 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

Johannes Sixt· Apr 3, 2016, 06:46 UTC · re: Mehul Jain · lore

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

Am 03.04.2016 um 08:24 schrieb Mehul Jain:
Show 22 quoted lines
> 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
Mehul Jain· Apr 2, 2016, 17:58 UTC · re: Mehul Jain · lore

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

"--[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(+)
Show changes to t/t5520-pull.sh +10 −0
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
Eric Sunshine· Apr 3, 2016, 20:11 UTC · re: Mehul Jain · lore

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

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.
Show 6 quoted lines
> 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.
Show 28 quoted lines
> 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
Mehul Jain· Apr 3, 2016, 08:47 UTC · re: Mehul Jain · lore

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

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(-)
Show changes to t/t5520-pull.sh +7 −9
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
Eric Sunshine· Apr 3, 2016, 20:17 UTC · re: Mehul Jain · lore

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

On Sat, Apr 2, 2016 at 1:58 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
Show 19 quoted lines
> 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
Matthieu Moy· Apr 4, 2016, 07:31 UTC · re: Mehul Jain · lore

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

Mehul Jain <mehul.jain2029@gmail.com> writes:
Show 7 quoted lines
> -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/
Mehul Jain· Apr 4, 2016, 16:58 UTC · re: Matthieu Moy · lore

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

On Mon, Apr 4, 2016 at 1:01 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 18 quoted lines
> 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

Junio C Hamano· Apr 4, 2016, 17:00 UTC · re: Matthieu Moy · lore

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

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
Show 13 quoted lines
> 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.
Mehul Jain· Apr 4, 2016, 17:07 UTC · re: Junio C Hamano · lore

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

On Mon, Apr 4, 2016 at 10:30 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> 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

← back to recent threads