You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2008/03/05 20:50:45 UTC
Re: [PATCH] consistent out-of-date handling by commit
Stephen Butler wrote:
> Hi folks,
>
> Here is a patch that resolves issue #2740 ("out-of-date" error text
> varies depending on access method) and plugs a loophole in the commit
> safety net, as far as tree conflict detection is concerned.
>
> I mentioned this patch in an earlier email about the behavior of merge
> (http://svn.haxx.se/dev/archive-2008-02/0872.shtml). Our current tree
> conflict detection scheme relies on this patch, but this patch is not
> specific to tree conflicts.
Stephen,
This sounds like you're doing the right thing. I have reviewed the patch today
and have only a few simple observations. I'd be happy to see you commit this
(or I'll commit it for you) if you can address them.
> [[[
>
> When a Subversion client tries to commit a deletion, a property
> modification, or a text modification, and the item no longer exists in
> the repository, the commit should fail with an "out of date" error.
>
> The error message should be consistent for all remote access methods
> (v. issue #2740).
>
> * subversion/libsvn_client/commit_util.c
> (out_of_date): New function.
> (do_item_commit): To give the user an "out of date" error instead of
> "not found", replace SVN_ERR() with manual error handling when
> asking the repo to delete dirs or files, to open files, or to
> receive property deltas during commit.
>
> * subversion/libsvn_client/commit.c
> (svn_client_commit4): Give the user an "out of date" error instead
> of "not found", if the RA editor can't be initialized.
>
> * subversion/libsvn_ra_serf/commit.c
> (delete_entry): Treat the response '404 Not Found' as an error.
>
> * subversion/libsvn_ra_neon/commit.c
> (commit_delete_entry): Treat the response '404 Not Found' as an error.
>
> * subversion/libsvn_repos/commit.c
> (out_of_date): Extend for case where we don't know the node type
> because the node does not exist.
> (delete_entry): If a path is not found, return an error.
>
> * subversion/tests/cmdline/commit_tests.py
> (commit_out_of_date_deletions): Extend test cases to include all
> relevant combinations of deletion vs (property-change | text-change |
> deletion). Use svntest.actions.run_and_verify_commit consistently.
>
> ]]]
>
> Index: subversion/libsvn_client/commit_util.c
> ===================================================================
> --- subversion/libsvn_client/commit_util.c (revision 29632)
> +++ subversion/libsvn_client/commit_util.c (working copy)
> @@ -44,6 +44,16 @@
> #define SVN_CLIENT_COMMIT_DEBUG
> */
>
> +/* Create and return a generic out-of-dateness error. */
> +static svn_error_t *
> +out_of_date(const char *path, svn_node_kind_t kind)
> +{
> + return svn_error_createf(SVN_ERR_WC_NOT_UP_TO_DATE, NULL,
> + (kind == svn_node_dir
> + ? _("Directory '%s' is out of date")
> + : _("File '%s' is out of date")),
> + path);
> +}
>
>
> /*** Harvesting Commit Candidates ***/
> @@ -1082,6 +1092,7 @@ do_item_commit(void **dir_baton,
> apr_hash_t *file_mods = cb_baton->file_mods;
> const char *notify_path_prefix = cb_baton->notify_path_prefix;
> svn_client_ctx_t *ctx = cb_baton->ctx;
> + svn_error_t *err = SVN_NO_ERROR;
>
> /* Do some initializations. */
> *dir_baton = NULL;
> @@ -1191,8 +1202,17 @@ do_item_commit(void **dir_baton,
> if (item->state_flags & SVN_CLIENT_COMMIT_ITEM_DELETE)
> {
> assert(parent_baton);
> - SVN_ERR(editor->delete_entry(path, item->revision,
> - parent_baton, pool));
> + 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."
> + {
> + svn_error_clear(err);
> + return out_of_date(path, kind);
> + }
> + return err;
> + }
> }
>
> /* If this item is supposed to be added, do so. */
> @@ -1248,9 +1268,19 @@ do_item_commit(void **dir_baton,
> if (! file_baton)
> {
> assert(parent_baton);
> - SVN_ERR(editor->open_file(path, parent_baton,
> - item->revision,
> - file_pool, &file_baton));
> + err = editor->open_file(path, parent_baton,
> + item->revision,
> + file_pool, &file_baton);
> + if (err)
> + {
> + if (err->apr_err == SVN_ERR_FS_NOT_FOUND
> + || err->apr_err == SVN_ERR_RA_DAV_PATH_NOT_FOUND)+ {
(A glitch in the patch there, but I inserted the missing newline by hand.)
> + svn_error_clear(err);
> + return out_of_date(path, kind);
> + }
> + return err;
> + }
> +
> }
> }
> else
> @@ -1259,24 +1290,38 @@ do_item_commit(void **dir_baton,
[...]
>
> SVN_ERR(svn_wc_entry(&tmp_entry, item->path, adm_access, TRUE, pool));
> - SVN_ERR(svn_wc_transmit_prop_deltas
> - (item->path, adm_access, tmp_entry, editor,
> - (kind == svn_node_dir) ? *dir_baton : file_baton, NULL, pool));
>
> + /* When committing a dir that no longer exists in the repo, the
> + "not found" error appears during the delta transmisssion. */
> + err = svn_wc_transmit_prop_deltas
> + (item->path, adm_access, tmp_entry, editor,
> + (kind == svn_node_dir) ? *dir_baton : file_baton, NULL, pool);
> +
> + if (err)
> + {
> + if (err->apr_err == SVN_ERR_FS_NOT_FOUND
> + || err->apr_err == SVN_ERR_RA_DAV_PATH_NOT_FOUND)
> + {
> + svn_error_clear(err);
> + return out_of_date(path, kind);
> + }
> + return err;
> + }
> +
> /* Make any additional client -> repository prop changes. */
> if (item->outgoing_prop_changes)
> {
> @@ -1312,9 +1357,19 @@ do_item_commit(void **dir_baton,
> if (! file_baton)
> {
> assert(parent_baton);
> - SVN_ERR(editor->open_file(path, parent_baton,
> - item->revision,
> - file_pool, &file_baton));
> + err = editor->open_file(path, parent_baton,
> + item->revision,
> + file_pool, &file_baton);
> + if (err)
> + {
> + if (err->apr_err == SVN_ERR_FS_NOT_FOUND
> + || err->apr_err == SVN_ERR_RA_DAV_PATH_NOT_FOUND)
> + {
> + svn_error_clear(err);
> + return out_of_date(path, svn_node_file);
> + }
> + return err;
> + }
Rather than repeating this code four times, please could you put it in the
helper function "out_of_date()"? I think it would fit very neatly there, with
the helper's purpose described as something like:
* If the error ERR is one that indicates an out-of-date
* condition, replace it with [or, at your option, 'append to it']
* a specific and consistent out-of-date error.
> }
>
> /* Add this file mod to the FILE_MODS hash. */
> Index: subversion/libsvn_client/commit.c
> ===================================================================
> --- subversion/libsvn_client/commit.c (revision 29632)
> +++ subversion/libsvn_client/commit.c (working copy)
> @@ -1689,8 +1689,18 @@ svn_client_commit4(svn_commit_info_t **commit_info_p,
> base_url, base_dir, base_dir_access,
> log_msg, commit_items, commit_info_p,
> TRUE, lock_tokens, keep_locks, pool)))
> - goto cleanup;
> -
> + {
> + /* For consistency, re-raise a "not found" error as "out of date". */
> + 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.
> + }
> + goto cleanup;
> + }
> Index: subversion/libsvn_ra_neon/commit.c
> ===================================================================
> --- subversion/libsvn_ra_neon/commit.c (revision 29632)
> +++ subversion/libsvn_ra_neon/commit.c (working copy)
> @@ -751,15 +751,11 @@ static svn_error_t * commit_delete_entry(const char *p
> APR_HASH_KEY_STRING, SVN_DAV_OPTION_KEEP_LOCKS);
> }
>
> - /* 404 is ignored, because mod_dav_svn is effectively merging
> - against the HEAD revision on-the-fly. In such a universe, a
> - failed deletion (because it's already missing) is OK; deletion
> - is an idempotent merge operation. */
> serr = svn_ra_neon__simple_request(&code, parent->cc->ras,
> "DELETE", child,
> extra_headers, NULL,
> - 204 /* Created */,
> - 404 /* Not Found */, pool);
> + 204 /* No Content */,
> + NULL, pool);
That should be "0" rather than "NULL" as it's a number rather than a pointer.
Otherwise, this is fine.
The tests (in commit_tests.py 28):
# Path Repo WC backup
# =========== ==== =========
# A/C pset del
# A/I del pset
# A/B/F del del
# A/D/H/omega text del
# A/B/E/alpha pset del
# A/D/H/chi del text
# A/B/E/beta del pset
# A/D/H/psi del del
By modifying the test suite to print the output, I see that each of the 8 cases
tested results in the line
svn: Commit failed (details follow):
followed by one "out of date" message, respectively:
svn: Directory '/A/C' is out of date
svn: Directory '' is out of date
svn: File or directory '/A/B/F' is out of date
svn: File '/A/D/H/omega' is out of date
svn: File '/A/B/E/alpha' is out of date
svn: File 'chi' is out of date
svn: File 'beta' is out of date
svn: File or directory '/A/D/H/psi' is out of date
So we see (i) ambiguity in the object's kind in the delete-and-delete cases,
(ii) lack of full relative path in some cases, and (iii) a complete absence of
any path identifier in one case. I believe (i) and (ii) are known
inconsistencies due to lack of information where they're detected, and I think
they're OK, but is (iii) something we can and should correct now? It's probably
a manifestation of (ii) in the case where the relative path being changed is ""
(properties on "this dir").
If this isn't something that you've made worse and isn't something you can
easily fix, that's fine, we can leave it for another time. (I should re-run the
test with your changes reverted to see what it was like before, but I don't
have time right now.)
Finally, before you commit please run a tab-to-space conversion as you inserted
a few tabs.
Thanks,
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] consistent out-of-date handling by commit
Posted by Stephen Butler <sb...@elego.de>.
Quoting Julian Foad <ju...@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 <ju...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] consistent out-of-date handling by commit
Posted by Julian Foad <ju...@btopenworld.com>.
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 <ju...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] consistent out-of-date handling by commit
Posted by Julian Foad <ju...@btopenworld.com>.
Stephen Butler wrote:
> Quoting Julian Foad <ju...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] consistent out-of-date handling by commit
Posted by Stephen Butler <sb...@elego.de>.
Quoting Julian Foad <ju...@btopenworld.com>:
> Stephen Butler wrote:
>> Hi folks,
>>
>> Here is a patch that resolves issue #2740 ("out-of-date" error text
>> varies depending on access method) and plugs a loophole in the commit
>> safety net, as far as tree conflict detection is concerned.
> This sounds like you're doing the right thing. I have reviewed the
> patch today and have only a few simple observations. I'd be happy to
> see you commit this (or I'll commit it for you) if you can address them.
>
>> Index: subversion/libsvn_client/commit_util.c
>> ===================================================================
>> --- subversion/libsvn_client/commit_util.c (revision 29632)
>> +++ subversion/libsvn_client/commit_util.c (working copy)
>> /*** Harvesting Commit Candidates ***/
>> @@ -1082,6 +1092,7 @@ do_item_commit(void **dir_baton,
>> apr_hash_t *file_mods = cb_baton->file_mods;
>> const char *notify_path_prefix = cb_baton->notify_path_prefix;
>> svn_client_ctx_t *ctx = cb_baton->ctx;
>> + svn_error_t *err = SVN_NO_ERROR;
>> /* Do some initializations. */
>> *dir_baton = NULL;
>> @@ -1191,8 +1202,17 @@ do_item_commit(void **dir_baton,
>> if (item->state_flags & SVN_CLIENT_COMMIT_ITEM_DELETE)
>> {
>> assert(parent_baton);
>> - SVN_ERR(editor->delete_entry(path, item->revision,
>> - parent_baton, pool));
>> + 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.
>> @@ -1312,9 +1357,19 @@ do_item_commit(void **dir_baton,
>> if (! file_baton)
>> {
>> assert(parent_baton);
>> - SVN_ERR(editor->open_file(path, parent_baton,
>> - item->revision,
>> - file_pool, &file_baton));
>> + err = editor->open_file(path, parent_baton,
>> + item->revision,
>> + file_pool, &file_baton);
>> + if (err)
>> + {
>> + if (err->apr_err == SVN_ERR_FS_NOT_FOUND
>> + || err->apr_err == SVN_ERR_RA_DAV_PATH_NOT_FOUND)
>> + {
>> + svn_error_clear(err);
>> + return out_of_date(path, svn_node_file);
>> + }
>> + return err;
>> + }
>
> Rather than repeating this code four times, please could you put it in
> the helper function "out_of_date()"? I think it would fit very neatly
> there, with the helper's purpose described as something like:
>
> * If the error ERR is one that indicates an out-of-date
> * condition, replace it with [or, at your option, 'append to it']
> * a specific and consistent out-of-date error.
OK. Will do.
>
>> }
>> /* Add this file mod to the FILE_MODS hash. */
>> Index: subversion/libsvn_client/commit.c
>> ===================================================================
>> --- subversion/libsvn_client/commit.c (revision 29632)
>> +++ subversion/libsvn_client/commit.c (working copy)
>> @@ -1689,8 +1689,18 @@ svn_client_commit4(svn_commit_info_t **commit_info_p,
>> base_url, base_dir, base_dir_access,
>> log_msg, commit_items, commit_info_p,
>> TRUE, lock_tokens, keep_locks, pool)))
>> - goto cleanup;
>> -
>> + {
>> + /* For consistency, re-raise a "not found" error as "out of date". */
>> + 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.
>
>> + }
>> + goto cleanup;
>> + }
>
>> Index: subversion/libsvn_ra_neon/commit.c
>> ===================================================================
>> --- subversion/libsvn_ra_neon/commit.c (revision 29632)
>> +++ subversion/libsvn_ra_neon/commit.c (working copy)
>> @@ -751,15 +751,11 @@ static svn_error_t * commit_delete_entry(const char *p
>> APR_HASH_KEY_STRING, SVN_DAV_OPTION_KEEP_LOCKS);
>> }
>> - /* 404 is ignored, because mod_dav_svn is effectively merging
>> - against the HEAD revision on-the-fly. In such a universe, a
>> - failed deletion (because it's already missing) is OK; deletion
>> - is an idempotent merge operation. */
>> serr = svn_ra_neon__simple_request(&code, parent->cc->ras,
>> "DELETE", child,
>> extra_headers, NULL,
>> - 204 /* Created */,
>> - 404 /* Not Found */, pool);
>> + 204 /* No Content */,
>> + NULL, pool);
>
> That should be "0" rather than "NULL" as it's a number rather than a
> pointer. Otherwise, this is fine.
OK.
>
>
> The tests (in commit_tests.py 28):
>
> # Path Repo WC backup
> # =========== ==== =========
> # A/C pset del
> # A/I del pset
> # A/B/F del del
> # A/D/H/omega text del
> # A/B/E/alpha pset del
> # A/D/H/chi del text
> # A/B/E/beta del pset
> # A/D/H/psi del del
>
> By modifying the test suite to print the output, I see that each of the
> 8 cases tested results in the line
> svn: Commit failed (details follow):
>
> followed by one "out of date" message, respectively:
> svn: Directory '/A/C' is out of date
> svn: Directory '' is out of date
> svn: File or directory '/A/B/F' is out of date
> svn: File '/A/D/H/omega' is out of date
> svn: File '/A/B/E/alpha' is out of date
> svn: File 'chi' is out of date
> svn: File 'beta' is out of date
> svn: File or directory '/A/D/H/psi' is out of date
>
> So we see (i) ambiguity in the object's kind in the delete-and-delete
> cases, (ii) lack of full relative path in some cases, and (iii) a
> complete absence of any path identifier in one case. I believe (i) and
> (ii) are known inconsistencies due to lack of information where they're
> detected, and I think they're OK, but is (iii) something we can and
> should correct now? It's probably a manifestation of (ii) in the case
> where the relative path being changed is "" (properties on "this dir").
>
> If this isn't something that you've made worse and isn't something you
> can easily fix, that's fine, we can leave it for another time. (I
> should re-run the test with your changes reverted to see what it was
> like before, but I don't have time right now.)
I'll investigate where the path string comes from, and see if I can
make it more consistent.
>
> Finally, before you commit please run a tab-to-space conversion as you
> inserted a few tabs.
Whoops, will fix.
>
> Thanks,
> - 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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org