"S.Ramaswamy" <ramaswamy@collab.net> writes:
> Fix issue #2105. Add "--no-ignore" switch to 'svn import'.
And 'svn add', right?
> * subversion/include/svn_client.h
> (svn_client_import) : Marked function as deprecated.
> (svn_client_import2) : Added prototype for new function and doc
> string.
> (svn_client_add2) : Marked function as deprecated.
> (svn_client_add3) : Added prototype for new function and doc
> string.
Time for some log message surgery :-).
It's easier and clearer to just say "New prototype". No need to say
there's a doc string -- that's automatically part of adding a new
prototype. (The phrasing was ambiguous and required careful reading
anyway, since the grouping could have gone either way grammatically.)
By the way, don't put those spaces before the colons. Some people's
colorizers depend on matching "(blah): ".
> * subversion/libsvn_client/commit.c
> (svn_client_import) : Re-implemented in terms of new function,
> 'svn_client_import2()'.
> (svn_client_import2) : New function. Same as the older function
> 'svn_client_import()' except for the additional boolean
> parameter 'no_ignore'. Setting 'no_ignore' to TRUE results
> in files and directories that match ignore patterns to be
> imported.
> (import_dir) : Added parameter 'no_ignore' to function.
> Calling 'import_dir()' with 'no_ignore' set to TRUE results
> in files directories that match ignore patterns to be imported.
> (import) : Added parameter 'no_ignore' to function.
> Calling 'import()' with 'no_ignore' set to TRUE results
> in files and directories that match ignore patterns to be
> imported.
In this case, the meaning of the new 'no_ignore' parameter is clear,
so there's no need to say (three times :-) ) what you say in their doc
strings anyway.
The change to import_dir() and import() is the same, so they can be
combined into one entry: "(import_dir, import): ...".
> * subversion/libsvn_client/add.c
> (svn_client_add) : Modified to call 'add()' with 'no_ignore'
> always set to FALSE.
> (svn_client_add2) : Re-implemented in terms of new function
> 'svn_client_add3()'.
> (svn_client_add3) : New function. Same as the older function
> 'svn_client_add2()' except for the additional boolean
> parameter 'no_ignore'. Setting 'no_ignore' to TRUE results
> in files and directories that match default ignore patterns
> to be be added during recursive directory adds.
> (add) : Added boolean parameter 'no_ignore' to function and pass
> this additional parameter to 'add_dir_recursive()'
> (add_dir_recursive) : Added boolean parameter 'no_ignore' to
> function. Files and directories that match default ignore
> patterns are ignored if no_ignore is TRUE.
All this duplicated information makes the log message larger and
harder to understand. Just assume the reader knows generally what
you're trying to do.
Try to put consequent changes after their dependencies. For example,
If you list add_dir_recursive() first, and then add(), and *then* the
others, then the reader gets the information in the order that makes
the most sense.
> * subversion/clients/cmdline/import-cmd.c
> (svn_cl__import) : Modified to call the new function
> 'svn_client_import2()'.
(Now I will take my fight against needless words to new extremes...)
Every change is a "modification", so no need to say that. If you're
writing about it in a log message, it's a modification. Just say what
you did, e.g.:
* subversion/clients/cmdline/import-cmd.c
(svn_cl__import): Call svn_client_import2().
The quotes are optional, but if you use them, there's probably no need
to do the "()" thing at the end.
> * subversion/clients/cmdline/main.c
> (svn_cl__cmd_table) : Added '--no-ignore' command line option to
> 'svn import' and 'svn add'.
>
> * subversion/tests/clients/cmdline/import_tests.py
> (test_list) : Added new test 'import_no_ignores()' to list of
> import tests.
> (import_no_ignores) : New test. Verifies that files which match
> ignore patterns can be imported with the '--no-ignore' option.
>
> * subversion/tests/clients/cmdline/basic_tests.py
> (test_list) : Added new test 'basic_add_no_ignores()' to list
> of basic tests.
> (basic_add_no_ignores) : New test. Verfies that files which
> match ignore patterns can be added with the '--no-ignore'
> option.
You don't need to say what the tests do. Obviously they're testing
the new functionality; the details are in the code and comments of the
tests themselves. Thus, a more compact idiom for this is:
* subversion/tests/clients/cmdline/basic_tests.py
(basic_add_no_ignores): New test.
(test_list): Run it.
Okay, that's a lot of comments about the log message. Let me show you
how much shorter and more digestible it is if you do all these things:
Fix issue #2105. Add "--no-ignore" switch to 'svn import' and 'svn add'.
* subversion/include/svn_client.h
(svn_client_import2): New prototype.
(svn_client_import): Deprecate.
(svn_client_add3): New prototype.
(svn_client_add2): Deprecate.
* subversion/libsvn_client/commit.c
(svn_client_import2): New function. Like svn_client_import() but
takes additional boolean parameter 'no_ignore'.
(svn_client_import): Re-implement using svn_client_import2().
(import_dir, import): Take new 'no_ignore' parameter.
* subversion/libsvn_client/add.c
(add_dir_recursive): Take new boolean 'no_ignore' parameter.
(add): Take new boolean 'no_ignore' parameter, pass it along to
add_dir_recursive().
(svn_client_add3): New function. Like svn_client_add2() but takes
additional boolean parameter 'no_ignore'.
(svn_client_add2): Re-implement using svn_client_add3().
(svn_client_add): Pass FALSE to add().
* subversion/clients/cmdline/import-cmd.c
(svn_cl__import): Call svn_client_import2().
* subversion/clients/cmdline/main.c
(svn_cl__cmd_table): New '--no-ignore' option for import and add.
* subversion/tests/clients/cmdline/import_tests.py
(import_no_ignores): New test.
(test_list): Run it.
* subversion/tests/clients/cmdline/basic_tests.py
(test_list): New test.
(basic_add_no_ignores): Run it.
Okay, on to the code changes:
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 14823)
> +++ subversion/include/svn_client.h (working copy)
> @@ -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.
> + *
> * 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.
> */
There's some inconsistency in our code, but generally the @since line
goes at the top. Might be nice if we cleaned up the rest of that file
to be consistent (in a separate commit of course).
> svn_error_t *
> +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);
> +
> +/**
> + * Similar to svn_client_add3(), 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_add2 (const char *path,
> svn_boolean_t recursive,
> svn_boolean_t force,
> @@ -744,13 +763,32 @@
> * Use @a nonrecursive to indicate that imported directories should not
> * recurse into any subdirectories they may have.
> *
> + * Use @a no_ignore to indicate that files and directories that match
> + * ignore patterns should be imported.
> + *
> * ### 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.
> + *
> + * @since New in 1.3.
> */
Ouch, how painful to see that "todo" there :-).
> +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.
> + *
> + * @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,
Is there a reason to use slightly different phrasing here?
> Index: subversion/libsvn_client/add.c
> ===================================================================
> --- subversion/libsvn_client/add.c (revision 14823)
> +++ subversion/libsvn_client/add.c (working copy)
> @@ -266,6 +266,7 @@
> 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)
> {
How sad that there's no doc string on that function. Consider adding
one (although it's not necessary for this change, as it was missing
prior to your change).
> @@ -426,7 +430,7 @@
> TRUE, 0, ctx->cancel_func, ctx->cancel_baton,
> pool));
>
> - err = add (path, recursive, FALSE, adm_access, ctx, pool);
> + err = add (path, recursive, FALSE, FALSE, adm_access, ctx, pool);
>
> err2 = svn_wc_adm_close (adm_access);
> if (err2)
Again, this weirdness predates your change, but:
It's odd that svn_client_add() is currently a wrapper around add()
instead of around svn_client_add2(), and that your change leaves it as
a wrapper around add() instead of making it be a wrapper around
svn_client_add3().
Meanwhile, in your change, svn_client_add2() becomes a wrapper around
svn_client_add3(), which is proper. I just think the same thing
should happen to svn_client_add().
By the way, there's a call to svn_client_add() on line 636 of
libsvn_client/add.c that also needs to be updated to
svn_client_add3(). (I found this by building a tags table and doing a
tags-search on "svn_client_add" throughout the code base.)
> --- subversion/libsvn_client/commit.c (revision 14823)
> +++ subversion/libsvn_client/commit.c (working copy)
> @@ -297,6 +297,7 @@
> const char *edit_path,
> svn_boolean_t nonrecursive,
> apr_hash_t *excludes,
> + svn_boolean_t no_ignore,
> import_ctx_t *import_ctx,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
import_dir() has a doc string, so you should update it when you add a
parameter.
> @@ -443,7 +446,7 @@
> *
> * EXCLUDES is a hash whose keys are absolute paths to exclude from
> * the import (values are unused).
> - *
> + *
> * Use POOL for any temporary allocation.
> *
> * Note: the repository directory receiving the import was specified
?? :-)
> @@ -458,6 +461,7 @@
> void *edit_baton,
> svn_boolean_t nonrecursive,
> apr_hash_t *excludes,
> + svn_boolean_t no_ignore,
> svn_client_ctx_t *ctx,
> apr_pool_t *pool)
> {
Likewise, import() has a doc string, please update it.
> @@ -522,15 +526,22 @@
>
> if (kind == svn_node_file)
> {
> - SVN_ERR (svn_wc_get_default_ignores (&ignores, ctx->config, pool));
> - if (! svn_cstring_match_glob_list (path, ignores))
> + svn_boolean_t ignores_match;
> +
> + if (!no_ignore)
> + {
> + SVN_ERR (svn_wc_get_default_ignores (&ignores, ctx->config, pool));
> + ignores_match = svn_cstring_match_glob_list (path, ignores);
> + }
> + if ((no_ignore) || (!ignores_match))
> SVN_ERR (import_file (editor, root_baton, path, edit_path,
> import_ctx, ctx, pool));
> }
If you just initialize ignores_match to FALSE when you declare it, you
can just do "if (!ignores_match)" for the second conditional.
> Index: subversion/tests/clients/cmdline/import_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/import_tests.py (revision 14823)
> +++ subversion/tests/clients/cmdline/import_tests.py (working copy)
> @@ -189,6 +189,83 @@
> None, None, 1)
>
> #----------------------------------------------------------------------
> +def import_no_ignores(sbox):
> + 'import ignored files in imported dirs'
> +
> + # import ignored files using the "--no-ignore" option
> +
> + sbox.build()
> + wc_dir = sbox.wc_dir
> +
> + dir_path = os.path.join(wc_dir, 'dir')
> + foo_c_path = os.path.join(dir_path, 'foo.c')
> + foo_o_path = os.path.join(dir_path, 'foo.o')
> + foo_lo_path = os.path.join(dir_path, 'foo.lo')
> + foo_rej_path = os.path.join(dir_path, 'foo.rej')
> +
> + os.mkdir(dir_path, 0755)
> + open(foo_c_path, 'w')
> + open(foo_o_path, 'w')
> + open(foo_lo_path, 'w')
> + open(foo_rej_path, 'w')
> +
> + # import new dir into repository
> + url = svntest.main.current_repo_url + '/dir'
> +
> + output, errput = svntest.actions.run_and_verify_svn(
> + None, None, [], 'import',
> + '--username', svntest.main.wc_author,
> + '--password', svntest.main.wc_passwd,
> + '-m', 'Log message for new import', '--no-ignore',
> + dir_path, url)
> +
> + lastline = string.strip(output.pop())
> + cm = re.compile ("(Committed|Imported) revision [0-9]+.")
> + match = cm.search (lastline)
> + if not match:
> + ### we should raise a less generic error here. which?
> + raise svntest.actions.SVNUnexpectedOutput
I'm not sure. I usually just raise svntest.Failure, actually.
> + # remove (uncontrolled) local dir
> + svntest.main.safe_rmtree(dir_path)
> +
> + # Create expected disk tree for the update (disregarding props)
> + expected_disk = svntest.main.greek_state.copy()
> + expected_disk.add({
> + 'dir/foo.c' : Item(''),
> + 'dir/foo.o' : Item(''),
> + 'dir/foo.lo' : Item(''),
> + 'dir/foo.rej' : Item(''),
> + })
> +
> + # Create expected status tree for the update (disregarding props).
> + # Newly imported file should be at revision 2.
> + expected_status = svntest.actions.get_virginal_state(wc_dir, 2)
> + expected_status.add({
> + 'dir' : Item(status=' ', wc_rev=2),
> + 'dir/foo.c' : Item(status=' ', wc_rev=2),
> + 'dir/foo.o' : Item(status=' ', wc_rev=2),
> + 'dir/foo.lo' : Item(status=' ', wc_rev=2),
> + 'dir/foo.rej' : Item(status=' ', wc_rev=2),
> + })
> +
> + # Create expected output tree for the update.
> + expected_output = svntest.wc.State(wc_dir, {
> + 'dir' : Item(status='A '),
> + 'dir/foo.c' : Item(status='A '),
> + 'dir/foo.o' : Item(status='A '),
> + 'dir/foo.lo' : Item(status='A '),
> + 'dir/foo.rej' : Item(status='A '),
> + })
> +
> + # do update and check three ways
> + svntest.actions.run_and_verify_update(wc_dir,
> + expected_output,
> + expected_disk,
> + expected_status,
> + None, None, None,
> + None, None, 1)
> +#----------------------------------------------------------------------
> def import_avoid_empty_revision(sbox):
> "avoid creating empty revisions with import"
>
> @@ -224,6 +301,7 @@
> test_list = [ None,
> Skip(import_executable, (os.name != 'posix')),
> import_ignores,
> + import_no_ignores,
> import_avoid_empty_revision,
> ]
If you always add tests at the end, then it won't mess up the
numbering (there might be issues or mail messages refering to
"import_tests N" where N is a number).
> --- subversion/tests/clients/cmdline/basic_tests.py (revision 14823)
> +++ subversion/tests/clients/cmdline/basic_tests.py (working copy)
> @@ -1440,6 +1440,35 @@
>
>
> #----------------------------------------------------------------------
> +def basic_add_no_ignores(sbox):
> + 'add ignored files in added dirs'
> +
> + # add ignored files using the '--no-ignore' option
> + sbox.build()
> + wc_dir = sbox.wc_dir
> +
> + dir_path = os.path.join(wc_dir, 'dir')
> + foo_c_path = os.path.join(dir_path, 'foo.c')
> + # add a few files that match typical ignore patterns
> + foo_o_path = os.path.join(dir_path, 'foo.o')
> + foo_lo_path = os.path.join(dir_path, 'foo.lo')
> + foo_rej_path = os.path.join(dir_path, 'foo.rej')
> +
> + os.mkdir(dir_path, 0755)
> + open(foo_c_path, 'w')
> + open(foo_o_path, 'w')
> + open(foo_lo_path, 'w')
> + open(foo_rej_path, 'w')
> +
> + output, err = svntest.actions.run_and_verify_svn(
> + "No output where some expected", SVNAnyOutput, None,
> + 'add', '--no-ignore', dir_path)
> +
> + for line in output:
> + # If we don't see ignores in the add output, fail the test.
> + if not re.match(r'^A\s+.*(foo.(o|rej|lo|c)|dir)$', line):
> + raise svntest.actions.SVNUnexpectedOutput
> +#----------------------------------------------------------------------
> def uri_syntax(sbox):
> 'make sure URI syntaxes are parsed correctly'
>
> @@ -1534,6 +1563,7 @@
> nonexistent_repository,
> basic_auth_cache,
> basic_add_ignores,
> + basic_add_no_ignores,
> uri_syntax,
> basic_checkout_file,
> basic_info,
Same comment about adding at the end.
Looking forward to v4 of the patch.
Best,
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon May 30 22:22:37 2005