Quoting Julian Foad <julianfoad_at_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_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-03-06 09:24:42 CET