[Karl, I've checked that all of your comments on v3 have been addressed or are
repeated here.]
I have a few comments, mostly very nit-picky style and formatting issues, but
I'll start with a bigger issue.
About the naming of the function parameter "no_ignore":
The command-line option has to be "--no-ignore" because its absense has to
imply "ignore". However, the function parameters do not have to match that sense.
Negative option names lead to ugly usage and confusing descriptions. "Ignore"
is already a negative sort of action, though with a sort of positive side to it
as well; "no_ignore" is close to a double negative. Imagine describing
something that sets it to FALSE: "We set no_ignore to false because we don't
want not to exclude certain items." It only takes a few seconds of
concentration to understand it correctly, but what a potential for mistakes!
We already have the verb "ignore" in common usage in Subversion, so it's a bit
late to change that, but definitely get rid of the "no_".
So, call the parameter "do_ignore" or "use_ignore_lists" or similar.
Yes, then there will be a negation required between the command-line option
value and the function parameter, with its own potential for mistakes, but I am
sure that that is the lesser evil.
S.Ramaswamy wrote:
> Index: subversion/include/svn_client.h
> ===================================================================
[...]
@@ -630,11 +630,30 @@
* @a ctx->notify_func2 with @a ctx->notify_baton2 and the path of the
* added item.
*
+ * Use @a no_ignore to indicate that files and directories that match
+ * ignore patterns should be added.
In most doc strings, you should state what the function does, not what the
caller of the function should do. Thus: "If @a no_ignore is true, also add
files and directories that match the global or local ignore patterns."
Or rather, "If @a use_ignore_lists is true, do not add files and directories
that match the global or local ignore patterns."
+ *
* Important: this is a *scheduling* operation. No changes will
* happen to the repository until a commit occurs. This scheduling
* can be removed with svn_client_revert().
+ *
+ * @since New in 1.3.
*/
svn_error_t *
+svn_client_add3 (const char *path,
[...]
> @@ -744,13 +763,25 @@
> * Use @a nonrecursive to indicate that imported directories should not
> * recurse into any subdirectories they may have.
> *
> - * ### kff todo: This import is similar to cvs import, in that it does
> - * not change the source tree into a working copy. However, this
> - * behavior confuses most people, and I think eventually svn _should_
> - * turn the tree into a working copy, or at least should offer the
> - * option. However, doing so is a bit involved, and we don't need it
> - * right now.
You have removed that "to do" comment without mentioning the fact or doing what
it suggests. Presumably you removed it just because you think the comment is
not useful - it can be represented better by an issue in the issue tracker.
Removing it is not really related to the rest of this patch. I think that's
OK, but just wanted to check that this is why you removed it, and it wasn't
that you thought you had done what it suggests.
Oh ... Now I have seen an earlier message where Karl says to you, "Ouch, how
painful to see that "todo" there :-) ." I think Karl was just expressing how
he is sorry that that feature has not yet been implemented. He didn't mean
that you should remove the comment.
> + * Use @a no_ignore to indicate that files and directories that match
> + * ignore patterns should be imported.
Here there was already a part of the doc-string phrased as an instruction to
the user in this style, so copying that style is not so bad, though I'd still
recommend that you use the style I said above.
> + *
> + * @since New in 1.3.
> */
> +svn_error_t *svn_client_import2 (svn_client_commit_info_t **commit_info,
> + const char *path,
> + const char *url,
> + svn_boolean_t nonrecursive,
> + svn_boolean_t no_ignore,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool);
> +
> +/**
> + * Similar to svn_client_import2 except for @a no_ignore set to FALSE
> + * always.
Karl pointed out this different wording last time. Make the wording the same
as it is in other functions - "Similar to svn_client_import2(), but with the @a
no_ignore parameter always set to @c FALSE."
> + *
> + * @deprecated Provided for backward compatibility with the 1.2 API.
> + */
> svn_error_t *svn_client_import (svn_client_commit_info_t **commit_info,
> const char *path,
> const char *url,
> @@ -758,7 +789,6 @@
> svn_client_ctx_t *ctx,
> apr_pool_t *pool);
>
> -
It's not a big deal, but try not to make spurious white space changes. If this
change made the spacing between functions consistent, that would be fine, but
it is completely inconsistent within this file - sometimes one blank line
between functions and sometimes two lines - so adjusting just this one doesn't
help.
> /** @since New in 1.2.
> *
> * Commit files or directories into repository, authenticating with
> Index: subversion/libsvn_client/add.c
> ===================================================================
> --- subversion/libsvn_client/add.c (revision 14888)
> +++ subversion/libsvn_client/add.c (working copy)
> @@ -262,10 +262,23 @@
> return SVN_NO_ERROR;
> }
>
> +/* Schedule directory DIRNAME recursively for addition with access baton
> + * ADM_ACCESS.
> + *
> + * If DIRNAME (or any item below directory DIRNAME) is already scheduled for
> + * addition, add will fail and return an error unless FORCE is TRUE.
Extra space after "add".
> + *
> + * Files and directories that match ignore patterns will not be added unless
> + * NO_IGNORE is TRUE.
> + *
> + * A non-null CTX->CANCEL_FUNC will be called with CTX->CANCEL_BATON if the
> + * user cancels the operation.
> + */
> static svn_error_t *
> add_dir_recursive (const char *dirname,
> svn_wc_adm_access_t *adm_access,
> svn_boolean_t force,
> + svn_boolean_t no_ignore,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> @@ -293,7 +306,8 @@
>
> SVN_ERR (svn_wc_adm_retrieve (&dir_access, adm_access, dirname, pool));
>
> - SVN_ERR (svn_wc_get_default_ignores (&ignores, ctx->config, pool));
> + if (!no_ignore)
(Notice how it is a little uncomfortable to read the double negative "!no_ignore".)
> + SVN_ERR (svn_wc_get_default_ignores (&ignores, ctx->config, pool));
>
> /* Create a subpool for iterative memory control. */
> subpool = svn_pool_create (pool);
> @@ -322,7 +336,8 @@
> || (this_entry.name[1] == '.' && this_entry.name[2] == '\0')))
> continue;
>
> - if (svn_cstring_match_glob_list (this_entry.name, ignores))
> + if ((!no_ignore) && svn_cstring_match_glob_list (this_entry.name,
> + ignores))
> continue;
>
> /* Construct the full path of the entry. */
> @@ -332,7 +347,7 @@
> if (this_entry.filetype == APR_DIR)
> {
> SVN_ERR (add_dir_recursive (fullpath, dir_access, force,
> - ctx, subpool));
> + no_ignore, ctx, subpool));
> }
> else if (this_entry.filetype != APR_UNKFILE)
> {
> @@ -384,6 +399,7 @@
> add (const char *path,
> svn_boolean_t recursive,
> svn_boolean_t force,
> + svn_boolean_t no_ignore,
> svn_wc_adm_access_t *adm_access,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> @@ -393,7 +409,7 @@
>
> SVN_ERR (svn_io_check_path (path, &kind, pool));
> if ((kind == svn_node_dir) && recursive)
> - err = add_dir_recursive (path, adm_access, force, ctx, pool);
> + err = add_dir_recursive (path, adm_access, force, no_ignore, ctx, pool);
> else if (kind == svn_node_file)
> err = add_file (path, ctx, adm_access, pool);
> else
> @@ -410,13 +426,13 @@
> return err;
> }
>
> -
> -
Again, if you were making the spacing consistently one line between functions,
that would be fine, but you're just changing a few and leaving many.
> svn_error_t *
> -svn_client_add (const char *path,
> - svn_boolean_t recursive,
> - svn_client_ctx_t *ctx,
> - apr_pool_t *pool)
> +svn_client_add3 (const char *path,
> + svn_boolean_t recursive,
> + svn_boolean_t force,
> + svn_boolean_t no_ignore,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> {
> svn_error_t *err, *err2;
> svn_wc_adm_access_t *adm_access;
> @@ -426,7 +442,7 @@
> TRUE, 0, ctx->cancel_func, ctx->cancel_baton,
> pool));
>
> - err = add (path, recursive, FALSE, adm_access, ctx, pool);
> + err = add (path, recursive, force, no_ignore, adm_access, ctx, pool);
>
> err2 = svn_wc_adm_close (adm_access);
> if (err2)
> @@ -440,37 +456,35 @@
> return err;
> }
>
> -
(And here.)
> svn_error_t *
> -svn_client_add2 (const char *path,
> +svn_client_add2 (const char *path,
> svn_boolean_t recursive,
> svn_boolean_t force,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
> - svn_error_t *err, *err2;
> - svn_wc_adm_access_t *adm_access;
> - const char *parent_path = svn_path_dirname (path, pool);
> + return svn_client_add3 (path,
> + recursive,
> + force,
> + FALSE,
> + ctx,
> + pool);
This isn't very important, but when calling a function we don't usually put
each parameter on a separate line.
> +}
>
> - SVN_ERR (svn_wc_adm_open3 (&adm_access, NULL, parent_path,
> - TRUE, 0, ctx->cancel_func, ctx->cancel_baton,
> - pool));
> -
> - err = add (path, recursive, force, adm_access, ctx, pool);
> -
> - err2 = svn_wc_adm_close (adm_access);
> - if (err2)
> - {
> - if (err)
> - svn_error_clear (err2);
> - else
> - err = err2;
> - }
> -
> - return err;
> +svn_error_t *
> +svn_client_add (const char *path,
> + svn_boolean_t recursive,
> + svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + return svn_client_add3 (path,
> + recursive,
> + FALSE,
> + FALSE,
> + ctx,
> + pool);
(And here.)
> }
>
> -
(And here.)
> static svn_error_t *
> path_driver_cb_func (void **dir_baton,
> void *parent_baton,
[...]
> Index: subversion/tests/clients/cmdline/import_tests.py
> ===================================================================
[...]
> + if not match:
> + ### we should raise a less generic error here. which?
Ask questions like this on the mailing list, or get rid of the "###" comment if
it has been answered.
> + raise svntest.Failure
[...]
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jun 2 04:38:52 2005