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

[Mehul Jain <mehul.jain2029@gmail.com>]

On Fri, Mar 25, 2016 at 2:01 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> +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".

That's a mistake. All test titles must spell "rebase.autoStash".

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

Actually test_config unset the config variable once the test is complete.
Thus I felt that test_unconfig might not be needed.

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

But considering this point, I'm convinced that indeed test_unconfig
should have been used.

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

Nice. This will increase fluency of the code and also lead to significant
reduction in number of new lines introduced by this patch.

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

Main reason to use test_i18ngrep here and check for this specific
error is that in future if some developer make changes which might
trigger git-pull not to die at die_on_unclean_work_tree() check (if
work tree is dirty) but leads git-pull to die somewhere else then
basically he/she will not understand the bug introduced by him/her as
test "pull --rebase --no-autostash & rebase.autostash=true" might pass.
test_i18ngrep will make sure that this does not happen.

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

Yes, that will work too as no other error messages for git-pull contain these
words.

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

I agree with all of these comments. I will introduce two new function to
reduce the code and the above mention loop. Also the work on Matthieu's
comment.

I feel that most of your comments are necessary and should be there in
the next patch. But I have a doubt regarding the next patch. As Junio has
merged v10 of current series in next branch (as noticed from his mail),
sending a new patch should be based on the current patch (i.e. on next
branch) or master branch (i.e. continuing with this series)?

Thanks,
Mehul