[PATCH 0/5] modify tests for --[no-]autostash option

[PATCH 0/5] modify tests for --[no-]autostash option

[Mehul Jain]

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

This series contain changes suggested by Eric and Matthieu on following
series

        http://thread.gmane.org/gmane.comp.version-control.git/289434

Changes made:
        * [Patch 4/5] reduces the code needed to test possible
          combinations of --autostash and rebase.autostash by introducing
          two functions.

          Also introduce a loop to tackle the repetitive code used to
          test the usage of --[no-]autostash without --rebase.

        * [Patch 5/5] introduces two new tests to check the cases when
          "git pull --[no-]autostash" is called with pull.rebase=true.


Mehul Jain (5):
  t/t5520: change rebase.autoStash to rebase.autostash
  t/t5520: explicitly unset rebase.autostash
  t/t5520: use test_i18ngrep instead of test_cmp
  t/t5520: modify tests to reduce common code
  t/t5520: test --[no-]autostash with pull.rebase=true

 t/t5520-pull.sh | 120 ++++++++++++++++++++++++++++----------------------------
 1 file changed, 61 insertions(+), 59 deletions(-)

-- 
2.7.1.340.g69eb491.dirty

[PATCH 1/5] t/t5520: change rebase.autoStash to rebase.autostash

[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

Re: [PATCH 1/5] t/t5520: change rebase.autoStash to rebase.autostash

[Eric Sunshine]

On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> t/t5520: change rebase.autoStash to rebase.autostash

This subject is written at too low a level, talking about details of
the patch rather than giving a high-level overview. A further
shortcoming is that there's no explanation of *why* this change is
desirable. Here's an attempt which addresses both problems.

    t5520: use consistent capitalization in test titles

(Note that I dropped the leading "t/" since it's implied.)

The patch itself is fine.

> 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 2/5] t/t5520: explicitly unset rebase.autostash

[Mehul Jain]

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

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

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

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

[Eric Sunshine]

On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> t/t5520: explicitly unset rebase.autostash

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

    t5520: ensure consistent test conditions

or:

    t5520: make test expectations explicit

or something.

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

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

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

The patch itself looks ok.

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

> 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 3/5] t/t5520: use test_i18ngrep instead of test_cmp

[Mehul Jain]

test_cmp is used for error checking when test_i18ngrep could be used.

Use test_i18ngrep to check for the valid error.

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

Re: [PATCH 3/5] t/t5520: use test_i18ngrep instead of test_cmp

[Eric Sunshine]

On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> t/t5520: use test_i18ngrep instead of test_cmp

As mentioned for earlier patches, this is too low-level, whereas it
should be giving a high-level overview.

> test_cmp is used for error checking when test_i18ngrep could be used.
>
> Use test_i18ngrep to check for the valid error.

"could be used" is not sufficient justification to explain why this
change is desirable. See [1] for a good explanation of why this change
should be made.

The patch itself looks fine.

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

> 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 4/5] t/t5520: modify tests to reduce common code

[Mehul Jain]

There exist three groups of tests which have repetitive lines of code.

Introduce two functions test_rebase_autostash() and
test_rebase_no_autostash() to reduce the number of lines. Also introduce
loops to futher reduce the current implementation.

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

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index d03cb84..2611170 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -9,6 +9,24 @@ modify () {
 	mv "$2.x" "$2"
 }
 
+test_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_rebase_no_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_expect_success setup '
 	echo file >file &&
 	git add file &&
@@ -256,75 +274,39 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb
 	test "$(cat file)" = "modified again"
 '
 
-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_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"
-'
+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 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_rebase_autostash
 '
 
-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_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
-'
+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 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_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>actual &&
+			test_i18ngrep "only valid with --rebase" actual
+		'
+	done
 
 test_expect_success 'pull.rebase' '
 	git reset --hard before-rebase &&
-- 
2.7.1.340.g69eb491.dirty

Re: [PATCH 4/5] t/t5520: modify tests to reduce common code

[Junio C Hamano]

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

> There exist three groups of tests which have repetitive lines of code.
>
> Introduce two functions test_rebase_autostash() and
> test_rebase_no_autostash() to reduce the number of lines. Also introduce
> loops to futher reduce the current implementation.

