On Fri, Nov 21, 2008 at 07:37:54PM -0800, neels_at_tigris.org wrote:
> Author: neels
> Date: Fri Nov 21 19:37:54 2008
> New Revision: 34329
>
> Log:
> Fix some confusion with "incoming", "local", "older" and "their" versions.
Ouch, sorry about that :/
Some comments inline.
> Include the operation that caused the tree-conflict in abbreviated
> info output.
>
> Show a proper "their" path on some delete cases (in update code).
>
> * subversion/include/svn_wc.h
> (svn_wc_operation_str):
> New helper function that returns a string for an svn_wc_operation_t.
> ### TODO: really go public?
>
> * subversion/libsvn_wc/util.c
> (svn_wc_operation_str):
> New helper function that returns a string for an svn_wc_operation_t.
>
> * subversion/libsvn_wc/update_editor.c
> (do_entry_deletion):
> New argument THEIR_PATH propagated from delete_entry(), for passing
> it to check_tree_conflict().
> (delete_entry): Fill in a THEIR_PATH for do_entry_deletion().
> (close_edit, complete_directory): Just pass NULL for THEIR_PATH. ### TODO
>
> * subversion/svn/info-cmd.c
> (print_info): Don't take older/their as local/incoming. Capitalize output.
>
> * subversion/svn/tree-conflicts.c
> (select_action, select_reason): Rename these...
> (action_str, reason_str): ...to these
>
> Modified:
> branches/tc_url_rev/subversion/include/svn_wc.h
> branches/tc_url_rev/subversion/libsvn_wc/update_editor.c
> branches/tc_url_rev/subversion/libsvn_wc/util.c
> branches/tc_url_rev/subversion/svn/info-cmd.c
> branches/tc_url_rev/subversion/svn/tree-conflicts.c
>
> Modified: branches/tc_url_rev/subversion/include/svn_wc.h
> URL: http://svn.collab.net/viewvc/svn/branches/tc_url_rev/subversion/include/svn_wc.h?pathrev=34329&r1=34328&r2=34329
> ==============================================================================
> --- branches/tc_url_rev/subversion/include/svn_wc.h Fri Nov 21 15:55:47 2008 (r34328)
> +++ branches/tc_url_rev/subversion/include/svn_wc.h Fri Nov 21 19:37:54 2008 (r34329)
> @@ -1167,6 +1167,12 @@ typedef enum svn_wc_operation_t
>
> } svn_wc_operation_t;
>
> +/** Provides a human redable string for a given @a operation.
> + * @since New in 1.6.
> + */
> +const char *
> +svn_wc_operation_str(svn_wc_operation_t operation, apr_pool_t *pool);
> +
Why not call it svn_wc_operation_str_human_readable()?
We may want an XML-compatible version of this function, too.
> /** Info about one of the conflicting versions of a node. Each field may
> * have its respective null/invalid/unknown value if the corresponding
>
> Modified: branches/tc_url_rev/subversion/libsvn_wc/update_editor.c
> URL: http://svn.collab.net/viewvc/svn/branches/tc_url_rev/subversion/libsvn_wc/update_editor.c?pathrev=34329&r1=34328&r2=34329
> ==============================================================================
> --- branches/tc_url_rev/subversion/libsvn_wc/update_editor.c Fri Nov 21 15:55:47 2008 (r34328)
> +++ branches/tc_url_rev/subversion/libsvn_wc/update_editor.c Fri Nov 21 19:37:54 2008 (r34329)
> @@ -556,6 +556,7 @@ static svn_error_t *
> do_entry_deletion(struct edit_baton *eb,
> const char *parent_path,
> const char *path,
> + const char *their_path,
> int *log_number,
> apr_pool_t *pool);
>
> @@ -610,7 +611,7 @@ complete_directory(struct edit_baton *eb
> if (!target_access && entry->kind == svn_node_dir)
> {
> int log_number = 0;
> - SVN_ERR(do_entry_deletion(eb, eb->anchor, eb->target,
> + SVN_ERR(do_entry_deletion(eb, eb->anchor, eb->target, NULL,
> &log_number, pool));
> }
> else
> @@ -1625,6 +1626,7 @@ static svn_error_t *
> do_entry_deletion(struct edit_baton *eb,
> const char *parent_path,
> const char *path,
> + const char *their_path,
> int *log_number,
> apr_pool_t *pool)
> {
> @@ -1677,7 +1679,7 @@ do_entry_deletion(struct edit_baton *eb,
> SVN_ERR(check_tree_conflict(&tree_conflict, eb, log_item, full_path,
> entry, adm_access,
> svn_wc_conflict_action_delete,
> - svn_node_none, NULL, pool));
> + svn_node_none, their_path, pool));
>
> if (tree_conflict != NULL)
> {
> @@ -1821,11 +1823,13 @@ delete_entry(const char *path,
> apr_pool_t *pool)
> {
> struct dir_baton *pb = parent_baton;
> -
> - SVN_ERR(check_path_under_root(pb->path, svn_path_basename(path, pool),
> - pool));
> - return do_entry_deletion(pb->edit_baton, pb->path, path, &pb->log_number,
> - pool);
> + const char *path_basename = svn_path_basename(path, pool);
> + const char *their_path = svn_path_url_add_component(pb->new_URL,
> + path_basename, pool);
> +
> + SVN_ERR(check_path_under_root(pb->path, path_basename, pool));
> + return do_entry_deletion(pb->edit_baton, pb->path, path, their_path,
> + &pb->log_number, pool);
> }
>
>
> @@ -4057,7 +4061,8 @@ close_edit(void *edit_baton,
> pretend that the editor deleted the entry. The helper function
> do_entry_deletion() will take care of the necessary steps. */
> if ((*eb->target) && (svn_wc__adm_missing(eb->adm_access, target_path)))
> - SVN_ERR(do_entry_deletion(eb, eb->anchor, eb->target, &log_number, pool));
> + SVN_ERR(do_entry_deletion(eb, eb->anchor, eb->target, NULL,
> + &log_number, pool));
>
> /* The editor didn't even open the root; we have to take care of
> some cleanup stuffs. */
>
> Modified: branches/tc_url_rev/subversion/libsvn_wc/util.c
> URL: http://svn.collab.net/viewvc/svn/branches/tc_url_rev/subversion/libsvn_wc/util.c?pathrev=34329&r1=34328&r2=34329
> ==============================================================================
> --- branches/tc_url_rev/subversion/libsvn_wc/util.c Fri Nov 21 15:55:47 2008 (r34328)
> +++ branches/tc_url_rev/subversion/libsvn_wc/util.c Fri Nov 21 19:37:54 2008 (r34329)
> @@ -29,6 +29,7 @@
> #include "svn_dirent_uri.h"
> #include "svn_path.h"
> #include "wc.h" /* just for prototypes of things in this .c file */
> +#include "tree_conflicts.h" /* just for the SVN_WC__OPERATION* defines */
> #include "private/svn_wc_private.h"
>
> #include "svn_private_config.h"
> @@ -406,3 +407,18 @@ svn_wc__conflict_version_dup(const svn_w
>
> return new_version;
> }
> +
> +const char *
> +svn_wc_operation_str(svn_wc_operation_t operation, apr_pool_t *pool)
> +{
> + switch(operation){
> + case svn_wc_operation_update:
> + return SVN_WC__OPERATION_UPDATE;
> + case svn_wc_operation_switch:
> + return SVN_WC__OPERATION_SWITCH;
> + case svn_wc_operation_merge:
> + return SVN_WC__OPERATION_MERGE;
You haven't marked the above strings for translation.
> + }
> + return _("unknown operation");
> +}
> +
>
> Modified: branches/tc_url_rev/subversion/svn/info-cmd.c
> URL: http://svn.collab.net/viewvc/svn/branches/tc_url_rev/subversion/svn/info-cmd.c?pathrev=34329&r1=34328&r2=34329
> ==============================================================================
> --- branches/tc_url_rev/subversion/svn/info-cmd.c Fri Nov 21 15:55:47 2008 (r34328)
> +++ branches/tc_url_rev/subversion/svn/info-cmd.c Fri Nov 21 19:37:54 2008 (r34329)
> @@ -431,20 +431,20 @@ print_info(void *baton,
>
> if (info->tree_conflict)
> {
> - const char *desc, *incoming_version, *local_version;
> + const char *desc, *older_version, *their_version;
>
> SVN_ERR(svn_cl__get_human_readable_tree_conflict_description(
> &desc, info->tree_conflict, pool));
> - incoming_version =
> - svn_cl__node_description(info->tree_conflict->their_version, pool);
> - local_version =
> + older_version =
> svn_cl__node_description(info->tree_conflict->older_version, pool);
> + their_version =
> + svn_cl__node_description(info->tree_conflict->their_version, pool);
>
> svn_cmdline_printf(pool,
> _("Tree conflict: %s\n"
> - " incoming version: %s\n"
> - " local version: %s"),
> - desc, incoming_version, local_version);
> + " Older version: %s\n"
> + " Their version: %s"),
> + desc, older_version, their_version);
I'm wondering why we use "older" and "their" for merge.
It only makes sense for update.
In case of merge, wouldn't it be more straightforward to say
"merge-right" and "merge-left"?
> }
>
> /* Print extra newline separator. */
>
> Modified: branches/tc_url_rev/subversion/svn/tree-conflicts.c
> URL: http://svn.collab.net/viewvc/svn/branches/tc_url_rev/subversion/svn/tree-conflicts.c?pathrev=34329&r1=34328&r2=34329
> ==============================================================================
This hunk below should have been done on trunk, because
it's got nothing do to with the topic of the branch.
Please revert this hunk on the branch.
And I'm -1 on applying it to trunk anyway, see below.
> --- branches/tc_url_rev/subversion/svn/tree-conflicts.c Fri Nov 21 15:55:47 2008 (r34328)
> +++ branches/tc_url_rev/subversion/svn/tree-conflicts.c Fri Nov 21 19:37:54 2008 (r34329)
> @@ -25,7 +25,7 @@
> #include "svn_private_config.h"
>
> static const char *
> -select_action(const svn_wc_conflict_description_t *conflict)
> +action_str(const svn_wc_conflict_description_t *conflict)
> {
> switch (conflict->action)
> {
> @@ -35,13 +35,13 @@ select_action(const svn_wc_conflict_desc
> case svn_wc_conflict_action_add:
> return _("add");
> case svn_wc_conflict_action_delete:
> - return _("delete");
> + return _("delete or move");
I don't think this is a good idea.
Part of what I did not like about the old long human readable
descriptions was that they went out of their way to explain
to users that a move may be involved.
I am now thinking we should not pretend to be smarter than we
really are. Let's just say "delete", because we really don't
know any better. And people *know* that Subversion represents
moves as delete+add. We don't hide this fact in the update/status
output either. Just saying "delete" is more consistent.
Mercurial also does the same, btw., it just says delete.
Once the working copy has been made aware of moves, we can
adjust the output without having to lie about what we know.
> }
> return NULL;
> }
>
> static const char *
> -select_reason(const svn_wc_conflict_description_t *conflict)
> +reason_str(const svn_wc_conflict_description_t *conflict)
> {
> switch (conflict->reason)
> {
> @@ -51,7 +51,7 @@ select_reason(const svn_wc_conflict_desc
> case svn_wc_conflict_reason_obstructed:
> return _("obstruction");
> case svn_wc_conflict_reason_deleted:
> - return _("delete");
> + return _("delete or move");
> case svn_wc_conflict_reason_added:
> return _("add");
> case svn_wc_conflict_reason_missing:
> @@ -68,12 +68,14 @@ svn_cl__get_human_readable_tree_conflict
> const svn_wc_conflict_description_t *conflict,
> apr_pool_t *pool)
> {
> - const char *victim_name, *action, *reason;
> + const char *victim_name, *action, *reason, *operation;
> victim_name = svn_path_basename(conflict->path, pool);
> - action = select_action(conflict);
> - reason = select_reason(conflict);
> + reason = reason_str(conflict);
> + action = action_str(conflict);
> + operation = svn_wc_operation_str(conflict->operation, pool);
> SVN_ERR_ASSERT(action && reason);
> - *desc = apr_psprintf(pool, _("incoming %s, local %s"), action, reason);
> + *desc = apr_psprintf(pool, _("local %s, incoming %s (upon %s)."),
> + reason, action, operation);
I liked it better without parentheses and full-stop, and I don't think
that printing the operation which produced the conflict really makes
much sense.
The typical user will still remember what they were doing just before
the conflicts came about. It's not like people run an update/merge and
resolve conflicts 2 weeks later.
Stefan
> return SVN_NO_ERROR;
> }
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe_at_subversion.tigris.org
> For additional commands, e-mail: svn-help_at_subversion.tigris.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-11-22 11:59:56 CET