svn commit: r1341060 - /subversion/trunk/subversion/libsvn_client/commit.c

[st...@apache.org]

Author: stsp
Date: Mon May 21 14:58:18 2012
New Revision: 1341060

URL: http://svn.apache.org/viewvc?rev=1341060&view=rev
Log:
Make 'svn import' sort directory entries before processing them and notifying
for each path. Avoids random output order with APR-1.4.6.

* subversion/libsvn_client/commit.c
  (import_children): Use a sorted array of hash keys instead of iterating over
   the dirents hash directly.

Modified:
    subversion/trunk/subversion/libsvn_client/commit.c

Modified: subversion/trunk/subversion/libsvn_client/commit.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/commit.c?rev=1341060&r1=1341059&r2=1341060&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/commit.c (original)
+++ subversion/trunk/subversion/libsvn_client/commit.c Mon May 21 14:58:18 2012
@@ -402,14 +402,19 @@ import_children(const char *dir_abspath,
                 svn_client_ctx_t *ctx,
                 apr_pool_t *scratch_pool)
 {
-  apr_hash_index_t *hi;
+  apr_array_header_t *sorted_dirents;
+  int i;
   apr_pool_t *iterpool = svn_pool_create(scratch_pool);
 
-  for (hi = apr_hash_first(scratch_pool, dirents); hi; hi = apr_hash_next(hi))
+  sorted_dirents = svn_sort__hash(dirents, svn_sort_compare_items_as_paths,
+                                   scratch_pool);
+  for (i = 0; i < sorted_dirents->nelts; i++)
     {
       const char *this_abspath, *this_edit_path;
-      const char *filename = svn__apr_hash_index_key(hi);
-      const svn_io_dirent2_t *dirent = svn__apr_hash_index_val(hi);
+      svn_sort__item_t item = APR_ARRAY_IDX(sorted_dirents, i,
+                                            svn_sort__item_t);
+      const char *filename = item.key;
+      const svn_io_dirent2_t *dirent = item.value;
 
       svn_pool_clear(iterpool);
 

Re: r1341060 - Sort output of 'svn import'

[Julian Foad <ju...@btopenworld.com>]

> Author: stsp

> New Revision: 1341060
> 
> URL: http://svn.apache.org/viewvc?rev=1341060&view=rev
> Log:
> Make 'svn import' sort directory entries before processing them and 
> notifying
> for each path. Avoids random output order with APR-1.4.6.

I agree it avoids random output order, but I want to point out that this is not specific to APR 1.4.6.

The ordering wasn't stable beforethat (with the old APR hashing), because the order the directory entries were read into thehash, and so the order of multiple keys in one bucket, was notcontrolled.

It took me a while to demonstrate that to myself, as (1) I had to test with a sufficient number of directory entries get several in one hash bucket, and (2) my file system was always returning directory entries in the same order (no matter what order I created them) until I ran the same test on two different file systems (a hard disk and a RAM disk).

Running this test with Subversion 1.7.5 and APR 1.4.5 ...

$ mkdir -p /harddisk/D/D8/{4,3,2,1}{4,3,2,1}{4,3,2,1}{4,3,2,1}{4,3,2,1}{4,3,2,1}
$ mkdir -p /ramdisk/D/D9/{4,3,2,1}{4,3,2,1}{4,3,2,1}{4,3,2,1}{4,3,2,1}{4,3,2,1}
$ svn import /harddisk/D file:///`pwd`/repo -m "" > D8.out
$ svn import /ramdisk/D file:///`pwd`/repo -m "" > D9.out
$ vimdiff D8.out D9.out

... shows a difference in ordering.

There is a backport nomination for this revision in branches/1.7.x/STATUS:

 * r1341060
   Sort output of 'svn import'
   Notes: Needs a backport branch, noted here for completeness.
   Votes:
     +0: stsp (pending a proper conflict-free backport)
 
to which I have just added a '-0' vote, on the basis that it's an enhancement rather than a fix, and mentioning the above.

I have no objection in principle to backporting it as an enhancement, but in that case should we not do the same for 'svn add', 'svn export', etc.?

Also, I see on trunk there is a patch for 'svnadmin dump' ordering, for issue #4134 <http://subversion.tigris.org/issues/show_bug.cgi?id=4134>, that we could back-port if/when it's complete (which isn't clear from reading the issue).

- Julian


> * subversion/libsvn_client/commit.c
>   (import_children): Use a sorted array of hash keys instead of iterating over
>    the dirents hash directly.
> 
> Modified:
>     subversion/trunk/subversion/libsvn_client/commit.c
> 
> Modified: subversion/trunk/subversion/libsvn_client/commit.c
> URL: 
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/commit.c?rev=1341060&r1=1341059&r2=1341060&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/libsvn_client/commit.c (original)
> +++ subversion/trunk/subversion/libsvn_client/commit.c Mon May 21 14:58:18 2012
> @@ -402,14 +402,19 @@ import_children(const char *dir_abspath,
>                  svn_client_ctx_t *ctx,
>                  apr_pool_t *scratch_pool)
> {
> -  apr_hash_index_t *hi;
> +  apr_array_header_t *sorted_dirents;
> +  int i;
>    apr_pool_t *iterpool = svn_pool_create(scratch_pool);
> 
> -  for (hi = apr_hash_first(scratch_pool, dirents); hi; hi = apr_hash_next(hi))
> +  sorted_dirents = svn_sort__hash(dirents, svn_sort_compare_items_as_paths,
> +                                   scratch_pool);
> +  for (i = 0; i < sorted_dirents->nelts; i++)
>      {
>        const char *this_abspath, *this_edit_path;
> -      const char *filename = svn__apr_hash_index_key(hi);
> -      const svn_io_dirent2_t *dirent = svn__apr_hash_index_val(hi);
> +      svn_sort__item_t item = APR_ARRAY_IDX(sorted_dirents, i,
> +                                            svn_sort__item_t);
> +      const char *filename = item.key;
> +      const svn_io_dirent2_t *dirent = item.value;
> 
>        svn_pool_clear(iterpool);
>