Neels Janosch Hofmeyr wrote on Thu, 5 Jun 2008 at 03:25 +0200:
> Daniel Shahaf wrote:
> >> + 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)
>
Don't confuse me with the "facts"; I think SVN_ERR_INVALID_UTF_8 is
a better error code than APR_EINVAL here -- and I'll still think
so even if it doesn't exist. :)
> ;)
> 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.)
>
Why do you think that it "wouldn't make sense"? Do you think this
information is useless to callers of these APIs?
>
>
> >> +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?
>
How could it be something different? You pass revprop_table to
svn_repos_get_commit_editor5(), which documents that it expects "a hash
mapping <tt>const char *</tt> property names to @c svn_string_t values".
>
> >> + /* 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?
>
Files are always added to HEAD, so I don't understand your question.
(Did we grow some reverse-obliterate feature? Retroactively add files
to previous revisions?)
>
> >> + /* 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.
>
Errr, right. Hmm, I suppose it could be made clearer, for example
- svn_error_clear(err);
+ else
+ svn_error_clear(err);
but it's probably my fault for reviewing the patch before I woke up.
> > Looks good.
> Thanks :)
> But, with your improvements it looks much much better!
>
> A new version of the patch coming right up...
>
>
Thanks :) I'll look at the new version some day. (Slightly crazy
schedule here.)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-06-05 06:57:24 CEST