Sound like sensible idea.

> +for i in true false
> +	do
> +		test_expect_success "pull --rebase --autostash & rebase.autostash=$i" '
> +			test_config rebase.autostash $i &&
> +			test_rebase_autostash
> +		'
> +	done

The lines between do..done is over-indented (will locally fix--no
need to resend).

Re: [PATCH 4/5] t/t5520: modify tests to reduce common code

[Eric Sunshine]

On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> t/t5520: modify tests to reduce common code

As this is indeed a patch, "modify" is implied. Perhaps:

    t5520: factor out common code

> There exist three groups of tests which have repetitive lines of code.
>
> Introduce two functions test_rebase_autostash() and
> test_rebase_no_autostash() to reduce the number of lines. Also introduce
> loops to futher reduce the current implementation.

This patch is doing so much that it's difficult to review for
correctness. Taking [1] into consideration, better would be to split
it into at least three patches:

1. Factor out code into test_rebase_autostash() and modify the four
tests to call it.

2. Factor out code into test_rebase_autostash_fail() and modify the
three tests to call it.

3. Fold the two "pull $i (without --rebase) is illegal" tests into a for-loop.

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

> 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,24 @@ modify () {
> +test_rebase_no_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

In the spirit of patch 3/5 and [1], you could grep for a substring
rather than the full message, but that's a minor point, not worth a
re-roll.

    test_i18ngrep "uncommitted changes" err

> +}
> @@ -256,75 +274,39 @@ test_expect_success 'pull --rebase succeeds with dirty working directory and reb
> +for i in true false
> +       do
> +               test_expect_success "pull --rebase --autostash & rebase.autostash=$i" '
> +                       test_config rebase.autostash $i &&
> +                       test_rebase_autostash
> +               '
> +       done

I don't care too strongly, but I'm not convinced that this for-loop is
buying you much for these two cases since each test already has been
reduced to two simple lines, and the added abstraction of the for-loop
increases cognitive load a bit.

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

You might then ask why I suggested[1] the for-loop in this case but
not for the true/false case. Even though these are also two-line
tests, they are not quite as simple as two lines down to which the
true/false tests devolve. Anyhow, this alone is not worth a re-roll.

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

[Mehul Jain]

"--[no-]autostash" option for git-pull is only valid in rebase mode.
That is, either --rebase is 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 | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 2611170..4da9e52 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -316,6 +316,26 @@ test_expect_success 'pull.rebase' '
 	test new = "$(git show HEAD:file2)"
 '
 
+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_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_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 5/5] t/t5520: test --[no-]autostash with pull.rebase=true

[Eric Sunshine]

On Tue, Mar 29, 2016 at 9:30 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> "--[no-]autostash" option for git-pull is only valid in rebase mode.
> That is, either --rebase is 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>
> ---
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> @@ -316,6 +316,26 @@ 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"
> +'

With the exception of the missing --rebase argument, this is exactly
the same code as in test_rebase_autostash(), right? Rather than
repeating this code yet again, it might be nice to augment that
function to accept a (possibly) optional argument controlling whether
--rebase is used.

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

Ditto with regard to test_rebase_no_autostash() (or
test_rebase_autostash_fail() as I suggested in my patch 4/5 review).

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

[Mehul Jain]

Hi Eric,

Thanks for the reviews on this series.

On Wed, Mar 30, 2016 at 2:46 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> With the exception of the missing --rebase argument, this is exactly
> the same code as in test_rebase_autostash(), right? Rather than
> repeating this code yet again, it might be nice to augment that
> function to accept a (possibly) optional argument controlling whether
> --rebase is used.

Thanks for the idea. I have come up with something like this:

        * Introduce two function test_pull() and test_pull_fail() in
the place of
          test_rebase_autostash() and test_rebase_no_autostash.()

          Using these functions we can easily re-write all the 6 tests which
          deals with combination of autostash and rebase.autostash. Plus
          these functions helped in writing two new tests which deals with
          combination of pull.rebase and autostash. Thus reducing the code
          base to simpler and fewer lines of code. Also I could re-write one
          of the old test to reduce the repetition with them.

