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

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Mar 21, 2016 at 4:12 PM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
> If rebase.autoStash configuration variable is set, there is no way to
> override it for "git pull --rebase" from the command line.
>
> Teach "git pull --rebase" the --[no-]autostash command line flag which
> overrides the current value of rebase.autoStash, if set. As "git rebase"
> understands the --[no-]autostash option, it's just a matter of passing
> the option to underlying "git rebase" when "git pull --rebase" is called.

This version of the patch (coupled with patch 1/2) is a pleasant
improvement over previous versions due to the cleaner structure, less
noisy diff, and general simplicity (thus easier to reason about and
review).

See below for a nit and some comments about the tests.

> Signed-off-by: Mehul Jain <mehul.jain2029@gmail.com>
> ---
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> @@ -256,6 +256,76 @@ 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' '

Nit: Some of the test titles spell this as "rebase.autostash" while
others use "rebase.autoStash".

> +       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"
> +'
> +
> +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
retain the error message check, it's often sufficient to check for
just a fragment of the error string rather than the full message. For
instance, it might be fine to grep merely for "uncommitted changes".

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