Eric,
Thanks for the feedback. I'll resubmit another patch. I do have
one thing to discuss:
What is the real purpose of svn_accept_invalid? I can see it being
returned from svn_accept_from_word when an invalid accept word is
passed as an argument but beyond that, how else do you see it being
used? I also think that the command line parsing should handle this
as an error when you pass an invalid value as the accept flag. This
is how I do it now but you suggest otherwise. Can you explain?
The other things you mentioned are valid. Sometimes it is easy to
look over small things like docstrings when trying to get code
working. Please bear with me with this patch as I'm learning the
Subversion coding standards and API as I go along.
Take care,
Jeremy
On 6/7/07, Eric Gillespie <epg@pretzelnet.org> wrote:
> "Jeremy Whitlock" <jcscoobyrs@gmail.com> writes:
>
> > [[[
> > 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
> ^^^^^^^^
>
> resolved
>
> > Index: subversion/include/svn_types.h
> > ===================================================================
> > --- subversion/include/svn_types.h (revision 25291)
> > +++ subversion/include/svn_types.h (working copy)
> > @@ -199,6 +199,34 @@
> > svn_recursive
> > };
> >
> > +/** The concept of automatic conflict resolution.
> > + *
> > + * @since New in 1.5
> > + */
> > +typedef enum
> > +{
>
> I think you want 'svn_accept_invalid = -1' here; see below.
>
> > + /* Resolve the conflict as usual */
> > + svn_accept_default,
> > +
> > + /* Resolve the conflict with the pre-update base file */
> > + svn_accept_orig,
> > +
> > + /* Resolve the conflict with the pre-update working copy file */
> > + svn_accept_mine,
> > +
> > + /* Resolve the conflict with the during-update repository file, new base */
> > + svn_accept_base,
> > +
> > +} svn_accept_t;
>
> Unfortunately, i don't think we've finished painting this
> particular bikeshed yet ;->. I finally went and got myself a
> conflict so i could see if we could take name inspiration from
> there. So, how about svn_accept_{old,new,work} with strings
> "left", "new", and "working"?
>
> > Index: subversion/include/svn_wc.h
> > ===================================================================
> > --- subversion/include/svn_wc.h (revision 25291)
> > +++ subversion/include/svn_wc.h (working copy)
> > @@ -2474,6 +2474,23 @@
> > void *cancel_baton,
> > apr_pool_t *pool);
> >
> > +/**
> > + * Just like "" but with the ability to pass an svn_accept_t argument
> > + * for automatical conflict resolution handling.
> > + *
> > + * @since New in 1.5
>
> You have this backwards. Move the svn_wc_resolved_conflict2
> docstring here and document the new parameter. Then add a
> "similar to"/deprecated comment for the old one; see
> svn_wc_adm_open3 for example. svn_client_resolved2, too.
>
> > Index: subversion/libsvn_wc/adm_ops.c
>
> > @@ -2519,6 +2524,25 @@
> > svn_boolean_t was_present, need_feedback = FALSE;
> > apr_uint64_t modify_flags = 0;
> > svn_wc_entry_t *entry = svn_wc_entry_dup(orig_entry, pool);
> > +
> > + /* Handle automatic conflict resolution before the temporary files are
> > + * deleted. */
> > + if (accept != svn_accept_default)
> > + {
> > + const char *base_name;
> > +
> > + if (accept == svn_accept_orig) {
> > + base_name = entry->conflict_old;
> > + } else if (accept == svn_accept_mine) {
> > + base_name = entry->conflict_wrk;
> > + } else if (accept == svn_accept_base) {
> > + base_name = entry->conflict_new;
> > + } /* Should never be anything else thanks to the svn_accept_from_word() */
>
> Not all callers use svn_accept_from_word; maybe just say "Ignore
> invalid values". If someone passes some random value instead of
> one of the svn_accept_ values, that's his problem.
>
> > @@ -2594,6 +2618,8 @@
> > svn_boolean_t resolve_text;
> > /* TRUE if property conflicts are to be resolved. */
> > svn_boolean_t resolve_props;
> > + /* Placeholder for the type of automatic conflict resolution to perform */
> > + svn_accept_t accept;
>
> Drop "Placeholder for the".
>
> > Index: subversion/libsvn_subr/kitchensink.c
> > ===================================================================
> > --- subversion/libsvn_subr/kitchensink.c (revision 25291)
> > +++ subversion/libsvn_subr/kitchensink.c (working copy)
> > @@ -32,6 +32,18 @@
> > return uuid_str;
> > }
> >
> > +svn_accept_t
> > +svn_accept_from_word(const char *word)
> > +{
> > + if (strcmp(word, "orig") == 0)
> > + return svn_accept_orig;
> > + if (strcmp(word, "mine") == 0)
> > + return svn_accept_mine;
> > + if (strcmp(word, "base") == 0)
> > + return svn_accept_base;
> > + /* Return default, which means no automatic conflict resolution */
> > + return svn_accept_default;
>
> This comment is wrong, as the only svn_accept_from_word caller
> treats svn_accept_default as an error.
>
> > Index: subversion/svn/main.c
>
> > @@ -1381,6 +1385,21 @@
> > case 'g':
> > opt_state.use_merge_history = TRUE;
> > break;
> > + case svn_cl__accept_opt:
> > + opt_state.accept = svn_accept_from_word(opt_arg);
> > +
> > + /* We need to make sure that the value passed to the accept flag
> > + * was one of the three expected. Since default is what gets
> > + * set when one of the three expected are not passed, checking for
> > + * this as part of the command line parsing makes sense. */
> > + if (opt_state.accept == svn_accept_default)
> > + {
> > + return svn_cmdline_handle_exit_error
> > + (svn_error_createf(SVN_ERR_CL_ARG_PARSING_ERROR, NULL,
> > + _("'%s' is not a valid accept value; try "
> > + "'orig', 'mine', or 'base'"),
> > + opt_arg), pool, "svn: ");
> > + }
>
> You've conflated default behavior with malformed input. I
> preferred how you were using svn_accept_unknown for this earlier
> (though i think svn_accept_invalid is a better name).
>
> --
> 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 8 05:57:09 2007