On Fri, Mar 25, 2016 at 2:01 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html