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

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

[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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Mehul Jain
In reply to this post by 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 <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Mehul Jain
In reply to this post by 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 <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Mehul Jain
In reply to this post by 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 <[hidden email]>
Signed-off-by: Mehul Jain <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Mehul Jain
In reply to this post by 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 <[hidden email]>
Signed-off-by: Mehul Jain <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Mehul Jain
In reply to this post by Mehul Jain
These two tests are almost similar and thus can be folded in a for-loop.

Helped-by: Eric Sunshine <[hidden email]>
Signed-off-by: Mehul Jain <[hidden email]>
---
 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 [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Mehul Jain
In reply to this post by 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 <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Johannes Sixt-3
In reply to this post by Mehul Jain
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 <[hidden email]>
> Signed-off-by: Mehul Jain <[hidden email]>
> ---
>   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 [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Mehul Jain
On Sun, Apr 3, 2016 at 12:20 AM, Johannes Sixt <[hidden email]> 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 [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Johannes Sixt-3
Am 03.04.2016 um 08:24 schrieb Mehul Jain:

> On Sun, Apr 3, 2016 at 12:20 AM, Johannes Sixt <[hidden email]> 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 [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Mehul Jain
In reply to this post by Mehul Jain
These two tests are almost similar and thus can be folded in a for-loop.

Helped-by: Eric Sunshine <[hidden email]>
Signed-off-by: Mehul Jain <[hidden email]>
---
 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 [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Eric Sunshine
In reply to this post by Mehul Jain
On Sat, Apr 2, 2016 at 1:58 PM, Mehul Jain <[hidden email]> 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 <[hidden email]>
> Signed-off-by: Mehul Jain <[hidden email]>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Eric Sunshine
In reply to this post by Mehul Jain
On Sat, Apr 2, 2016 at 1:58 PM, Mehul Jain <[hidden email]> 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 <[hidden email]>
> Signed-off-by: Mehul Jain <[hidden email]>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Eric Sunshine
In reply to this post by Mehul Jain
On Sat, Apr 2, 2016 at 1:58 PM, Mehul Jain <[hidden email]> 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 <[hidden email]>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Eric Sunshine
In reply to this post by Mehul Jain
On Sat, Apr 2, 2016 at 1:58 PM, Mehul Jain <[hidden email]> 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 <[hidden email]>

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Matthieu Moy-2
In reply to this post by Mehul Jain
Mehul Jain <[hidden email]> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Mehul Jain
On Mon, Apr 4, 2016 at 1:01 PM, Matthieu Moy
<[hidden email]> wrote:

> Mehul Jain <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
In reply to this post by Matthieu Moy-2
Matthieu Moy <[hidden email]> writes:

> Mehul Jain <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> wrote:

> Matthieu Moy <[hidden email]> writes:
>
>> Mehul Jain <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html