[PATCH] add a test of svn_subst_translate_string2() to the subst_translate-test test suite (v. 2)

Attached is version 2 of the patch and log message.

>> +    strncpy(orig_lc_all, setlocale(LC_ALL, NULL), sizeof orig_lc_all);
>
> sizeof() with parens please.

Fixed.

>> +    if ((! setlocale(LC_ALL, "English.1252")) &&
>> +        (! setlocale(LC_ALL, "German.1252")) &&
>> +        (! setlocale(LC_ALL, "French.1252")) &&
>
> What are these three?  Are they Windows syntax?

Yes.

>> +        (! setlocale(LC_ALL, "en_US.ISO-8859-1")) &&
>> +        (! setlocale(LC_ALL, "en_GB.ISO-8859-1")) &&
>> +        (! setlocale(LC_ALL, "de_DE.ISO-8859-1")))
>> +      return svn_error_createf(SVN_ERR_TEST_SKIPPED, NULL, "None of the locales English.1252, German.1252, French.1252, en_US.ISO-8859-1, en_GB.ISO-8859-1, and de_DE.ISO-8859-1 are installed.");
>
> The error message is too verbose, and doesn't fit within 80 columns.

Currently the error message is not displayed if none of the locales
are available; the output is simply:

SKIP: lt-subst_translate-test 2: test
svn_subst_translate_string2(encoding = NULL)

I wrapped it to 80 characters, but I would rather not change the
verbosity of the text because I like how it indicates what was tried
and also that setlocale() fails because a locale is not installed.

>> +
>> +    SVN_ERR(svn_subst_translate_string2(&new_value, &translated_to_utf8,
>> +                                        &translated_line_endings,
>> +                                        source_string, NULL, FALSE,
>> +                                        pool, pool));
>> +    SVN_TEST_STRING_ASSERT(new_value->data, "\xc3\x86");
>> +    SVN_TEST_ASSERT(translated_to_utf8 == TRUE);
>> +    SVN_TEST_ASSERT(translated_line_endings == FALSE);
>> +
>> +    SVN_TEST_ASSERT(setlocale(LC_ALL, orig_lc_all) != NULL);
>> +  }
>
> So you only restore the locale if the test passed.  How much does it
> matter?

It might cause problems in subsequent tests in the same test suite.

Using one of Brane's suggestions
(http://thread.gmane.org/gmane.comp.version-control.subversion.devel/125792/focus=125795),
the new patch uses a wrapper function to ensure that the original
LC_ALL locale is restored before the test returns.

>> +
>> +  return SVN_NO_ERROR;
>> +}
>> +
>> +static svn_error_t *
>>  test_repairing_svn_subst_translate_string2(apr_pool_t *pool)
>>  {
>>      {
>> @@ -171,6 +210,8 @@ struct svn_test_descriptor_t test_funcs[] =
>>      SVN_TEST_NULL,
>>      SVN_TEST_PASS2(test_svn_subst_translate_string2,
>>                     "test svn_subst_translate_string2()"),
>> +    SVN_TEST_PASS2(test_svn_subst_translate_string2_null_encoding,
>> +                   "test svn_subst_translate_string2(), ENCODING = 0"),
>
> I know you are limited to 50 chars, but this "= 0" sacrifices
> readability IMHO.  IMO, there are clearer alternatives, e.g.:
>    "test svn_subst_translate_string2(encoding=NULL)"

I like that. Changed.