Re: bug in repos_replay

Hi,

David Glasser wrote:
> On 1/24/07, David Glasser <glasser@mit.edu> wrote:
>> Does anyone know of other places in the code where we need to combine
>> an svn_delta_path_driver traversal with other editor calls that might
>> be somewhat out of sequence? It looks like this is asking for an "add
>> entries to the list that svn_delta_path_driver is running through"
>> API, though that might be overengineering.
>
> Hmm, here's a possibility: svn_delta_path_driver drives
> path_driver_cb_func, which finds out what the change it is simulating
> is via the changed_paths hash in its baton.
>
> path_driver_cb_func could provide changed_paths to add_subdir. If
> add_subdir notices that one of the files or directories it is adding
> has a changed_path entry, then it can call path_driver_cb_func itself.
> (We can make path_driver_cb_func erase its entry in changed_paths
> after processing it so that things don't happen twice.)

Hey, I actually spent some time today trying to get to the bottom of this. This
is what I did:
- when listing props or calculating deltas, add_subdir uses the *target* of the
copy, not the source (this way we don't lose the modifications that were done
after the copy)
- in add_subdir, I eliminate directory entries from the changed_paths hash;
these are then handled normally by add_subdir
- if path_driver_cb_func is handed a path that was deleted from changed_paths,
it ignores it (i.e. returns immediately)

Here's what I've got so far (no log message yet, sorry, and the test still
doesn't pass because of a minor inconsistency in the expected output):

Index: subversion/libsvn_repos/replay.c
===================================================================
--- subversion/libsvn_repos/replay.c (revision 23217)
+++ subversion/libsvn_repos/replay.c (working copy)
@@ -157,6 +157,7 @@
            const char *source_path,
            svn_repos_authz_func_t authz_read_func,
            void *authz_read_baton,
+ apr_hash_t *changed_paths,
            apr_pool_t *pool,
            void **dir_baton)
 {
@@ -168,7 +169,7 @@
   SVN_ERR(editor->add_directory(path, parent_baton, NULL,
                                 SVN_INVALID_REVNUM, pool, dir_baton));

- SVN_ERR(svn_fs_node_proplist(&props, source_root, source_path, pool));
+ SVN_ERR(svn_fs_node_proplist(&props, target_root, path, pool));

   for (phi = apr_hash_first(pool, props); phi; phi = apr_hash_next(phi))
     {
@@ -202,6 +203,13 @@

       new_path = svn_path_join(path, dent->name, subpool);

+ /* If a file or subdirectory of the copied directory is listed as a
+ changed path (because it was modified after the copy but before the
+ commit), we remove it from the changed_paths hash so that future
+ calls to path_driver_cb_func will ignore it. */
+ if (apr_hash_get(changed_paths, new_path, APR_HASH_KEY_STRING))
+ apr_hash_set(changed_paths, new_path, APR_HASH_KEY_STRING, NULL);
+
       if (authz_read_func)
         SVN_ERR(authz_read_func(&readable, target_root, new_path,
                                 authz_read_baton, pool));
@@ -218,7 +226,7 @@
                              svn_path_join(source_path, dent->name,
                                            subpool),
                              authz_read_func, authz_read_baton,
- subpool, &new_dir_baton));
+ changed_paths, subpool, &new_dir_baton));

           SVN_ERR(editor->close_directory(new_dir_baton, subpool));
         }
@@ -227,18 +235,13 @@
           svn_txdelta_window_handler_t delta_handler;
           void *delta_handler_baton, *file_baton;
           svn_txdelta_stream_t *delta_stream;
- const char *new_src_path;
           unsigned char digest[APR_MD5_DIGESTSIZE];

- SVN_ERR(editor->add_file(svn_path_join(path, dent->name, subpool),
- *dir_baton, NULL, SVN_INVALID_REVNUM,
- pool, &file_baton));
+ SVN_ERR(editor->add_file(new_path, *dir_baton, NULL,
+ SVN_INVALID_REVNUM, pool, &file_baton));

- new_src_path = svn_path_join(source_path, dent->name, subpool);
+ SVN_ERR(svn_fs_node_proplist(&props, target_root, new_path, subpool));

- SVN_ERR(svn_fs_node_proplist(&props, source_root, new_src_path,
- subpool));
-
           for (phi = apr_hash_first(pool, props);
                phi;
                phi = apr_hash_next(phi))
@@ -258,7 +261,7 @@
                                           &delta_handler_baton));

           SVN_ERR(svn_fs_get_file_delta_stream
- (&delta_stream, NULL, NULL, source_root, new_src_path,
+ (&delta_stream, NULL, NULL, target_root, new_path,
                    pool));

           SVN_ERR(svn_txdelta_send_txstream(delta_stream,
@@ -267,8 +270,8 @@
                                             pool));

           SVN_ERR(svn_fs_file_md5_checksum(digest,
- source_root,
- new_src_path,
+ target_root,
+ new_path,
                                            pool));
           SVN_ERR(editor->close_file(file_baton,
                                      svn_md5_digest_to_cstring(digest, pool),
@@ -331,6 +334,13 @@
     cb->copies->nelts--;

   change = apr_hash_get(cb->changed_paths, path, APR_HASH_KEY_STRING);
+ if (! change)
+ {
+ /* This can only happen if the path was removed from cb->changed_paths
+ by an earlier call to add_subdir, which means the path was already
+ handled and we should simply ignore it. */
+ return SVN_NO_ERROR;
+ }
   switch (change->change_kind)
     {
     case svn_fs_path_change_add:
@@ -416,7 +426,7 @@
               SVN_ERR(add_subdir(copyfrom_root, root, editor, edit_baton,
                                  path, parent_baton, real_copyfrom_path,
                                  cb->authz_read_func, cb->authz_read_baton,
- pool, dir_baton));
+ cb->changed_paths, pool, dir_baton));
             }
           else
             {

> This feels very dirty, though. Does anyone have an opinion? (It also
> probably wouldn't work for newly added files inside the copied
> directory, I guess, unless you actually throw in a loop over
> changed_paths. Ick.)

In my testing it also works for newly added files. We should of course extend
the test case to cover this scenario.

-- 
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org