[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Issue #2285

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: 2005-04-28 02:13:10 CEST

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
Received on Thu Apr 28 02:49:50 2005

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.