Julian Foad <julian.f...@wandisco.com> wrote on 07/14/2011 06:58:24 AM:
> The attached patch makes some improvements to how we report that the WC
> is locked or busy (the 'run cleanup' messages). I need a sanity check
> to make sure I've understood the relationship between how we want to
> handle 'the work queue is busy' and 'there is a write lock on a WC
> directory'.
I'm all for removing the overloaded "lock" terminology. We see a lot
of confused users who think they somehow incorrectly locked a file
in the repository when they see the old messages...
Kevin R.
> When the WC is locked we may (depending on the timing) see:
>
> $ svn up -q & svn up -q
> svn: E155004: Working copy '/home/...' locked.
> svn: E155004: '/home/...' is already locked.
> svn: run 'svn cleanup' to remove locks (type 'svn help cleanup' for
> details)
>
> (or, depending on the timing, there may be two 'database is locked'
> lines instead of the 'is already locked' line), or
>
> $ svn up & svn commit
> svn: E155004: Commit failed (details follow):
> svn: E155004: Working copy '/home/...' locked.
> svn: E155004: '/home/...' is already locked.
> svn: run 'svn cleanup' to remove locks (type 'svn help cleanup' for
> details)
>
> When the work queue is not empty, we see:
>
> $ svn status
> svn: E155037: Previous operation has not finished; run 'cleanup' if it
> was interrupted
>
>
> My recommendations, mainly for the WQ-not-empty case:
>
> 1. The 'E155037' message shown above comes from libsvn_wc. But the
> part about running 'cleanup' should be added by 'svn', since in other
> clients it may have a different name or may not be applicable at all,
> especially if the client can find out that in fact the other operation
> is still running. So libsvn_wc would say just:
>
> "A previous operation on the working copy has not finished"
>
> and 'svn' would wrap that in:
>
> "Run 'svn cleanup' if the previous operation was interrupted"
>
> See also point (2).
>
> 2. Consider wrapping this SVN_ERR_WC_CLEANUP_REQUIRED (E155037) error
> in a SVN_ERR_WC_LOCKED (E155004) error to preserve API compatibility
> w.r.t. this kind of situation. Inside libsvn_wc, of course the
> condition being reported here is not the same as a 'lock', but to an
> outside caller the net effect is basically that it's a lock.
>
> If we don't wrap this in SVN_ERR_WC_LOCKED, then in order to support
> (1), 'svn' should learn to recognize the new error code as well, at the
> point where it determines whether to suggest the user should run
> 'cleanup'.
>
> 3. The error code SVN_ERR_WC_CLEANUP_REQUIRED is misnamed, since
> libsvn_wc does not know whether cleanup is required. Rename it to
> SVN_ERR_WC_BUSY or SVN_ERR_WC_WORK_QUEUE_NOT_EMPTY.
>
> 4. libsvn_wc should include the WC root in the error message:
>
> "A previous operation on the working copy at '<wcroot_abspath>' has
> not finished"
>
> 5. 'svn' should print its message about running 'cleanup' via the
> standard error message mechanism instead of using cmdline_fputs(), so
> that the message appears in the standard (new) format with the
> 'Ennnnnn:' prefix. This suggestion is independent of the others so I'll
> commit it separately, but it's included in this patch for you to see.
>
>
> With this patch, typical results are:
>
> $ svn up -q & svn up -q
> svn: E155004: Run 'svn cleanup' to remove locks (type 'svn help
> cleanup' for details)
> svn: E155004: Working copy '/home/...' locked.
> svn: E155004: '/home/...' is already locked.
>
> $ svn up -q & svn commit
> svn: E155004: Run 'svn cleanup' to remove locks (type 'svn help
> cleanup' for details)
> svn: E155004: Commit failed (details follow):
> svn: E155004: Working copy '/home/...' locked.
> svn: E155004: '/home/...' is already locked.
>
> and for the more interesting, WQ-not-empty, case:
>
> $ svn status
> svn: E155037: Run 'svn cleanup' to remove locks (type 'svn help
> cleanup' for details)
> svn: E155037: A previous operation on the working copy at '/home/...'
> has not finished
>
>
> I would propose all of this for 1.7.0 back-port.
>
> Concerns?
>
> - Julian
>
> Improve how we report that the WC is locked or busy (the 'run cleanup'
> messages).
>
> * subversion/include/svn_error_codes.h
> (SVN_ERR_WC_CLEANUP_REQUIRED): Rename to SVN_ERR_WC_BUSY and remove
the
> words about running 'cleanup', since libsvn_wc doesn't know whether
> cleanup is required.
>
> * subversion/libsvn_wc/wc_db_wcroot.c
> (verify_no_work): Take the WC root path and include that in the error
> message. Add a doc string.
> (svn_wc__db_pdh_create_wcroot): Adjust accordingly.
>
> * subversion/svn/main.c
> (main): Print the message about running 'cleanup' when the subcommand
> returned SVN_ERR_WC_BUSY as well as when it returned
SVN_ERR_WC_LOCKED.
> Print the message about running 'cleanup' using the standard
mechanism
> for error messages (so, for example, the error code will be
displayed).
> --This line, and those below, will be ignored--
>
> Index: subversion/include/svn_error_codes.h
> ===================================================================
> --- subversion/include/svn_error_codes.h (revision 1145998)
> +++ subversion/include/svn_error_codes.h (working copy)
> @@ -520,10 +520,9 @@ SVN_ERROR_START
> "The working copy needs to be upgraded")
>
> /** @since New in 1.7. */
> - SVN_ERRDEF(SVN_ERR_WC_CLEANUP_REQUIRED,
> + SVN_ERRDEF(SVN_ERR_WC_BUSY,
> SVN_ERR_WC_CATEGORY_START + 37,
> - "Previous operation has not finished; "
> - "run 'cleanup' if it was interrupted")
> + "A previous operation on the working copy has not
finished")
>
> /** @since New in 1.7. */
> SVN_ERRDEF(SVN_ERR_WC_INVALID_OPERATION_DEPTH,
> Index: subversion/libsvn_wc/wc_db_wcroot.c
> ===================================================================
> --- subversion/libsvn_wc/wc_db_wcroot.c (revision 1145998)
> +++ subversion/libsvn_wc/wc_db_wcroot.c (working copy)
> @@ -139,9 +139,12 @@ get_path_kind(svn_wc__db_t *db,
> }
>
>
> -/* */
> +/* Return an error if the work queue in SDB is non-empty.
WCROOT_ABSPATH
> + * is used in the error message. */
> static svn_error_t *
> -verify_no_work(svn_sqlite__db_t *sdb)
> +verify_no_work(svn_sqlite__db_t *sdb,
> + const char *wcroot_abspath,
> + apr_pool_t *scratch_pool)
> {
> svn_sqlite__stmt_t *stmt;
> svn_boolean_t have_row;
> @@ -151,8 +154,11 @@ verify_no_work(svn_sqlite__db_t *sdb)
> SVN_ERR(svn_sqlite__reset(stmt));
>
> if (have_row)
> - return svn_error_create(SVN_ERR_WC_CLEANUP_REQUIRED, NULL,
> - NULL /* nothing to add. */);
> + return svn_error_createf(SVN_ERR_WC_BUSY, NULL,
> + _("A previous operation on the working "
> + "copy at '%s' has not finished"),
> + svn_dirent_local_style(wcroot_abspath,
> + scratch_pool));
>
> return SVN_NO_ERROR;
> }
> @@ -275,12 +281,12 @@ svn_wc__db_pdh_create_wcroot(svn_wc__db_
> if (format >= SVN_WC__HAS_WORK_QUEUE
> && (enforce_empty_wq || (format < SVN_WC__VERSION &&
auto_upgrade)))
> {
> - svn_error_t *err = verify_no_work(sdb);
> + svn_error_t *err = verify_no_work(sdb, wcroot_abspath,
scratch_pool);
> if (err)
> {
> /* Special message for attempts to upgrade a 1.7-dev wc with
> outstanding workqueue items. */
> - if (err->apr_err == SVN_ERR_WC_CLEANUP_REQUIRED
> + if (err->apr_err == SVN_ERR_WC_BUSY
> && format < SVN_WC__VERSION && auto_upgrade)
> err = svn_error_quick_wrap(err, _("Cleanup with an older
1.7 "
> "client before upgrading
with "
> Index: subversion/svn/main.c
> ===================================================================
> --- subversion/svn/main.c (revision 1145998)
> +++ subversion/svn/main.c (working copy)
> @@ -2596,6 +2596,14 @@ main(int argc, const char *argv[])
> _("Please see the 'svn
> upgrade' command"));
> }
>
> + /* Tell the user about 'svn cleanup' if any error on the stack
> + was about locked working copies. */
> + if (svn_error_find_cause(err, SVN_ERR_WC_LOCKED)
> + || svn_error_find_cause(err, SVN_ERR_WC_BUSY))
> + err = svn_error_quick_wrap(err,
> + _("Run 'svn cleanup' to remove locks
"
> + "(type 'svn help cleanup' for
> details)"));
> +
> /* Issue #3014:
> * Don't print anything on broken pipes. The pipe was likely
> * closed by the process at the other end. We expect that
> @@ -2606,14 +2614,6 @@ main(int argc, const char *argv[])
> if (err->apr_err != SVN_ERR_IO_PIPE_WRITE_ERROR)
> svn_handle_error2(err, stderr, FALSE, "svn: ");
>
> - /* Tell the user about 'svn cleanup' if any error on the stack
> - was about locked working copies. */
> - if (svn_error_find_cause(err, SVN_ERR_WC_LOCKED))
> - svn_error_clear(svn_cmdline_fputs(_("svn: run 'svn cleanup' to
"
> - "remove locks (type 'svn
help "
> - "cleanup' for details)\n"),
> - stderr, pool));
> -
> svn_error_clear(err);
> svn_pool_destroy(pool);
> return EXIT_FAILURE;
