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

Quoting Julian Foad <julianfoad_at_btopenworld.com>:

> Stephen,
>
> Did you have a chance to update this patch? Would you like me to
> something with it?

Hi Julian,

I tidied up the patch, following your recommendations, and tested it.
I'll commit it to a new feature branch, like the branch we made for
diff-callbacks.

BTW, I'll rework the diff-callbacks, too.

Thanks for the feedback!

Steve

>
> - 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

-- 
Stephen Butler | Software Developer
elego Software Solutions GmbH
Gustav-Meyer-Allee 25 | 13355 Berlin | Germany
fon: +49 30 2345 8696 | mobile: +49 163 25 45 015
fax: +49 30 2345 8695 | http://www.elegosoft.com
Geschäftsführer: Olaf Wagner | Sitz der Gesellschaft: Berlin
Amtsgericht Charlottenburg HRB 77719 | USt-IdNr: DE163214194
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org