Stephen,
Did you have a chance to update this patch? Would you like me to something with it?
- Julian
Julian Foad wrote:
> Stephen Butler wrote:
>
>> Quoting Julian Foad <julianfoad_at_btopenworld.com>:
>>
>>>> + err = editor->delete_entry(path, item->revision,
>>>> parent_baton, pool);
>>>> + if (err)
>>>> + {
>>>> + if (err->apr_err == SVN_ERR_FS_NOT_FOUND
>>>> + || err->apr_err == SVN_ERR_RA_DAV_PATH_NOT_FOUND)
>>>
>>>
>>> So there are two errors that definitely mean "out of date", and these
>>> are them? I've no reason to doubt it, but nor do I know why. I'd be
>>> happy to hear you confirm, "Yes, there are exactly two such errors and
>>> those are them, in all four cases that we deal with in this function."
>>
>>
>> Yes indeed, SVN_ERR_FS_NOT_FOUND is sent by the file or svn RA layers,
>> and SVN_ERR_RA_DAV_PATH_NOT_FOUND is sent by the http/https RA layers,
>> with the same meaning.
>
>
> OK.
>
> It sounds to me like all the RA layers ought to be made to return the
> same error code for this case. That's a separate issue, though.
>
>
>>>> + if (cmt_err->apr_err == SVN_ERR_FS_NOT_FOUND
>>>> + || cmt_err->apr_err == SVN_ERR_RA_DAV_PATH_NOT_FOUND)
>>>> + {
>>>> + svn_error_clear(cmt_err);
>>>> + cmt_err = svn_error_createf(SVN_ERR_WC_NOT_UP_TO_DATE, NULL,
>>>> + _("Directory '%s' is out of
>>>> date"),
>>>> + base_dir);
>>>
>>>
>>> I can see the point of giving an "out of date" error for consistency. I
>>> can't see any value in hiding the lower-level errors that lead to this
>>> diagnosis. I would expect you to add the "out of date" error to the
>>> chain of errors. I tried doing so and your tests still pass. Not a big
>>> deal, but do you have an opinion one way or the other?
>>>
>>> The same question applies to the four cases in commit_util.c above.
>>
>>
>> I suggest we give the SVN_ERR_WC_NOT_UP_TO_DATE error code in all cases,
>> because the not-found state is due to changes in the repo.
>
>
> I agree we should return an SVN_ERR_WC_NOT_UP_TO_DATE error code in all
> cases.
>
> I was asking if there is any reason for the "svn_error_clear()" which
> removes the underlying error(s) from the chain of errors. Usually we
> append the high-level error code to the chain of low-level errors that
> we receive from a called function, in order to be able to display more
> detail about why the error happened.
>
> So, your patch returns this error chain:
>
> SVN_ERR_WC_NOT_UP_TO_DATE
>
> I am suggesting it should instead return either this error chain:
>
> SVN_ERR_WC_NOT_UP_TO_DATE,
> SVN_ERR_FS_NOT_FOUND
>
> or this error chain:
>
> SVN_ERR_WC_NOT_UP_TO_DATE,
> SVN_ERR_RA_DAV_PATH_NOT_FOUND
>
> by executing "cmt_err = svn_error_createf(SVN_ERR_WC_NOT_UP_TO_DATE,
> cmt_err, ...)".
>
> Regards,
> - Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-03-31 17:49:56 CEST