On Sat, Nov 19, 2005 at 01:27:52PM -0800, Kean Johnston wrote:
> Attached is my first patch for Subversion, so go easy with the
> flames :) This is to change the order in which files are shown
> by and 'svn diff', such that all files in a directory are shown
> first, alphabetically, then all directories, also alphabetically.
I like the idea of making the output from 'svn diff' deterministic, and
I'm personally not too bothered about the precise order, or whether it
matches the ordering of other svn subcommands (though it might be nice
if it did).
I am curious about the ordering you picked: was there any
(performance? usability?) reason for it? I guess if I had to come up
with an ordering for diff myself, I'd just have picked 'sort by name',
not 'files first, directories second, sorted by name'.
For repos->wc diffs with local modifications, we'll actually report the
repos->wc diffs followed by the wc->wc diffs, with each group sorted,
won't we? (instead of sorting the whole output, which would be hard).
I've not really reviewed the patch itself, since I'm not familiar enough
with libsvn_repos, but it looks generally good to me.
> Index: subversion/libsvn_repos/delta.c
> ===================================================================
> --- subversion/libsvn_repos/delta.c (revision 17442)
> +++ subversion/libsvn_repos/delta.c (working copy)
> void *val;
> @@ -970,8 +978,11 @@
> svn_pool_clear (subpool);
>
> /* KEY is the entry name in target, VAL the dirent */
> - apr_hash_this (hi, &key, &klen, &val);
> - t_entry = val;
> + val = item->value;
> + t_entry = item->value;
> + key = item->key;
> + klen = item->klen;
> +
I think you can probably delete the 'val' variable entirely: it's not
used anywhere else in this function.
> Index: subversion/libsvn_repos/repos.h
> ===================================================================
> --- subversion/libsvn_repos/repos.h (revision 17442)
> +++ subversion/libsvn_repos/repos.h (working copy)
> @@ -240,6 +241,14 @@
> apr_pool_t *pool);
>
>
> +/* Sort an svn_fs_direct_t hash first by file type (such that nodes of type
> + svn_node_file appear before svn_node_dir nodes), and then alphabetically
> + as defined by svn_path_compare_paths.
> +
> + This function is intended as a callback for svn_sort__hash(). */
> +int
> +svn_repos_sort_by_type_and_path (const svn_sort__item_t *item1,
> + const svn_sort__item_t *item2);
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
That should probably be svn_repos__sort_by_type_and_path(). (And isn't it
'type and name'?)
> Index: subversion/libsvn_wc/diff.c
> ===================================================================
> --- subversion/libsvn_wc/diff.c (revision 17442)
> +++ subversion/libsvn_wc/diff.c (working copy)
> @@ -691,9 +725,12 @@
>
> subpool = svn_pool_create (dir_baton->pool);
>
> - for (hi = apr_hash_first (dir_baton->pool, entries); hi;
> - hi = apr_hash_next (hi))
> + sorted = svn_sort__hash (entries, sort_by_type_and_path, dir_baton->pool);
> +
> + for (i = 0; i < sorted->nelts; ++i)
> {
> + const svn_sort__item_t *item = &APR_ARRAY_IDX(sorted, i,
> + const svn_sort__item_t);
> const void *key;
> void *val;
> const svn_wc_entry_t *entry;
Again, 'val' isn't used after your patch, so you could remove it.
And 'key' is also unused (except for comparing against
SVN_WC_ENTRY_THIS_DIR, below, where we could easily use 'name'), so I'd
get rid of that as well.
> @@ -702,9 +739,9 @@
>
> svn_pool_clear (subpool);
>
> - apr_hash_this (hi, &key, NULL, &val);
> - name = key;
> - entry = val;
> + name = item->key;
> + key = item->key;
> + entry = item->value;
>
> /* Skip entry for the directory itself. */
> if (strcmp (key, SVN_WC_ENTRY_THIS_DIR) == 0)
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Nov 20 21:34:45 2005