Eric,
Replying inline would be hard to follow so please accept my responses here:
Continuing lines need to be indented: Thanks for the tip. I wasn't
sure if it mattered since it was wrapped by my email application but
now I know.
Trailing white space: Yeah. That is a good ol' Eclipse feature...I'll
remember this next time.
svn_accept_unknown being redundant: It isn't. I use it for a few
checks throughout the code. If you come into through the command
line, this is handled automatically but if you use the libraries, I
thought it would be safe to check for it. If not, feel free to remove
upon patching.
Negative values: I used the --depth flag as an example. Maybe I
should had done something different.
Match enum values in comment: I agree. I overlooked this when
refactoring as the original suggestion was to use orig, mine and repo
which repo was later changed to base.
Code left over from a previous attempt: Not sure. I revved the
function as requested.
Unrelated whitespace change: Another Eclipse feature.
svn_accept_to_word never used: I followed the --depth flag handling
for this as well. I planned on using but I guess it was never
required.
Thanks for reviewing.
Take care,
Jeremy
On 6/1/07, Eric Gillespie <epg@pretzelnet.org> wrote:
> "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
Received on Fri Jun 1 22:01:45 2007