Here are the functions and there implementations:

---

test_pull () {
        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_pull_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 'pull --rebase succeeds with dirty working
directory and rebase.autostash set' '
        test_config rebase.autostash true &&
        test_pull --rebase
'

test_expect_success "pull --rebase --autostash & rebase.autostash=true" '
        test_config rebase.autostash true &&
        test_pull --rebase --autostash
'

test_expect_success "pull --rebase --autostash & rebase.autostash=false" '
        test_config rebase.autostash false &&
        test_pull --rebase --autostash
'

test_expect_success 'pull --rebase: --autostash & rebase.autostash unset' '
        test_unconfig rebase.autostash &&
        test_pull --rebase --autostash
'

test_expect_success "pull --rebase --no-autostash & rebase.autostash=true" '
        test_config rebase.autostash true &&
        test_pull_fail --rebase --no-autostash
'

test_expect_success "pull --rebase --no-autostash & rebase.autostash=false" '
        test_config rebase.autostash false &&
        test_pull_fail --rebase --no-autostash
'

test_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
        test_unconfig rebase.autostash &&
        test_pull_fail --rebase --no-autostash
'

test_expect_success 'pull --autostash & pull.rebase=true' '
        test_config pull.rebase true &&
        test_pull --autostash
'

test_expect_success 'pull --no-autostash & pull.rebase=true' '
        test_config pull.rebase true &&
        test_pull_fail --no-autostash
'
---

I'm sorry if this is bit difficult to digest without diff output. I
just wanted to
know if the above mention functions looks suitable to you.

Also I've read your comments on other patches of this series, I will make
changes accordingly ones above mention functions, tests looks fit for a
re-roll.

Thanks,
Mehul

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

[Eric Sunshine]

On Wed, Mar 30, 2016 at 3:00 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> On Wed, Mar 30, 2016 at 2:46 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> With the exception of the missing --rebase argument, this is exactly
>> the same code as in test_rebase_autostash(), right? Rather than
>> repeating this code yet again, it might be nice to augment that
>> function to accept a (possibly) optional argument controlling whether
>> --rebase is used.
>
> Thanks for the idea. I have come up with something like this:
>
> test_pull () {
>         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_pull_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
> }

Considering that these are specifically testing behavior related to
autostashing, it might make sense to have "autostash" in the function
names, but that's a very minor point.

> test_expect_success 'pull --rebase succeeds with dirty working
> directory and rebase.autostash set' '
>         test_config rebase.autostash true &&
>         test_pull --rebase
> '
> [...]
> test_expect_success 'pull --no-autostash & pull.rebase=true' '
>         test_config pull.rebase true &&
>         test_pull_fail --no-autostash
> '
>
> I'm sorry if this is bit difficult to digest without diff output. I
> just wanted to
> know if the above mention functions looks suitable to you.

This is exactly what I had in mind for simplifying the tests, and it's
perfectly easy to read in this form (a diff would be worse for this
illustration).

One other possibility would be to make this all table-driven by
collecting all of the above state information into a table and then
feeding that into a function (either as its argument list or via
stdin). For instance:

    test_autostash <<\-EOF
    ok,--rebase,rebase.autostash=true
    ok,--rebase --autostash,rebase.autostash=true
    ok,--rebase --autostash,rebase.autostash=false
    ok,--rebase --autostash,rebase.autostash=
    err,--rebase --no-autostash,rebase.autostash=true
    err,--rebase --no-autostash,rebase.autostash=false
    err,--rebase --no-autostash,rebase.autostash=
    ok,--autostash,pull.rebase=true
    err,--no-autostash,pull.rebase=true
   EOF

The function would loop over the input, split each line apart by
setting IFS=, and then run the test based upon the state information.
"ok" means autostash is expected to succeed, and err means it is
expected to fail. The function would want to specially recognize the
"foo.bar=" in the last argument in order to invoke test_unconfig()
rather than test_config().

However, this may be a case of diminishing returns. The tests as you
illustrated them are sufficiently simple and easy to grok that the
table-driven approach may not add much value (aside from making it
easier to see at a glance if any cases were omitted).

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

[Mehul Jain]

Hi Eric,

On Thu, Mar 31, 2016 at 2:01 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> One other possibility would be to make this all table-driven by
> collecting all of the above state information into a table and then
> feeding that into a function (either as its argument list or via
> stdin). For instance:
>
>     test_autostash <<\-EOF
>     ok,--rebase,rebase.autostash=true
>     ok,--rebase --autostash,rebase.autostash=true
>     ok,--rebase --autostash,rebase.autostash=false
>     ok,--rebase --autostash,rebase.autostash=
>     err,--rebase --no-autostash,rebase.autostash=true
>     err,--rebase --no-autostash,rebase.autostash=false
>     err,--rebase --no-autostash,rebase.autostash=
>     ok,--autostash,pull.rebase=true
>     err,--no-autostash,pull.rebase=true
>    EOF
>
> The function would loop over the input, split each line apart by
> setting IFS=, and then run the test based upon the state information.
> "ok" means autostash is expected to succeed, and err means it is
> expected to fail. The function would want to specially recognize the
> "foo.bar=" in the last argument in order to invoke test_unconfig()
> rather than test_config().

I tried out this method also. Below is the script that I wrote for this:

---

test_autostash () {
    OLDIFS=$IFS
    IFS=',    ='
    while read -r expect cmd config_variable value
    do
        test_expect_success "$cmd, $config_variable=$value" '
            if [ "$value" = "" ]; then
                test_unconfig $config_variable
            else
                test_config $config_variable $value
            fi &&

            git reset --hard before-rebase &&
            echo dirty >new_file &&
            git add new_file &&

            if [ $expect = "ok" ]; then
                git pull '$cmd' . copy &&
                echo test_cmp_rev HEAD^ copy &&
                test "$(cat new_file)" = dirty &&
                test "$(cat file)" = "modified again"
            else
                test_must_fail git pull '$cmd' . copy 2>err &&
                test_i18ngrep "uncommitted changes." err
            fi
        '
    done
    IFS=$OLDIFS
}


test_autostash <<-\EOF
    ok,--rebase,rebase.autostash=true
    ok,--rebase --autostash,rebase.autostash=true
    ok,--rebase --autostash,rebase.autostash=false
    ok,--rebase --autostash,rebase.autostash=
    err,--rebase --no-autostash,rebase.autostash=true
    err,--rebase --no-autostash,rebase.autostash=false
    err,--rebase --no-autostash,rebase.autostash=
    ok,--autostash,pull.rebase=true
    err,--no-autostash,pull.rebase=true
    EOF


---

Things worked out perfectly.

Unfortunately there was a strange behaviour that I noticed
and frankly I don't understand why it happened.

In test_autostash() there's a line

    echo test_cmp_rev HEAD^ copy &&

Originally it should have been

    test_cmp_rev HEAD^ copy &&

but this raise following error while testing

    ./t5520-pull.sh: 684: eval: diff -u: not found

I'm not able to understand why putting an "echo" before
test_cmp didn't raise the above error. This looks quite
strange. Any thoughts?

Though the above code works perfectly and can be used in
place of previous tests. Only problem remains is tests titles.
Currently with this script, test titles will be:

ok 21 - --rebase, rebase.autostash=true
ok 22 - --rebase --autostash, rebase.autostash=true
ok 23 - --rebase --autostash, rebase.autostash=false
ok 24 - --rebase --autostash, rebase.autostash=
ok 25 - --rebase --no-autostash, rebase.autostash=true
ok 26 - --rebase --no-autostash, rebase.autostash=false
ok 27 - --rebase --no-autostash, rebase.autostash=
ok 28 - --autostash, pull.rebase=true
ok 29 - --no-autostash, pull.rebase=true

Any thoughts/suggestions on them?

Thanks,
Mehul

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

[Eric Sunshine]

On Fri, Apr 1, 2016 at 6:27 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> I tried out this method also. Below is the script that I wrote for this:
>
> test_autostash () {
>     OLDIFS=$IFS
>     IFS=',    ='
>     while read -r expect cmd config_variable value
>     do
>         test_expect_success "$cmd, $config_variable=$value" '
>             if [ "$value" = "" ]; then
>                 test_unconfig $config_variable
>             else
>                 test_config $config_variable $value
>             fi &&
>
>             git reset --hard before-rebase &&
>             echo dirty >new_file &&
>             git add new_file &&
>
>             if [ $expect = "ok" ]; then
>                 git pull '$cmd' . copy &&
>                 echo test_cmp_rev HEAD^ copy &&
>                 test "$(cat new_file)" = dirty &&
>                 test "$(cat file)" = "modified again"
>             else
>                 test_must_fail git pull '$cmd' . copy 2>err &&
>                 test_i18ngrep "uncommitted changes." err
>             fi
>         '
>     done
>     IFS=$OLDIFS
> }
>
> test_autostash <<-\EOF
>     ok,--rebase,rebase.autostash=true
>     [...]
>     err,--no-autostash,pull.rebase=true
>     EOF
>
> Things worked out perfectly.
>
> Unfortunately there was a strange behaviour that I noticed
> and frankly I don't understand why it happened.
>
> In test_autostash() there's a line
>
>     echo test_cmp_rev HEAD^ copy &&
>
> Originally it should have been
>
>     test_cmp_rev HEAD^ copy &&
>
> but this raise following error while testing
>
>     ./t5520-pull.sh: 684: eval: diff -u: not found

This is caused by the custom IFS=',\t=' which is still in effect when
test_cmp_rev() is invoked. You need to restore IFS within the loop
itself.

> I'm not able to understand why putting an "echo" before
> test_cmp didn't raise the above error. This looks quite
> strange. Any thoughts?

With 'echo', test_cmp_rev() is never even invoked (the command is just
printed by 'echo'), and 'echo' succeeds (returns 0), so the test
succeeds, but isn't actually doing an revision verification.

The test also behaves incorrectly on these lines:

    git pull '$cmd' . copy &&

and:

    test_must_fail git pull '$cmd' . copy 2>err &&

Those single quotes around $cmd are within the second argument to
test_expect_success(), which itself is a single quoted string. So,
'$cmd' is actually ending and re-starting the "outer" quoted string.
This isn't a problem when $cmd has a single token (such as
"--rebase"), but it causes test_expect_success() to complain about
incorrect number of arguments when $cmd is composed of multiple tokens
(such as "--rebase --autostash").

