"Rui, Guo" <timmyguo_at_mail.ustc.edu.cn> writes:
> I changed my mind. It should be OK to not check the depth value as other lib
> functions do. Here is the updated svn_wc_add3 patch, the cleanup patch just
> state the same.
>
> [[[
> Introduced a new svn_wc_add3() to handle depth correctly. This makes the
> --depth option in 'svn add' works in the same way as ci, up etc.
"work" :-)
(Your English is very good, by the way. I'm correcting these little
problems just because I'm reviewing anyway. Minor grammar problems
don't interfere with comprehension.)
> * subversion/include/svn_wc.h
> (svn_wc_add3): New, accept depth parameters and ignore it for files.
> (svn_wc_add2): Now deprecated.
"parameter"
Also, you don't have to document the function here -- you're going to do
that in the .h file anyway. So just say:
* subversion/include/svn_wc.h
(svn_wc_add3): New function.
(svn_wc_add2): Deprecate.
Or if you want to emphasize the purpose of this change, you could say:
* subversion/include/svn_wc.h
(svn_wc_add3): New function, taking --depth parameter.
(svn_wc_add2): Deprecate.
But I think even the first way is enough.
> * subversion/libsvn_wc/adm_ops.c
> (svn_wc_add3): New, accept depth parameters and ignore it for files.
> Obsolete comment deleted.
"parameter"
Again, no need to document this here (and also there would be no need to
repeat what you said earlier in the same log message).
It is impossible to delete an obsolete comment from a function that's
new in this commit :-). Seriously: you don't need to mention that in
the log message. It's just a natural part of the change -- anyone
reading the diff would easily understand why that comment went away,
therefore there is no need to make special mention of it in the log
message.
> (svn_wc_add2): Deprecated. Now calls svn_wc_add3 with a default
> svn_depth_infinity.
Again, don't describe the details, just give an overview:
(svn_wc_add2): Just wrap svn_wc_add3.
The purpose of a log message is to give an overview, and to prepare the
reader's mind for the actual diff. The log message should supplement
the diff, not duplicate it.
Therefore, we can combine the two files' log entries like this:
* subversion/include/svn_wc.h, subversion/libsvn_wc/adm_ops.c
(svn_wc_add3): New function.
(svn_wc_add2): Deprecate.
That's all you need to say. The reader will know what it means, from
the introductory part of the log message and from the diff.
> * subversion/libsvn_wc/copy.c
> (copy_added_file_administratively,
> copy_added_dir_administratively,
> copy_dir_administratively): Switch to svn_wc_add3, with a default
> svn_depth_infinity depth.
Try:
* subversion/libsvn_wc/copy.c
(copy_added_file_administratively, copy_added_dir_administratively,
copy_dir_administratively): Call svn_wc_add3 now.
> * subversion/libsvn_client/merge.c
> (merge_dir_added): Switch to svn_wc_add3, with a default svn_depth_infinity
> depth.
Try:
* subversion/libsvn_client/merge.c
(merge_dir_added): Call svn_wc_add3 now.
> * subversion/libsvn_client/copy.c
> (repos_to_wc_copy_single): Switch to svn_wc_add3, with a default
> svn_depth_infinity depth.
...you know what I'm going to say here...
> * subversion/libsvn_client/add.c
> (add_file, add_dir_recursive,
> add, add_parent_dirs): Switch to svn_wc_add3, with a default
> svn_depth_infinity depth.
...and here :-).
> (add): Pass all directory target to add_dir_recursive, otherwise
> adjustment will also be needed here.
"targets", but see below.
Okay, there are several independent problems here. Let's sort them out:
First, the 'add' function is listed twice, once in the "Switch to
svn_wc_add3..." part, and then again right below it. nGenerally,
affected symbol should be listed exactly once in the log message.
Second, I had trouble understanding the comment. I think you meant
something like this:
Unconditionally call add_dir_recursive on directory targets,
regardless of depth parameter, so the target's depth will be set
correctly.
...right? So, here's a new log message for that file, addressing both
of the above problems:
* subversion/libsvn_client/add.c
(add_file, add_dir_recursive, add_parent_dirs): Call svn_wc_add3 now.
(add): Same. Also, call add_dir_recursive on all directory targets,
regardless of depth parameter, so the target's depth will be set
correctly.
But there's still one more problem:
The note about why we invoke add_dir_recursive should really be a
comment in the code, not in the log message! It would be important for
a reader of the code to understand it. The log message can just say:
* subversion/libsvn_client/add.c
(add_file, add_dir_recursive, add_parent_dirs): Call svn_wc_add3 now.
(add): Same, and always call add_dir_recursive for directories.
...then in the code, put a comment explaining *why* it's being called.
> * subversion/tests/cmdline/depth_tests.py
> (add_tree_with_depth_files): Renamed to add_tree_with_depth to reflect the
> new content: depth verified & new tests added for the --force & --parents
> situation
> (test_list): update according to the rename.
In a rename, the new symbol should always appear in the symbol position
in the log message. So:
* subversion/tests/cmdline/depth_tests.py
(add_tree_with_depth): New name for add_tree_with_depth_files.
Verify the depth, and test --force and --parents.
(test_list): Adjust accordingly.
Okay, on to the code...
> --- subversion/include/svn_wc.h (revision 31014)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -2751,6 +2751,9 @@
> *
> * If @a path does not exist, return @c SVN_ERR_WC_PATH_NOT_FOUND.
> *
> + * The @a path is added at @a depth if it is a directory. The @a depth is
> + * ignored for files.
> + *
Good! We generally try to write in the "active voice", that is, the
mode of English in which a subject directly drives each verb. So:
+ * If @a path is a directory, add it at @a depth; otherwise,
+ * ignore @a depth.
> @@ -2794,9 +2797,26 @@
> * ### Update: see "###" comment in svn_wc_add_repos_file()'s doc
> * string about this.
> *
> - * @since New in 1.2.
> + * @since New in 1.5.
> */
> svn_error_t *
> +svn_wc_add3(const char *path,
> + svn_wc_adm_access_t *parent_access,
> + svn_depth_t depth,
> + const char *copyfrom_url,
> + svn_revnum_t copyfrom_rev,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + svn_wc_notify_func2_t notify_func,
> + void *notify_baton,
> + apr_pool_t *pool);
> +
> +/**
> + * Similar to svn_wc_add3(), but takes an @c svn_depth_infinity instead.
> + *
> + * @deprecated Provided for backward compatibility with the 1.2 API.
> + */
> +svn_error_t *
> svn_wc_add2(const char *path,
> svn_wc_adm_access_t *parent_access,
> const char *copyfrom_url,
Good, but it doesn't "take" a depth parameter. (I know what you were
trying to say, of course.) Try this:
+/**
+ * Similar to svn_wc_add3(), but with the @a depth parameter always
+ * @c svn_depth_infinity.
+ *
+ * @deprecated Provided for backward compatibility with the 1.2 API.
+ */
That's how we usually express this situation (see other deprecations in
the .h files for examples).
> --- subversion/libsvn_client/add.c (revision 31014)
> +++ subversion/libsvn_client/add.c (working copy)
> @@ -430,13 +429,13 @@
> svn_error_t *err;
>
> SVN_ERR(svn_io_check_path(path, &kind, pool));
> - if (kind == svn_node_dir && depth >= svn_depth_files)
> + if (kind == svn_node_dir && depth >= svn_depth_empty)
> err = add_dir_recursive(path, adm_access, depth,
> force, no_ignore, ctx, pool);
Yup, this is where the comment would help.
But why are we checking depth at all, if we accept anything from
empty<->infinity? Just check "(kind == svn_node_dir)", in that case. I
mean, if depth is 'svn_depth_unknown' or some other strange value, your
code would just skip the call to add_dir_recursive() entirely. Surely
that's no better (nor worse) than calling add_dir_recursive() with a bad
depth :-) ?
Other than that stuff, the patch looks very good. I applied it, made
tweaks as outlined above, and ran 'make check'. These tests failed:
switch_tests.py 26: switch to dir_at_peg where dir doesn't exist in HEAD
switch_tests.py 28: switch to old rev of now renamed branch
log_tests.py 16: test 'svn log -g' on a single revision
log_tests.py 19: test 'svn log -g' a path added before merge
merge_tests.py 19: merge should skip over unversioned obstructions
blame_tests.py 10: test 'svn blame -g'
blame_tests.py 11: don't look for merged files out of range
So I reverted the patch and tried with a clean trunk_at_r31039. This time,
no failures. Could you look into this, please? Here's the adjusted
version of the patch that I used. It should be functionally the same as
yours, I only tweaked comments and formatting.
[[[
Introduce svn_wc_add3(), which takes a depth parameter. This makes
'svn add --depth' work the same way as commit, update, etc.
Patch by: Guo Rui <timmyguo_at_mail.ustc.edu.cn>
(Minor tweaks by me.)
* subversion/include/svn_wc.h, subversion/libsvn_wc/adm_ops.c
(svn_wc_add3): New function.
(svn_wc_add2): Deprecate.
* subversion/libsvn_wc/copy.c
(copy_added_file_administratively, copy_added_dir_administratively,
copy_dir_administratively): Call svn_wc_add3 now.
* subversion/libsvn_client/merge.c
(merge_dir_added): Call svn_wc_add3 now.
* subversion/libsvn_client/copy.c
(repos_to_wc_copy_single): Call svn_wc_add3 now.
* subversion/libsvn_client/add.c
(add_file, add_dir_recursive, add_parent_dirs): Call svn_wc_add3 now.
(add): Same, and always call add_dir_recursive for directories.
Add braces to conditional chain, for readability.
* subversion/tests/cmdline/depth_tests.py
(add_tree_with_depth): New name for add_tree_with_depth_files.
Verify the depth, and test --force and --parents.
(test_list): Adjust accordingly.
]]]
Index: subversion/include/svn_wc.h
===================================================================
--- subversion/include/svn_wc.h (revision 31039)
+++ subversion/include/svn_wc.h (working copy)
@@ -2756,6 +2756,9 @@
*
* If @a path does not exist, return @c SVN_ERR_WC_PATH_NOT_FOUND.
*
+ * If @a path is a directory, add it at @a depth; otherwise, ignore
+ * @a depth.
+ *
* If @a copyfrom_url is non-NULL, it and @a copyfrom_rev are used as
* `copyfrom' args. This is for copy operations, where one wants
* to schedule @a path for addition with a particular history.
@@ -2799,9 +2802,27 @@
* ### Update: see "###" comment in svn_wc_add_repos_file()'s doc
* string about this.
*
- * @since New in 1.2.
+ * @since New in 1.5.
*/
svn_error_t *
+svn_wc_add3(const char *path,
+ svn_wc_adm_access_t *parent_access,
+ svn_depth_t depth,
+ const char *copyfrom_url,
+ svn_revnum_t copyfrom_rev,
+ svn_cancel_func_t cancel_func,
+ void *cancel_baton,
+ svn_wc_notify_func2_t notify_func,
+ void *notify_baton,
+ apr_pool_t *pool);
+
+/**
+ * Similar to svn_wc_add3(), but with the @a depth parameter always
+ * @c svn_depth_infinity.
+ *
+ * @deprecated Provided for backward compatibility with the 1.2 API.
+ */
+svn_error_t *
svn_wc_add2(const char *path,
svn_wc_adm_access_t *parent_access,
const char *copyfrom_url,
Index: subversion/libsvn_wc/copy.c
===================================================================
--- subversion/libsvn_wc/copy.c (revision 31039)
+++ subversion/libsvn_wc/copy.c (working copy)
@@ -75,8 +75,8 @@
if (src_is_added)
{
- SVN_ERR(svn_wc_add2(dst_path, dst_parent_access, NULL,
- SVN_INVALID_REVNUM, cancel_func,
+ SVN_ERR(svn_wc_add3(dst_path, dst_parent_access, svn_depth_infinity,
+ NULL, SVN_INVALID_REVNUM, cancel_func,
cancel_baton, notify_func,
notify_baton, pool));
}
@@ -150,7 +150,7 @@
/* Add the directory, adding locking access for dst_path
to dst_parent_access at the same time. */
- SVN_ERR(svn_wc_add2(dst_path, dst_parent_access, NULL,
+ SVN_ERR(svn_wc_add3(dst_path, dst_parent_access, svn_depth_infinity, NULL,
SVN_INVALID_REVNUM, cancel_func, cancel_baton,
notify_func, notify_baton, pool));
@@ -755,7 +755,7 @@
SVN_ERR(svn_wc_adm_close(adm_access));
- SVN_ERR(svn_wc_add2(dst_path, dst_parent,
+ SVN_ERR(svn_wc_add3(dst_path, dst_parent, svn_depth_infinity,
copyfrom_url, copyfrom_rev,
cancel_func, cancel_baton,
notify_copied, notify_baton, pool));
Index: subversion/libsvn_wc/adm_ops.c
===================================================================
--- subversion/libsvn_wc/adm_ops.c (revision 31039)
+++ subversion/libsvn_wc/adm_ops.c (working copy)
@@ -1377,8 +1377,9 @@
svn_error_t *
-svn_wc_add2(const char *path,
+svn_wc_add3(const char *path,
svn_wc_adm_access_t *parent_access,
+ svn_depth_t depth,
const char *copyfrom_url,
svn_revnum_t copyfrom_rev,
svn_cancel_func_t cancel_func,
@@ -1526,13 +1527,6 @@
if (kind == svn_node_dir) /* scheduling a directory for addition */
{
- /* Note that both calls to svn_wc_ensure_adm3() below pass
- svn_depth_infinity. Even if 'svn add' were invoked with some
- other depth, we'd want to create the adm area with
- svn_depth_infinity, because when the user passes add a depth,
- that's just a way of telling Subversion what items to add,
- not a way of telling Subversion what depth the resultant
- newly-versioned directory should have. */
if (! copyfrom_url)
{
@@ -1550,7 +1544,7 @@
/* Make sure this new directory has an admistrative subdirectory
created inside of it */
SVN_ERR(svn_wc_ensure_adm3(path, NULL, new_url, p_entry->repos,
- 0, svn_depth_infinity, pool));
+ 0, depth, pool));
}
else
{
@@ -1560,7 +1554,7 @@
copyfrom arguments to the ensure call. */
SVN_ERR(svn_wc_ensure_adm3(path, NULL, copyfrom_url,
parent_entry->repos, copyfrom_rev,
- svn_depth_infinity, pool));
+ depth, pool));
}
/* We want the locks to persist, so use the access baton's pool */
@@ -1604,7 +1598,7 @@
/* Change the entry urls recursively (but not the working rev). */
SVN_ERR(svn_wc__do_update_cleanup(path, adm_access,
- svn_depth_infinity, new_url,
+ depth, new_url,
parent_entry->repos,
SVN_INVALID_REVNUM, NULL,
NULL, FALSE, apr_hash_make(pool),
@@ -1636,6 +1630,22 @@
return SVN_NO_ERROR;
}
+svn_error_t *
+svn_wc_add2(const char *path,
+ svn_wc_adm_access_t *parent_access,
+ const char *copyfrom_url,
+ svn_revnum_t copyfrom_rev,
+ svn_cancel_func_t cancel_func,
+ void *cancel_baton,
+ svn_wc_notify_func2_t notify_func,
+ void *notify_baton,
+ apr_pool_t *pool)
+{
+ return svn_wc_add3(path, parent_access, svn_depth_infinity,
+ copyfrom_url, copyfrom_rev,
+ cancel_func, cancel_baton,
+ notify_func, notify_baton, pool);
+}
svn_error_t *
svn_wc_add(const char *path,
@@ -1658,7 +1668,6 @@
svn_wc__compat_call_notify_func, &nb, pool);
}
-
/* Thoughts on Reversion.
Index: subversion/libsvn_client/merge.c
===================================================================
--- subversion/libsvn_client/merge.c (revision 31039)
+++ subversion/libsvn_client/merge.c (working copy)
@@ -1100,7 +1100,7 @@
else
{
SVN_ERR(svn_io_make_dir_recursively(path, subpool));
- SVN_ERR(svn_wc_add2(path, adm_access,
+ SVN_ERR(svn_wc_add3(path, adm_access, svn_depth_infinity,
copyfrom_url, copyfrom_rev,
merge_b->ctx->cancel_func,
merge_b->ctx->cancel_baton,
@@ -1117,7 +1117,7 @@
if (! entry || entry->schedule == svn_wc_schedule_delete)
{
if (!merge_b->dry_run)
- SVN_ERR(svn_wc_add2(path, adm_access,
+ SVN_ERR(svn_wc_add3(path, adm_access, svn_depth_infinity,
copyfrom_url, copyfrom_rev,
merge_b->ctx->cancel_func,
merge_b->ctx->cancel_baton,
Index: subversion/libsvn_client/copy.c
===================================================================
--- subversion/libsvn_client/copy.c (revision 31039)
+++ subversion/libsvn_client/copy.c (working copy)
@@ -1389,8 +1389,8 @@
/* Schedule dst_path for addition in parent, with copy history.
(This function also recursively puts a 'copied' flag on every
entry). */
- SVN_ERR(svn_wc_add2(pair->dst, adm_access, pair->src,
- src_revnum,
+ SVN_ERR(svn_wc_add3(pair->dst, adm_access, svn_depth_infinity,
+ pair->src, src_revnum,
ctx->cancel_func, ctx->cancel_baton,
ctx->notify_func2, ctx->notify_baton2, pool));
Index: subversion/libsvn_client/add.c
===================================================================
--- subversion/libsvn_client/add.c (revision 31039)
+++ subversion/libsvn_client/add.c (working copy)
@@ -232,8 +232,8 @@
pool));
/* Add the file */
- SVN_ERR(svn_wc_add2(path, adm_access, NULL, SVN_INVALID_REVNUM,
- ctx->cancel_func, ctx->cancel_baton,
+ SVN_ERR(svn_wc_add3(path, adm_access, svn_depth_infinity, NULL,
+ SVN_INVALID_REVNUM, ctx->cancel_func, ctx->cancel_baton,
NULL, NULL, pool));
if (is_special)
@@ -308,9 +308,8 @@
SVN_ERR(ctx->cancel_func(ctx->cancel_baton));
/* Add this directory to revision control. */
- err = svn_wc_add2(dirname, adm_access,
- NULL, SVN_INVALID_REVNUM,
- ctx->cancel_func, ctx->cancel_baton,
+ err = svn_wc_add3(dirname, adm_access, depth, NULL, SVN_INVALID_REVNUM,
+ ctx->cancel_func, ctx->cancel_baton,
ctx->notify_func2, ctx->notify_baton2, pool);
if (err && err->apr_err == SVN_ERR_ENTRY_EXISTS && force)
svn_error_clear(err);
@@ -430,15 +429,25 @@
svn_error_t *err;
SVN_ERR(svn_io_check_path(path, &kind, pool));
- if (kind == svn_node_dir && depth >= svn_depth_files)
- err = add_dir_recursive(path, adm_access, depth,
- force, no_ignore, ctx, pool);
+ if (kind == svn_node_dir)
+ {
+ /* We use add_dir_recursive for all directory targets,
+ and pass depth along no matter what it is, so that the
+ target's depth will be set correctly. */
+ err = add_dir_recursive(path, adm_access, depth,
+ force, no_ignore, ctx, pool);
+
+ }
else if (kind == svn_node_file)
- err = add_file(path, ctx, adm_access, pool);
+ {
+ err = add_file(path, ctx, adm_access, pool);
+ }
else
- err = svn_wc_add2(path, adm_access, NULL, SVN_INVALID_REVNUM,
- ctx->cancel_func, ctx->cancel_baton,
- ctx->notify_func2, ctx->notify_baton2, pool);
+ {
+ err = svn_wc_add3(path, adm_access, depth, NULL, SVN_INVALID_REVNUM,
+ ctx->cancel_func, ctx->cancel_baton,
+ ctx->notify_func2, ctx->notify_baton2, pool);
+ }
/* Ignore SVN_ERR_ENTRY_EXISTS when FORCE is set. */
if (err && err->apr_err == SVN_ERR_ENTRY_EXISTS && force)
@@ -482,7 +491,8 @@
SVN_ERR(add_parent_dirs(parent_path, &adm_access, ctx, pool));
SVN_ERR(svn_wc_adm_retrieve(&adm_access, adm_access, parent_path,
pool));
- SVN_ERR(svn_wc_add2(path, adm_access, NULL, SVN_INVALID_REVNUM,
+ SVN_ERR(svn_wc_add3(path, adm_access, svn_depth_infinity,
+ NULL, SVN_INVALID_REVNUM,
ctx->cancel_func, ctx->cancel_baton,
ctx->notify_func2, ctx->notify_baton2, pool));
}
Index: subversion/tests/cmdline/depth_tests.py
===================================================================
--- subversion/tests/cmdline/depth_tests.py (revision 31039)
+++ subversion/tests/cmdline/depth_tests.py (working copy)
@@ -1210,17 +1210,38 @@
# Check that the new directory was added at depth=empty.
verify_depth(None, "empty", other_I_path)
-def add_tree_with_depth_files(sbox):
- "add multi-subdir tree with --depth=files" # For issue #2931
+def add_tree_with_depth(sbox):
+ "add multi-subdir tree with --depth options" # For issue #2931
sbox.build()
wc_dir = sbox.wc_dir
new1_path = os.path.join(wc_dir, 'new1')
new2_path = os.path.join(new1_path, 'new2')
+ new3_path = os.path.join(new2_path, 'new3')
+ new4_path = os.path.join(new3_path, 'new4')
os.mkdir(new1_path)
os.mkdir(new2_path)
+ os.mkdir(new3_path)
+ os.mkdir(new4_path)
+ # Simple case, add new1 only, set depth to files
svntest.actions.run_and_verify_svn(None, None, [],
"add", "--depth", "files", new1_path)
+ verify_depth(None, "files", new1_path)
+ # Force add new1 at new1 again, should include new2 at empty, the depth of
+ # new1 should not change
+ svntest.actions.run_and_verify_svn(None, None, [],
+ "add", "--depth", "immediates",
+ "--force", new1_path)
+ verify_depth(None, "files", new1_path)
+ verify_depth(None, "empty", new2_path)
+
+ # add new4 with intermediate path, the intermediate path is added at empty
+ svntest.actions.run_and_verify_svn(None, None, [],
+ "add", "--depth", "immediates",
+ "--parents", new4_path)
+ verify_depth(None, "infinity", new3_path)
+ verify_depth(None, "immediates", new4_path)
+
def upgrade_from_above(sbox):
"upgrade a depth=empty wc from above"
@@ -2016,7 +2037,7 @@
diff_in_depthy_wc,
commit_depth_immediates,
depth_immediates_receive_new_dir,
- add_tree_with_depth_files,
+ add_tree_with_depth,
upgrade_from_above,
status_in_depthy_wc,
depthy_update_above_dir_to_be_deleted,
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-05-06 08:07:03 CEST