[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 
Signed-off-by: Mehul Jain 
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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  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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mehul Jain]

On Sun, Apr 3, 2016 at 12:20 AM, Johannes Sixt  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 
Signed-off-by: Mehul Jain 
---
  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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[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 
Signed-off-by: Mehul Jain 
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html