Moreover, $cmd shouldn't be quoted at all. When $cmd is "--rebase
--autostash", you want git-pull to see the --rebase and --autostash as
separate arguments, but the quoting causes them to be treated as a
single argument, which git-pull doesn't recognize. So, dropping the
quotes around $cmd is the correct thing to do.

> Though the above code works perfectly and can be used in
> place of previous tests. Only problem remains is tests titles.
> Currently with this script, test titles will be:
>
> ok 21 - --rebase, rebase.autostash=true
> ok 22 - --rebase --autostash, rebase.autostash=true
> ok 23 - --rebase --autostash, rebase.autostash=false
> ok 24 - --rebase --autostash, rebase.autostash=
> ok 25 - --rebase --no-autostash, rebase.autostash=true
> ok 26 - --rebase --no-autostash, rebase.autostash=false
> ok 27 - --rebase --no-autostash, rebase.autostash=
> ok 28 - --autostash, pull.rebase=true
> ok 29 - --no-autostash, pull.rebase=true
>
> Any thoughts/suggestions on them?

I don't see a particular problem with the titles. You could prefix
them with "pull " if that's what you mean.

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

[Mehul Jain]

On Mon, Apr 4, 2016 at 12:58 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Fri, Apr 1, 2016 at 6:27 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
>> In test_autostash() there's a line
>>
>>     echo test_cmp_rev HEAD^ copy &&
>>
>> Originally it should have been
>>
>>     test_cmp_rev HEAD^ copy &&
>>
>> but this raise following error while testing
>>
>>     ./t5520-pull.sh: 684: eval: diff -u: not found
>
> This is caused by the custom IFS=',\t=' which is still in effect when
> test_cmp_rev() is invoked. You need to restore IFS within the loop
> itself.

