Daniel Shahaf wrote:
> Neels Janosch Hofmeyr wrote on Thu, 5 Jun 2008 at 03:25 +0200:
>> Daniel Shahaf wrote:
>>> [...]
>>> 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?
The point is that validate_prop() is currently a private function in
subversion/libsvn_repos/fs-wrap.c, and that it is only ever used in
cases where the specification is to return only
SVN_ERR_BAD_PROPERTY_VALUE. To return more precise errors would mean
that the callers in fs-wrap.c would *all* wrap code around the
validate_prop() call that abstracts every error to
SVN_ERR_BAD_PROPERTY_VALUE.
It would make more sense (I tried to say) to just make validate_prop()
return SVN_ERR_BAD_PROPERTY_VALUE, until someone actually needs more
differentiated error codes.
>>>> +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".
Hm, that's true. The patch being committed already, I might post another
patch to change that.
>> 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.
Question already answered :)
> (Did we grow some reverse-obliterate feature? Retroactively add files
> to previous revisions?)
No, I'm just a noob asking questions.
>>>> + /* 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);
Well, it's used in the current way in dozens of other places in that
file, so I guess it's appropriate to stick with that.
> but it's probably my fault for reviewing the patch before I woke up.
I'd say you did pretty good...
We've got it in upstream, nice work :)
--
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-09 02:03:31 CEST