patch, 5 partsmodify tests for --[no-]autostash option

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

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

On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:

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.

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

On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:

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.

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.

The patch itself looks ok.

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

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

On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:

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

"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

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

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

Sound like sensible idea.

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

On Tue, Mar 29, 2016 at 9:29 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:

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

    t5520: factor out common code

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

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

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.

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.

"--[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(+)

On Tue, Mar 29, 2016 at 9:30 AM, Mehul Jain <mehul.jain2029@gmail.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.

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

Hi Eric,

Thanks for the reviews on this series.

On Wed, Mar 30, 2016 at 2:46 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:

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

On Wed, Mar 30, 2016 at 3:00 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:

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.

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

Hi Eric,

On Thu, Mar 31, 2016 at 2:01 AM, Eric Sunshine <sunshine@sunshineco.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
    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

On Fri, Apr 1, 2016 at 6:27 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:

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.

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.

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

On Mon, Apr 4, 2016 at 12:58 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:

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

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

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

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

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

On Mon, Apr 4, 2016 at 1:36 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:

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.

Eric Sunshine <sunshine@sunshineco.com> writes:

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.

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

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