Thanks for pointing it out. I made a mistake by not considering
the consequences of setting IFS=',\t='. I tried it out again and
this time all tests passed perfectly.

I should  been more careful in the first place while playing
with IFS, but instead of that, I kept on thinking that there is some
other problem with the script which lead to me making foolish
changes in the script like putting an echo before "test_cmp_rev ...".

It was nice of you to take out some time and point it out :)

Also now that I have sent v2[1] of this series, which goes
in different direction as far as implementation of these tests
are concerned. I think the script now is useless (but I
learned a bit about shell while writing it).

Thanks,
Mehul

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

[Matthieu Moy]

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

> On Mon, Apr 4, 2016 at 12:58 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Fri, Apr 1, 2016 at 6:27 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
>>> In test_autostash() there's a line
>>>
>>>     echo test_cmp_rev HEAD^ copy &&
>>>
>>> Originally it should have been
>>>
>>>     test_cmp_rev HEAD^ copy &&
>>>
>>> but this raise following error while testing
>>>
>>>     ./t5520-pull.sh: 684: eval: diff -u: not found
>>
>> This is caused by the custom IFS=',\t=' which is still in effect when
>> test_cmp_rev() is invoked. You need to restore IFS within the loop
>> itself.
>
> Thanks for pointing it out. I made a mistake by not considering
> the consequences of setting IFS=',\t='. I tried it out again and
> this time all tests passed perfectly.

