Attached is version 3 of the patch and log message.
>> >> + (! 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)
>>
>
> Oddly, the message isn't displayed even with --verbose. But if you
> write an error message, I think it's best to assume it'll be read by
> someone. (or else don't write it at all)
>
>> 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.
>>
>
> +1 on leaving the latter detail in. However, the error message just
> duplicates the code; those details should be conveyed by err->file and
> err->line, not by err->message.
>
> So, sorry, but I'm not yet convinced that listing all the locales in the
> log message is a good idea.
Okay. I changed it to "Tried %d locales, but none are installed."
>> [[[
>> Add a test of svn_subst_translate_string2() to the subst_translate test
>> suite.
>>
>> As discussed at:
>> http://thread.gmane.org/gmane.comp.version-control.subversion.devel/125782
>>
>
> Message-ID, please, for forward compatibility. (The apache.org and
> gmane.org archives allow you to embed the Message-ID in the URL; or just
> give it separately.)
Okay. The Message-ID is
<[email protected]>, which
is provided separately because I like how the Gmane link displays the
whole discussion thread.
>> * subversion/tests/libsvn_subr/subst_translate-test.c
>> (test_svn_subst_translate_string2_null_encoding_helper): New function. It
>> is
>> the core of the new svn_subst_translate_string2_null_encoding test.
>> (test_svn_subst_translate_string2_null_encoding): New test that tests
>> svn_subst_translate_string2() with ENCODING set to NULL. It wraps
>> test_svn_subst_translate_string2_null_encoding_helper() to ensure that
>> the
>> system-default character encoding is set to either CP-1252 or ISO-8859-1
>> before test_svn_subst_translate_string2_null_encoding_helper() is called,
>> later restoring the original system-default character encoding.
>
> This amount of information ought to be a docstring, not a log message.
Fixed.
>> (test_funcs): Add test_svn_subst_translate_string2_null_encoding().
>> ]]]
>
>> @@ -99,7 +102,64 @@ test_svn_subst_translate_string2(apr_pool_t *pool)
>> return SVN_NO_ERROR;
>> }
>>
>> +/* The body of the svn_subst_translate_string2_null_encoding test. It should
>> + only be called by test_svn_subst_translate_string2_null_encoding(), as
>> this
>> + code assumes that the process locale has been changed to a locale that
>> uses
>> + either CP-1252 or ISO-8859-1 for the default narrow string encoding. */
>> static svn_error_t *
>> +test_svn_subst_translate_string2_null_encoding_helper(apr_pool_t *pool)
>> +{
>> + {
>> + svn_string_t *new_value = NULL;
>> + svn_boolean_t translated_to_utf8 = FALSE;
>> + svn_boolean_t translated_line_endings = TRUE;
>
> Why did you initialize these two variables? The API will always set
> them for you, so you shouldn't initialize them.
It's part of the test to check that the API sets them.
>> + /* 'Æ', which is 0xc6 in both ISO-8859-1 and Windows-1252 */
>> + svn_string_t *source_string = svn_string_create("\xc6", pool);
>> +
>> + 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);
>> + }
>> +
>> + return SVN_NO_ERROR;
>> +}
>> +
>> +/* Test that when ENCODING is NULL, the system-default language encoding is
>> used. */
>> +static svn_error_t *
>> +test_svn_subst_translate_string2_null_encoding(apr_pool_t *pool)
>> +{
>
> +1 on the rest; looks good.
>
> Thanks,
>
> Daniel
[[[
Add a test of svn_subst_translate_string2() to the subst_translate test suite.
As discussed at:
http://thread.gmane.org/gmane.comp.version-control.subversion.devel/125782
Message-ID: <[email protected]>
* subversion/tests/libsvn_subr/subst_translate-test.c
(test_svn_subst_translate_string2_null_encoding_helper): New function. It is
the core of the new svn_subst_translate_string2_null_encoding test.
(test_svn_subst_translate_string2_null_encoding): New test that tests
svn_subst_translate_string2() with ENCODING set to NULL.
(test_funcs): Add test_svn_subst_translate_string2_null_encoding().
]]]
Index: subversion/tests/libsvn_subr/subst_translate-test.c
===================================================================
--- subversion/tests/libsvn_subr/subst_translate-test.c (revision 1072544)
+++ subversion/tests/libsvn_subr/subst_translate-test.c (working copy)
@@ -21,12 +21,17 @@
* ====================================================================
*/
+#include <locale.h>
+#include <string.h>
+
#include "../svn_test.h"
#include "svn_types.h"
#include "svn_string.h"
#include "svn_subst.h"
+#define ARRAY_LEN(ary) ((sizeof (ary)) / (sizeof ((ary)[0])))
+
/* Test inputs and expected output for svn_subst_translate_string2(). */
struct translate_string2_data_t
{
@@ -99,7 +104,84 @@ test_svn_subst_translate_string2(apr_pool_t *pool)
return SVN_NO_ERROR;
}
+/* The body of the svn_subst_translate_string2_null_encoding test. It should
+ only be called by test_svn_subst_translate_string2_null_encoding(), as this
+ code assumes that the process locale has been changed to a locale that uses
+ either CP-1252 or ISO-8859-1 for the default narrow string encoding. */
static svn_error_t *
+test_svn_subst_translate_string2_null_encoding_helper(apr_pool_t *pool)
+{
+ {
+ svn_string_t *new_value = NULL;
+ svn_boolean_t translated_to_utf8 = FALSE;
+ svn_boolean_t translated_line_endings = TRUE;
+ /* 'Æ', which is 0xc6 in both ISO-8859-1 and Windows-1252 */
+ svn_string_t *source_string = svn_string_create("\xc6", pool);
+
+ 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);
+ }
+
+ return SVN_NO_ERROR;
+}
+
+/* Test that when ENCODING is NULL, the system-default language encoding is used.
+
+ This is a wrapper of test_svn_subst_translate_string2_null_encoding_helper()
+ that ensures that the system-default character encoding is set to either
+ CP-1252 or ISO-8859-1 before test_svn_subst_translate_string2_null_encoding_helper()
+ is called, later restoring the original system-default character encoding. */
+static svn_error_t *
+test_svn_subst_translate_string2_null_encoding(apr_pool_t *pool)
+{
+ char orig_lc_all[256] = { '\0' };
+ svn_error_t *test_result;
+
+ const char *other_locales[] =
+ {
+ /* For Windows' msvcrt */
+ "English.1252", "German.1252", "French.1252",
+
+ /* For glibc */
+ "en_US.ISO-8859-1", "en_GB.ISO-8859-1", "de_DE.ISO-8859-1",
+
+ /* For OpenBSD's libc */
+ "en_US.ISO8859-1", "en_GB.ISO8859-1", "de_DE.ISO8859-1",
+
+ /* Must be last */
+ NULL
+ };
+ const char **other_locale;
+
+ strncpy(orig_lc_all, setlocale(LC_ALL, NULL), sizeof (orig_lc_all));
+
+ for (other_locale = other_locales; *other_locale != NULL; ++other_locale)
+ {
+ if (setlocale(LC_ALL, *other_locale))
+ break;
+ }
+
+ /* If the end of the list of other locales to try has been reached, then none
+ of the tested locales are installed, so the test must be skipped. */
+ if (*other_locale == NULL)
+ return svn_error_createf(SVN_ERR_TEST_SKIPPED, NULL,
+ "Tried %d locales, but none are installed.",
+ (int) (ARRAY_LEN(other_locales) - 1));
+
+ test_result = test_svn_subst_translate_string2_null_encoding_helper(pool);
+
+ /* Restore the original locale for category LC_ALL. */
+ SVN_TEST_ASSERT(setlocale(LC_ALL, orig_lc_all) != NULL);
+
+ return test_result;
+}
+
+static svn_error_t *
test_repairing_svn_subst_translate_string2(apr_pool_t *pool)
{
{
@@ -171,6 +253,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 = NULL)"),
SVN_TEST_PASS2(test_repairing_svn_subst_translate_string2,
"test repairing svn_subst_translate_string2()"),
SVN_TEST_PASS2(test_svn_subst_translate_cstring2,
