[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] consistent out-of-date handling by commit

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Thu, 06 Mar 2008 10:24:39 +0000

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-06 11:25:02 CET

This is an archived mail posted to the Subversion Dev mailing list.