I think it would be much simpler to drop the loop, and write instead
something like (untested):

test_autostash () {
	expect="$1"
        cmd="$2"
        config_variable="$3"
        value="$4"
        test_expect_success "$cmd, $config_variable=$value" '
            if [ "$value" = "" ]; then
                test_unconfig $config_variable
            else
                test_config $config_variable $value
            fi &&

            git reset --hard before-rebase &&
            echo dirty >new_file &&
            git add new_file &&

            if [ $expect = "ok" ]; then
                git pull '$cmd' . copy &&
                test_cmp_rev HEAD^ copy &&
                test "$(cat new_file)" = dirty &&
                test "$(cat file)" = "modified again"
            else
                test_must_fail git pull '$cmd' . copy 2>err &&
                test_i18ngrep "uncommitted changes." err
            fi
        '
}

test_autostash ok --rebase rebase.autostash=true
test_autostash ok '--rebase --autostash' rebase.autostash=true
test_autostash ok '--rebase --autostash' rebase.autostash=false
test_autostash ok '--rebase --autostash' rebase.autostash=
test_autostash err '--rebase --no-autostash' rebase.autostash=true
test_autostash err '--rebase --no-autostash' rebase.autostash=false
test_autostash err '--rebase --no-autostash' rebase.autostash=
test_autostash ok --autostash pull.rebase=true
test_autostash err --no-autostash pull.rebase=true

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

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

[Mehul Jain]

On Mon, Apr 4, 2016 at 10:22 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> I think it would be much simpler to drop the loop, and write instead
> something like (untested):

I tested it (with few minor changes), and worked fine.

test_autostash () {
        OLDIFS=$IFS
        IFS='='
        set -- $*
        IFS=$OLDIFS
        expect=$1
        cmd=$2
        config_variable=$3
        value=$4
        test_expect_success "$cmd, $config_variable=$value"     '
                if [ "$value" = "" ]; then
                        test_unconfig $config_variable
                else
                        test_config $config_variable $value
                fi &&

                git reset --hard before-rebase &&
                echo dirty >new_file &&
                git add new_file &&

                if [ $expect = "ok" ]; then
                        git pull $cmd . copy &&
                        test_cmp_rev HEAD^ copy &&
                        test "$(cat new_file)" = dirty &&
                        test "$(cat file)" = "modified again"
                else
                        test_must_fail git pull $cmd . copy 2>err &&
                        test_i18ngrep "uncommitted changes." err
                fi
        '
}

test_autostash ok '--rebase' rebase.autostash=true
test_autostash ok '--rebase --autostash' rebase.autostash=true
test_autostash ok '--rebase --autostash' rebase.autostash=false
test_autostash ok '--rebase --autostash' rebase.autostash=
test_autostash err '--rebase --no-autostash' rebase.autostash=true
test_autostash err '--rebase --no-autostash' rebase.autostash=false
test_autostash err '--rebase --no-autostash' rebase.autostash=
test_autostash ok '--autostash' pull.rebase=true
test_autostash err '--no-autostash' pull.rebase=true

