You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2005/04/27 22:01:12 UTC
[PATCH] Issue #2285
I can't seem to reach tigris.org right now, so I'm posting this
preliminary patch here, in case anyone wants to review. I'm still
testing it ('make check' takes a while), and there may be one
refactorization in the test suite I want to do before committing.
[[[
Fix issue #2285: Non-commit operations should error if given a log message.
### Not ready for commit yet, still being tested. ###
* subversion/include/svn_error_codes.h
(SVN_ERR_CL_UNNECESSARY_LOG_MESSAGE): New error code.
* subversion/libsvn_client/client.h, subversion/libsvn_client/log.c
(svn_client__ensure_no_log_message): New helper function.
* subversion/libsvn_client/copy.c
(wc_to_wc_copy, repos_to_wc_copy): Call new helper function.
* subversion/libsvn_client/delete.c
(svn_client_delete): Call new helper function.
* subversion/libsvn_client/add.c
(svn_client_mkdir): Call new helper function.
* subversion/tests/clients/cmdline/commit_tests.py
(local_mods_are_not_commits): New test.
(test_list): Run it.
* subversion/tests/clients/cmdline/svntest/actions.py
(match_or_fail): New helper, does regexp matching.
(display_lines): New parameter 'expected_is_regexp', conditionally
adjust display accordingly.
(run_and_verify_svn): Treat string type in 'expected' parameter as a
regexp to match.
]]]
Index: subversion/include/svn_error_codes.h
===================================================================
--- subversion/include/svn_error_codes.h (revision 14483)
+++ subversion/include/svn_error_codes.h (working copy)
@@ -1008,6 +1008,10 @@
SVN_ERR_CL_CATEGORY_START + 8,
"Something is wrong with the log message's contents")
+ SVN_ERRDEF (SVN_ERR_CL_UNNECESSARY_LOG_MESSAGE,
+ SVN_ERR_CL_CATEGORY_START + 9,
+ "A log message was given where none was necessary")
+
SVN_ERROR_END
Index: subversion/libsvn_client/delete.c
===================================================================
--- subversion/libsvn_client/delete.c (revision 14483)
+++ subversion/libsvn_client/delete.c (working copy)
@@ -260,6 +260,8 @@
svn_pool_clear (subpool);
parent_path = svn_path_dirname (path, subpool);
+ SVN_ERR (svn_client__ensure_no_log_message (ctx, pool));
+
/* See if the user wants us to stop. */
if (ctx->cancel_func)
SVN_ERR (ctx->cancel_func (ctx->cancel_baton));
Index: subversion/libsvn_client/client.h
===================================================================
--- subversion/libsvn_client/client.h (revision 14483)
+++ subversion/libsvn_client/client.h (working copy)
@@ -176,6 +176,13 @@
apr_pool_t *pool);
+/* Try to extract a log message from CTX, and if succeed, return
+ SVN_ERR_CL_UNNECESSARY_LOG_MESSAGE. Use POOL for temporary
+ allocation only. */
+svn_error_t *
+svn_client__ensure_no_log_message (svn_client_ctx_t *ctx,
+ apr_pool_t *pool);
+
/* ---------------------------------------------------------------- */
/*** RA callbacks ***/
Index: subversion/libsvn_client/copy.c
===================================================================
--- subversion/libsvn_client/copy.c (revision 14483)
+++ subversion/libsvn_client/copy.c (working copy)
@@ -74,6 +74,8 @@
svn_wc_adm_access_t *adm_access, *src_access;
svn_error_t *err;
+ SVN_ERR (svn_client__ensure_no_log_message (ctx, pool));
+
/* Verify that SRC_PATH exists. */
SVN_ERR (svn_io_check_path (src_path, &src_kind, pool));
if (src_kind == svn_node_none)
@@ -760,6 +762,8 @@
svn_boolean_t same_repositories;
svn_opt_revision_t revision;
+ SVN_ERR (svn_client__ensure_no_log_message (ctx, pool));
+
/* Open a repository session to the given URL. We do not (yet) have a
working copy, so we don't have a corresponding path and tempfiles
cannot go into the admin area. */
Index: subversion/libsvn_client/log.c
===================================================================
--- subversion/libsvn_client/log.c (revision 14483)
+++ subversion/libsvn_client/log.c (working copy)
@@ -37,9 +37,30 @@
#include "svn_private_config.h"
-/*** Getting update information ***/
+/*** Helper used by commands other than log. ***/
+svn_error_t *
+svn_client__ensure_no_log_message (svn_client_ctx_t *ctx,
+ apr_pool_t *pool)
+{
+ if (ctx->log_msg_func)
+ {
+ const char *message, *tmp_file;
+ apr_array_header_t *commit_items_decoy =
+ apr_array_make (pool, 0, sizeof (svn_client_commit_item_t *));
+ SVN_ERR ((*ctx->log_msg_func) (&message, &tmp_file, commit_items_decoy,
+ ctx->log_msg_baton, pool));
+ if (tmp_file || (message && (message[0] != '\0')))
+ return svn_error_createf
+ (SVN_ERR_CL_UNNECESSARY_LOG_MESSAGE, NULL,
+ _("Local, non-commit operations do not take a log message"));
+ }
+
+ return SVN_NO_ERROR;
+}
+
+
/*** Public Interface. ***/
Index: subversion/libsvn_client/add.c
===================================================================
--- subversion/libsvn_client/add.c (revision 14483)
+++ subversion/libsvn_client/add.c (working copy)
@@ -622,6 +622,8 @@
svn_error_t *err;
int i;
+ SVN_ERR (svn_client__ensure_no_log_message (ctx, pool));
+
for (i = 0; i < paths->nelts; i++)
{
const char *path = APR_ARRAY_IDX (paths, i, const char *);
Index: subversion/tests/clients/cmdline/commit_tests.py
===================================================================
--- subversion/tests/clients/cmdline/commit_tests.py (revision 14483)
+++ subversion/tests/clients/cmdline/commit_tests.py (working copy)
@@ -1902,7 +1902,59 @@
source_url, tab_url)
match_bad_tab_path(tab_dir, errlines)
+#----------------------------------------------------------------------
+def local_mods_are_not_commits(sbox):
+ "local ops should not be treated like commits"
+
+ # For issue #2285.
+ #
+ # Some commands can run on either a URL or a local path. These
+ # commands take a log message, intended for the URL case.
+ # Therefore, they should make sure that getting a log message for
+ # a local operation errors (because not committing).
+ #
+ # This is in commit_tests.py because the unifying theme is that
+ # commits are *not* happening. And because there was no better
+ # place to put it :-).
+
+ sbox.build()
+ wc_dir = sbox.wc_dir
+ expected_error = '.*Local, non-commit operations do not take a log message.*'
+
+ # copy wc->wc
+ svntest.actions.run_and_verify_svn(None, None, expected_error,
+ 'cp', '-m', 'log msg',
+ os.path.join(wc_dir, 'iota'),
+ os.path.join(wc_dir, 'iota2'))
+
+ # copy repos->wc
+ svntest.actions.run_and_verify_svn(None, None, expected_error,
+ 'cp', '-m', 'log msg',
+ '--username',
+ svntest.main.wc_author,
+ '--password',
+ svntest.main.wc_passwd,
+ svntest.main.current_repo_url + "/iota",
+ os.path.join(wc_dir, 'iota2'))
+
+ # delete
+ svntest.actions.run_and_verify_svn(None, None, expected_error,
+ 'rm', '-m', 'log msg',
+ os.path.join(wc_dir, 'A', 'D', 'gamma'))
+
+ # mkdir
+ svntest.actions.run_and_verify_svn(None, None, expected_error,
+ 'mkdir', '-m', 'log msg',
+ os.path.join(wc_dir, 'newdir'))
+
+ # rename
+ svntest.actions.run_and_verify_svn(None, None, expected_error,
+ 'cp', '-m', 'log msg',
+ os.path.join(wc_dir, 'A', 'mu'),
+ os.path.join(wc_dir, 'A', 'yu'))
+
+
########################################################################
# Run the tests
@@ -1941,6 +1993,7 @@
from_wc_top_with_bad_editor,
mods_in_schedule_delete,
Skip(tab_test, (os.name != 'posix' or sys.platform == 'cygwin')),
+ local_mods_are_not_commits,
]
if __name__ == '__main__':
Index: subversion/tests/clients/cmdline/svntest/actions.py
===================================================================
--- subversion/tests/clients/cmdline/svntest/actions.py (revision 14483)
+++ subversion/tests/clients/cmdline/svntest/actions.py (working copy)
@@ -171,6 +171,8 @@
if type(expected_stdout) is type([]):
compare_and_display_lines(message, 'STDOUT', expected_stdout, out)
+ elif type(expected_stdout) is type(''):
+ match_or_fail(message, 'STDOUT', expected_stdout, out)
elif expected_stdout == SVNAnyOutput:
if len(out) == 0:
if message is not None: print message
@@ -180,6 +182,8 @@
if type(expected_stderr) is type([]):
compare_and_display_lines(message, 'STDERR', expected_stderr, err)
+ elif type(expected_stderr) is type(''):
+ match_or_fail(message, 'STDERR', expected_stderr, err)
elif expected_stderr == SVNAnyOutput:
if len(err) == 0:
if message is not None: print message
@@ -698,13 +702,18 @@
tree.dump_tree(actual)
-def display_lines(message, label, expected, actual):
+def display_lines(message, label, expected, actual, expected_is_regexp=None):
'Print two sets of output lines, expected and actual.'
if message is not None:
print message
if expected is not None:
- print 'EXPECTED', label + ':'
+ if expected_is_regexp:
+ print 'EXPECTED', label + ' (regexp):'
+ else:
+ print 'EXPECTED', label + ':'
map(sys.stdout.write, expected)
+ if expected_is_regexp:
+ map(sys.stdout.write, '\n')
if actual is not None:
print 'ACTUAL', label + ':'
map(sys.stdout.write, actual)
@@ -721,6 +730,17 @@
display_lines(message, label, expected, actual)
raise main.SVNLineUnequal
+def match_or_fail(message, label, expected, actual):
+ """Make sure that regexp EXPECTED matches at least one line in list ACTUAL.
+ If no match, then print MESSAGE (if it's not None), followed by
+ EXPECTED and ACTUAL, both labeled with LABEL, and raise SVNLineUnequal."""
+ matched = None
+ for line in actual:
+ if re.match(expected, line):
+ matched = 1
+ if not matched:
+ display_lines(message, label, expected, actual, 1)
+ raise main.SVNLineUnequal
######################################################################
# Other general utilities
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #2285
Posted by Julian Foad <ju...@btopenworld.com>.
kfogel@collab.net wrote:
> I can't seem to reach tigris.org right now, so I'm posting this
> preliminary patch here, in case anyone wants to review. I'm still
> testing it ('make check' takes a while), and there may be one
> refactorization in the test suite I want to do before committing.
>
> [[[
> Fix issue #2285: Non-commit operations should error if given a log message.
>
> ### Not ready for commit yet, still being tested. ###
>
> * subversion/include/svn_error_codes.h
> (SVN_ERR_CL_UNNECESSARY_LOG_MESSAGE): New error code.
>
> * subversion/libsvn_client/client.h, subversion/libsvn_client/log.c
> (svn_client__ensure_no_log_message): New helper function.
>
> * subversion/libsvn_client/copy.c
> (wc_to_wc_copy, repos_to_wc_copy): Call new helper function.
>
> * subversion/libsvn_client/delete.c
> (svn_client_delete): Call new helper function.
>
> * subversion/libsvn_client/add.c
> (svn_client_mkdir): Call new helper function.
>
> * subversion/tests/clients/cmdline/commit_tests.py
> (local_mods_are_not_commits): New test.
> (test_list): Run it.
>
> * subversion/tests/clients/cmdline/svntest/actions.py
> (match_or_fail): New helper, does regexp matching.
> (display_lines): New parameter 'expected_is_regexp', conditionally
> adjust display accordingly.
> (run_and_verify_svn): Treat string type in 'expected' parameter as a
> regexp to match.
> ]]]
It would be better to do the test framework enhancements in a separate commit.
>
> Index: subversion/include/svn_error_codes.h
> ===================================================================
> --- subversion/include/svn_error_codes.h (revision 14483)
> +++ subversion/include/svn_error_codes.h (working copy)
> @@ -1008,6 +1008,10 @@
> SVN_ERR_CL_CATEGORY_START + 8,
> "Something is wrong with the log message's contents")
>
> + SVN_ERRDEF (SVN_ERR_CL_UNNECESSARY_LOG_MESSAGE,
> + SVN_ERR_CL_CATEGORY_START + 9,
> + "A log message was given where none was necessary")
> +
> SVN_ERROR_END
> Index: subversion/libsvn_client/client.h
> ===================================================================
> --- subversion/libsvn_client/client.h (revision 14483)
> +++ subversion/libsvn_client/client.h (working copy)
> @@ -176,6 +176,13 @@
> apr_pool_t *pool);
>
>
> +/* Try to extract a log message from CTX, and if succeed, return
> + SVN_ERR_CL_UNNECESSARY_LOG_MESSAGE. Use POOL for temporary
> + allocation only. */
> +svn_error_t *
> +svn_client__ensure_no_log_message (svn_client_ctx_t *ctx,
"const svn_client_ctx_t *"?
> + apr_pool_t *pool);
> +
> Index: subversion/libsvn_client/log.c
> ===================================================================
> --- subversion/libsvn_client/log.c (revision 14483)
> +++ subversion/libsvn_client/log.c (working copy)
> @@ -37,9 +37,30 @@
> #include "svn_private_config.h"
>
>
> -/*** Getting update information ***/
> +/*** Helper used by commands other than log. ***/
>
> +svn_error_t *
> +svn_client__ensure_no_log_message (svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
Would this not fit better in "commit.c" or "commit_util.c"?
> +{
> + if (ctx->log_msg_func)
> + {
> + const char *message, *tmp_file;
> + apr_array_header_t *commit_items_decoy =
> + apr_array_make (pool, 0, sizeof (svn_client_commit_item_t *));
>
> + SVN_ERR ((*ctx->log_msg_func) (&message, &tmp_file, commit_items_decoy,
> + ctx->log_msg_baton, pool));
> + if (tmp_file || (message && (message[0] != '\0')))
This doesn't quite seem to correspond with the callback function type
svn_client_get_commit_log_t's doc string, though the latter is unclear and may
be the one at fault.
Isn't the log message editor (when so configured/specified) invoked when
log_msg_func is called? If so, this is going to invoke the log message editor
where it previously would not have been invoked, which is a bit odd, but
tolerable because this is the user-error case. Further, if the log message
editor was exited with no message written and the user chose to "cancel", it
seems to be going to allow the non-committy operation to proceed. That would
be nasty. I hope I misunderstand.
> + return svn_error_createf
> + (SVN_ERR_CL_UNNECESSARY_LOG_MESSAGE, NULL,
> + _("Local, non-commit operations do not take a log message"));
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> Index: subversion/tests/clients/cmdline/commit_tests.py
> ===================================================================
> --- subversion/tests/clients/cmdline/commit_tests.py (revision 14483)
> +++ subversion/tests/clients/cmdline/commit_tests.py (working copy)
> @@ -1902,7 +1902,59 @@
> source_url, tab_url)
> match_bad_tab_path(tab_dir, errlines)
>
> +#----------------------------------------------------------------------
>
> +def local_mods_are_not_commits(sbox):
> + "local ops should not be treated like commits"
> +
> + # For issue #2285.
> + #
> + # Some commands can run on either a URL or a local path. These
> + # commands take a log message, intended for the URL case.
> + # Therefore, they should make sure that getting a log message for
> + # a local operation errors (because not committing).
> + #
> + # This is in commit_tests.py because the unifying theme is that
> + # commits are *not* happening. And because there was no better
> + # place to put it :-).
Right. Doesn't the same apply to the new C function?
> +
> + sbox.build()
> + wc_dir = sbox.wc_dir
> + expected_error = '.*Local, non-commit operations do not take a log message.*'
> +
> + # copy wc->wc
> + svntest.actions.run_and_verify_svn(None, None, expected_error,
> + 'cp', '-m', 'log msg',
> + os.path.join(wc_dir, 'iota'),
> + os.path.join(wc_dir, 'iota2'))
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #2285
Posted by kf...@collab.net.
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
> Well, I think the cmdline client knows, at least if it asks a simple
> question. I mean, there is no magic in libsvn_client that makes it
> difficult for the cmdline client to know if it will do a commit or just
> local fiddling. The API docs state that if dst_path (or paths) is/are
> URL(s), then it is a repository operation. So, why not just make cmdline
> check if -F or -m was specified and then check if the target(s) are URLs.
Hmmm. Okay, I'll try that way.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #2285
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Tue, 2 May 2005 kfogel@collab.net wrote:
> "Peter N. Lundblad" <pe...@famlundblad.se> writes:
> > I'm uncomfortable with this idea. It makes invoking the log message func
> > with 0 commit items mean "don't ask the user, but give me a message if it
> > was specified by some other means" or something like that. That's not
> > documented in the API and I don't think we can introduce such a change and
> > be compatible.
> >
> > Also, to be honest, I think it is very hacky. Why not just make the
> > cmdline client check that -F or -m weren't specified when an operation was
> > invoking without an URL? I don't think libsvn_client should ask for a
> > message unless it really wants one.
>
> The problem is that the command-line client does not *know* if the
> target is a URL or not. That's not determined until we get way down
> into the libsvn_client, at which point the only way we know whether or
> not a log message was obtained is via the log message callback. I'm
> open to suggestions, but so far I've been unable to think of a less
> hacky way to do it :-(.
>
Well, I think the cmdline client knows, at least if it asks a simple
question. I mean, there is no magic in libsvn_client that makes it
difficult for the cmdline client to know if it will do a commit or just
local fiddling. The API docs state that if dst_path (or paths) is/are
URL(s), then it is a repository operation. So, why not just make cmdline
check if -F or -m was specified and then check if the target(s) are URLs.
> As for the log message function: I'm only depending on the actual
> behavior of our implementation (since I didn't change it), so although
"our implementation" is the problem here.
> technically there might be a compatibility concern here, I don't think
> it would be a problem in practice. We should document the behavior if
How do you know? (I don't think this is a theoreticla discussion.) Say I
wrote my PerfectSVN client. Its log_msg_func pops up a dialog when called,
but it doesn't special-case the zero commit items case. That's reasonable
since nothing told it to.
To be more concise: ABI also inlcudes semantics. Before log_msg_func was
invoked when we *wanted* a message; now it is also invoked when we *don't*
want a message:-)
> we're going to depend on it, though, yes.
>
Hm, "...invoke log_msg_func/baton when a log messages is needed - or not
needed":-)
OK, I'll stop being nasty. But I really think this introduces confusion
into our APIs without it being necessary.
Best,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #2285
Posted by kf...@collab.net.
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
> I'm uncomfortable with this idea. It makes invoking the log message func
> with 0 commit items mean "don't ask the user, but give me a message if it
> was specified by some other means" or something like that. That's not
> documented in the API and I don't think we can introduce such a change and
> be compatible.
>
> Also, to be honest, I think it is very hacky. Why not just make the
> cmdline client check that -F or -m weren't specified when an operation was
> invoking without an URL? I don't think libsvn_client should ask for a
> message unless it really wants one.
The problem is that the command-line client does not *know* if the
target is a URL or not. That's not determined until we get way down
into the libsvn_client, at which point the only way we know whether or
not a log message was obtained is via the log message callback. I'm
open to suggestions, but so far I've been unable to think of a less
hacky way to do it :-(.
As for the log message function: I'm only depending on the actual
behavior of our implementation (since I didn't change it), so although
technically there might be a compatibility concern here, I don't think
it would be a problem in practice. We should document the behavior if
we're going to depend on it, though, yes.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Issue #2285
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Wed, 27 Apr 2005 kfogel@collab.net wrote:
> +/*** Helper used by commands other than log. ***/
>
> +svn_error_t *
> +svn_client__ensure_no_log_message (svn_client_ctx_t *ctx,
> + apr_pool_t *pool)
> +{
> + if (ctx->log_msg_func)
> + {
> + const char *message, *tmp_file;
> + apr_array_header_t *commit_items_decoy =
> + apr_array_make (pool, 0, sizeof (svn_client_commit_item_t *));
>
> + SVN_ERR ((*ctx->log_msg_func) (&message, &tmp_file, commit_items_decoy,
> + ctx->log_msg_baton, pool));
> + if (tmp_file || (message && (message[0] != '\0')))
> + return svn_error_createf
> + (SVN_ERR_CL_UNNECESSARY_LOG_MESSAGE, NULL,
> + _("Local, non-commit operations do not take a log message"));
> + }
> +
> + return SVN_NO_ERROR;
> +}
I'm uncomfortable with this idea. It makes invoking the log message func
with 0 commit items mean "don't ask the user, but give me a message if it
was specified by some other means" or something like that. That's not
documented in the API and I don't think we can introduce such a change and
be compatible.
Also, to be honest, I think it is very hacky. Why not just make the
cmdline client check that -F or -m weren't specified when an operation was
invoking without an URL? I don't think libsvn_client should ask for a
message unless it really wants one.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org