Re: [PATCH] Add option to resolve conflicts by selecting a specific file (Issue 2784)

"Jeremy Whitlock" <jcscoobyrs@gmail.com> writes:

> Hi All,
> Attached is the patch used to resolve issue 2784. Here is a synopsis:
>
> This patch adds the "--accept" or "-a" flag support to the Subversion
> command line. Right now, only resolved subcommand is using it. This
> new flag will also be used by the following subcommands:

I really don't think we should start out with a short option, if
we need one at all.

> [[[
> Add option to resolve conflicts by selecting a specific file by adding
> support to the svn executable to handle the "--accept" flag. The
> "resovled" subcommand is the only subcommand using this new flag as
> part of this patch. (Issue 2784)
>
> * subversion/include/svn_client.h
> (svn_client_resolved): Revved to svn_client_resolved2() to handle
> the svn_accept_t enum.

Continuing lines should be indented with the rest.

Now, for the patch. Looks good! I don't think the Subversion
project cares about random trailing white space, since the code
is *full* of it. But it drives me nuts. Your patch is full of
it. Vim folks can get spacehi.vim, emacs folks can (setq
show-trailing-whitespace t).

> Index: subversion/include/svn_types.h
> ===================================================================
> --- subversion/include/svn_types.h (revision 25247)
> +++ subversion/include/svn_types.h (working copy)
> @@ -199,6 +199,48 @@
> svn_recursive
> };
>
> +/** The concept of automatic conflict resolution.
> + *
> + * @since New in 1.5
> + */
> + typedef enum
> + {
> + /* The order of the accept options is not important */
> +
> + /* Accept option unknown or ignored */
> + svn_accept_unknown = -2,

It looks like this is redundant. Perhaps rename svn_accept_empty
to svn_accept_merged (--accept=merged == leave conflicted, the
default) and use that instead?

> + /* Resolve the conflict with the pre-update base file */
> + svn_accept_orig = -1,

And why are these negative? Why specify values at all?

> + /* Resolve the conflict the usual way */
> + svn_accept_empty = 0,
> +
> + /* Resolve the conflict with the pre-update working copy file */
> + svn_accept_mine = 1,
> +
> + /* Resolve the conflict with the during-update repository file, new base */
> + svn_accept_base = 2,
> +
> + } svn_accept_t;
> +
> + /** Return a constant string expressing @a accept as an English word,
> + * e.g., "orig", "mine", or "repo". The string is not localized.

I think these words should match the enum values. I don't like
the words you've chosen, either. "orig" and "repo" are both from
the repository. I believe the standard terms are "orig",
"theirs", and "mine". At least, that's what i see most places,
including Perforce.

Actually, it looks like Perforce says "base". I think "orig" is
more common, but maybe i'm imagining things...

> Index: subversion/include/svn_wc.h
> ===================================================================
> --- subversion/include/svn_wc.h (revision 25247)
> +++ subversion/include/svn_wc.h (working copy)
> @@ -1583,7 +1583,8 @@
> * be a file or dir. Call callbacks in @a walk_callbacks, passing
> * @a walk_baton to each. Use @a pool for looping, recursion, and to
> * allocate all entries returned. @a adm_access must be an access baton
> - * for @a path.
> + * for @a path. The @a accept argument is used passed to @a walk_callbacks
> + * for automatic conflict resolution support.
> *
> * If @a cancel_func is non-null, call it with @a cancel_baton to determine
> * if the client has cancelled the operation.
> @@ -1603,6 +1604,23 @@
> *
> * @since New in 1.5.
> */
> +svn_error_t *svn_wc_walk_entries4(const char *path,
> + svn_wc_adm_access_t *adm_access,
> + const svn_wc_entry_callbacks2_t
> + *walk_callbacks,
> + void *walk_baton,
> + svn_boolean_t show_hidden,
> + svn_accept_t accept,
> + svn_cancel_func_t cancel_func,
> + void *cancel_baton,
> + apr_pool_t *pool);
> +
> +/**
> + * Similar to svn_wc_walk_entries4(), but without automatic conflict
> + * resolution support.
> + *
> + * @deprecated Provided for backward compatibility with the 1.4 API.
> + */
> svn_error_t *svn_wc_walk_entries3(const char *path,
> svn_wc_adm_access_t *adm_access,
> const svn_wc_entry_callbacks2_t

I guess this is left-over from a previous attempt?

> Index: subversion/libsvn_wc/adm_ops.c
> ===================================================================
> --- subversion/libsvn_wc/adm_ops.c (revision 25247)
> +++ subversion/libsvn_wc/adm_ops.c (working copy)

> @@ -2687,7 +2733,6 @@
> SVN_ERR(svn_wc_walk_entries2(path, adm_access,
> &resolve_walk_callbacks, baton,
> FALSE, cancel_func, cancel_baton, pool));
> -
> }
>
> return SVN_NO_ERROR;

unrelated whitespace change

> Index: subversion/libsvn_subr/kitchensink.c
> ===================================================================
> --- subversion/libsvn_subr/kitchensink.c (revision 25247)
> +++ subversion/libsvn_subr/kitchensink.c (working copy)
> @@ -32,7 +32,44 @@
> return uuid_str;
> }
>
> +const char *
> +svn_accept_to_word(svn_accept_t accept)

This is never used.

-- 
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org