Re: [PATCH v10 2/2] pull --rebase: add --[no-]autostash flag

>> +test_expect_success 'pull --rebase: --autostash & rebase.autoStash unset' '
>
> The title says that this is testing with rebase.autoStash unset,
> however, the test itself doesn't take any action to ensure that it is
> indeed unset.

>As with the two above tests which explicitly set
> rebase.autoStash, this test should explicitly unset rebase.autoStash
> to ensure consistent results even if some future change somehow
> pollutes the configuration globally. Therefore:
>
>     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"
>> +'
>
> With the addition of these three new tests, aside from the
> introductory 'test_{un}config', this exact sequence of commands is now
> repeated four times in the script. Such repetition suggests that the
> common code should be moved to a function. For instance:
>
>     test_rebase_autostash () {
>         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"
>     }
>
> And, a caller would look like this:
>
>     test_expect_success 'pull ... rebase.autostash=true' '
>         test_config rebase.autostash true &&
>         test_rebase_autostash
>     '
>
> Of course, you'd also update the original test, from which this code
> was copied, to also call the new function. Factoring out the common
> code into a function should probably be done as a separate preparatory
> patch.
>
> This suggestion isn't mandatory and doesn't demand a re-roll, but, if
> you're feeling ambitious, it would make the code easier to digest and
> review.

>> +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
>
> I don't care strongly, but many tests consider test_must_fail() alone
> sufficient to verify proper behavior and don't bother being more exact
> by checking the precise error message (since error messages sometimes
> get refined, thus requiring adjustments to the tests). If you do

>> +'
>> +
>> +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_expect_success 'pull --rebase --no-autostash & rebase.autostash unset' '
>
> Same comment as above:
>
>     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
>> +'
>
> Same comment as above about the common code shared by these three new
> test: moving it to a function is suggested.
>
>> +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
>
> Same comment as above about checking the exact error message (vs. just
> trusting test_must_fail).
>
> Also, you mentioned in your cover letter that you couldn't use
> test_i18ngrep because grep was mistaking "--[no-]autostash" in the
> above expression as a command-line option. If you were using the exact
> string as above as an argument to test_i18ngrep, then it is more
> likely that the problem was that grep was seeing "[no-]" as a
> character class rather than as a literal pattern to match. You could
> get around this either by escaping the [ and ] with a backslash (\) or
> by passing -F to test_i18ngrep.
>
> Alternately, as mentioned above, just grep for a fragment of the error
> message, such as "only valid with --rebase", rather than the full
> diagnostic.
>
>> +'
>> +
>> +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
>> +'
>
> Same comment as above about code common to these two tests. However,
> in this case, it might be easier simply to use a 'for' loop rather
> than a function:
>
>     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
>
> Take special note of how use of double (") and single (') quotes
> differ in this case from other tests since $i needs to be interpolated
> into the test body.