Perhaps this looks better than the one with the loop. Even better than
the implementation in v2[1].

I think it would be wise to go with the above script for v3 (as I will
be doing a re-roll of the series[1]).

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

Thanks,
Mehul

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

[Eric Sunshine]

On Mon, Apr 4, 2016 at 1:36 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> On Mon, Apr 4, 2016 at 10:22 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> I think it would be much simpler to drop the loop, and write instead
>> something like (untested):
>
> I tested it (with few minor changes), and worked fine.
>
> test_autostash () {
>         OLDIFS=$IFS
>         IFS='='
>         set -- $*
>         IFS=$OLDIFS
>         expect=$1
>         cmd=$2
>         config_variable=$3
>         value=$4
>         test_expect_success "$cmd, $config_variable=$value"     '
>                 if [ "$value" = "" ]; then
>                         test_unconfig $config_variable
>                 else
>                         test_config $config_variable $value
>                 fi &&
>
>                 git reset --hard before-rebase &&
>                 echo dirty >new_file &&
>                 git add new_file &&
>
>                 if [ $expect = "ok" ]; then
>                         git pull $cmd . copy &&
>                         test_cmp_rev HEAD^ copy &&
>                         test "$(cat new_file)" = dirty &&
>                         test "$(cat file)" = "modified again"
>                 else
>                         test_must_fail git pull $cmd . copy 2>err &&
>                         test_i18ngrep "uncommitted changes." err
>                 fi
>         '
> }
>
> test_autostash ok '--rebase' rebase.autostash=true
> test_autostash ok '--rebase --autostash' rebase.autostash=true
> test_autostash ok '--rebase --autostash' rebase.autostash=false
> test_autostash ok '--rebase --autostash' rebase.autostash=
> test_autostash err '--rebase --no-autostash' rebase.autostash=true
> test_autostash err '--rebase --no-autostash' rebase.autostash=false
> test_autostash err '--rebase --no-autostash' rebase.autostash=
> test_autostash ok '--autostash' pull.rebase=true
> test_autostash err '--no-autostash' pull.rebase=true
>
> Perhaps this looks better than the one with the loop. Even better than
> the implementation in v2[1].
>
> I think it would be wise to go with the above script for v3 (as I will
> be doing a re-roll of the series[1]).

This new function is sufficiently complex that it increases cognitive
load enough for me to question if it is really a win for such a small
number of tests. The individual tests, as implemented in the current
round, are quite easy to understand, and don't place any significant
cognitive burden on the reader.

Although I'm the one who brought up the idea of "automating" these
tests, I'm not convinced that it's an improvement in this case, but I
don't feel so strongly that I'd forbid it. So, choose the approach
which seems best to you while weighing comprehension load for people
new to these tests, as well as maintainability costs.

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

[Matthieu Moy]

Eric Sunshine <sunshine@sunshineco.com> writes:

> Although I'm the one who brought up the idea of "automating" these
> tests, I'm not convinced that it's an improvement in this case, but I
> don't feel so strongly that I'd forbid it.

Another option is to define helper functions to shorten the "manual"
tests, e.g. define:

setup_rebase_test () {
	git reset --hard before-rebase &&
	echo dirty >new_file &&
	git add new_file
}

rebase_test_ok () {
        git pull $1 . copy &&
        test_cmp_rev HEAD^ copy &&
        test "$(cat new_file)" = dirty &&
        test "$(cat file)" = "modified again"
}

rebase_test_err () {
        test_must_fail git pull $1 . copy 2>err &&
        test_i18ngrep "uncommitted changes." err
}

I'm also OK with keeping the "manual" tests.

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

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

[Matthieu Moy]

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

> On Mon, Apr 4, 2016 at 10:22 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> I think it would be much simpler to drop the loop, and write instead
>> something like (untested):
>
> I tested it (with few minor changes), and worked fine.
>
> test_autostash () {
>         OLDIFS=$IFS
>         IFS='='
>         set -- $*
>         IFS=$OLDIFS

This $IFS dance is not needed. If you need to split variable and value,
then just pass two arguments on the caller side.

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