Daniel Shahaf wrote:
>> Also adding regression test for 1796.
> ^
> 1796 what?
potatoes :)
(changed)
>> + if (svn_utf__is_valid(value->data, value->len) == FALSE)
>> + return svn_error_create
>> + (APR_EINVAL, NULL,
>> + _("Cannot accept log message because it is not encoded in "
>> + "UTF-8"));
>> +
>
> More specific error code (SVN_ERR_INVALID_UTF_8)?
You mean, I should create a new error code?
(I can't find SVN_ERR_INVALID_UTF_8 anywhere)
;)
kfogel might say: "Every error code got created some day"
> [...]
> Please also update svn_repos_fs_change_node_prop's docstring, which
> claims that only SVN_PROP_REVISION_DATE is validated.
>
> Hmm, that docstring (and validate_prop()'s docstring) also promise that
> you'll return SVN_ERR_BAD_PROPERTY_VALUE. So you need to wrap your
> errors by that error.
That's right, so it would suffice to just throw a
SVN_ERR_BAD_PROPERTY_VALUE for both cases of inalid UTF-8 and
inconsistent EOL style, and I don't need a SVN_ERR_INVALID_UTF_8 even if
it was there already. (The function validate_prop directly correlates
with the mentioned comment, so it wouldn't make sense to throw
differentiated errors only to wrap them to a more general error within
the same function.)
>> +static svn_error_t *
>> +prop_validation_commit_with_revprop(const char *filename,
>> + const void *prop_key,
>> + apr_ssize_t prop_klen,
>> + const void *prop_val,
>> + svn_repos_t *repos,
>
> Why are they "const void *"?
...because the arguments to apr_hash_set() are also const void*. I've
changed prop_key to const char*, but left prop_val a const void*, since,
technically, it could be something different (depending on prop_klen,
see apr_hash_set()). Is that good enough?
>> + /* Make an arbitrary change and commit using above values... */
>> +
>> + SVN_ERR(svn_repos_get_commit_editor5(&editor, &edit_baton, repos,
>> + NULL, "file://test", "/",
>> + revprop_table, NULL, NULL,
>> + NULL, NULL, pool));
>> +
>> + SVN_ERR(editor->open_root(edit_baton, 1, pool, &root_baton));
> ^
>
> Should this be 1 or 0? I'm not familiar with the editor or the C tests
> infrastructure, but based on the docstrings I think this should be zero?
> (I understand the code works, but I'd like to understand why.)
I just copied this from another test and didn't pay attention to this
parameter.
Hmm, I can just give base_revision a zero and files will still be added
to the HEAD, right?
>> + /* Test an invalid commit log message: LF */
>> + err = prop_validation_commit_with_revprop
[...]
>> + if (err == SVN_NO_ERROR)
>> + return svn_error_create(SVN_ERR_TEST_FAILED, err,
>> + "Failed to reject a log with inconsistent "
>> + "line ending style");
>> + else
>> + if (err->apr_err != SVN_ERR_IO_INCONSISTENT_EOL)
>> + return svn_error_create(SVN_ERR_TEST_FAILED, err,
>> + "Expected SVN_ERR_IO_INCONSISTENT_EOL, "
>> + "got another error.");
>> + svn_error_clear(err);
>> +
>
> So svn_error_clear() runs for two paths: on the "correct" error and on
> SVN_NO_ERROR.
No, it doesn't... It only runs for the third path, where err->apr_err ==
SVN_ERR_IO_INCONSISTENT_EOL, because of the return statements.
> Looks good.
Thanks :)
But, with your improvements it looks much much better!
A new version of the patch coming right up...
--
Neels Hofmeyr -- elego Software Solutions GmbH
Gustav-Meyer-Allee 25 / Gebäude 12, 13355 Berlin, Germany
phone: +49 30 23458696 mobile: +49 177 2345869 fax: +49 30 23458695
http://www.elegosoft.com | Geschäftsführer: Olaf Wagner | Sitz: Berlin
Handelsreg: Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
Received on 2008-06-05 03:26:27 CEST