On Tue, 2008-11-04 at 19:52 -0800, Greg Stein wrote:
> On Tue, Nov 4, 2008 at 8:10 AM, <julianfoad_at_tigris.org> wrote:
> >...
> > +++ trunk/subversion/libsvn_wc/tree_conflicts.c Tue Nov 4 08:10:00 2008 (r34043)
> > @@ -469,6 +469,24 @@ svn_wc__tree_conflict_exists(apr_array_h
> > }
> >
> > svn_error_t *
> > +svn_wc__del_tree_conflict_data(const char *victim_path,
> > + svn_wc_adm_access_t *adm_access,
> > + apr_pool_t *pool)
> > +{
> > + svn_stringbuf_t *log_accum = svn_stringbuf_create("", pool);
> > +
> > + SVN_ERR(svn_wc__loggy_del_tree_conflict_data(log_accum,
> > + victim_path,
> > + adm_access,
> > + pool));
>
> The function signature doesn't follow the regular pattern for loggy
> functions. The first param is a svn_string_t **, and it will create
> the stringbuf if it doesn't already exist (eg is NULL).
Thanks. r34067.
> >...
> > +static void
> > +array_remove_unordered(apr_array_header_t *array, int remove_index)
> > +{
> > + void *last_element = apr_array_pop(array);
> > +
> > + if (remove_index < array->nelts)
>
> This test will always be true.
Not if you're removing the last element! 'nelts' has been decremented by
the 'pop'. I guess this deserves a comment! r34066. Thanks. (I found it
needed more documentation improvement than just that.)
Actually it seems silly using arrays in this file; hashes would be more
appropriate I think, and instead of such a "remove" function we would
just write "apr_hash_set(..., NULL)".
> >...
> > +++ trunk/subversion/libsvn_wc/tree_conflicts.h Tue Nov 4 08:10:00 2008 (r34043)
> > @@ -153,6 +153,26 @@ svn_wc__loggy_add_tree_conflict_data(
> > svn_wc_adm_access_t *adm_access,
> > apr_pool_t *pool);
> >
> > +/* Like svn_wc__del_tree_conflict_data(), but append to the log accumulator
> > + * LOG_ACCUM a command to rewrite the entry field, and do not flush the log.
> > + * This function is meant to be used in the working copy library where
> > + * log accumulators are usually readily available.
> > + */
> > +svn_error_t *
> > +svn_wc__loggy_del_tree_conflict_data(svn_stringbuf_t *log_accum,
> > + const char *victim_path,
> > + svn_wc_adm_access_t *adm_access,
> > + apr_pool_t *pool);
>
> Again: this signature doesn't match the others.
>
> (that said... I prefer this form, but it isn't the time to change
> this; it'll mostly change in 1.7 regardless)
Made consistent with the current "**log_accum" form in r34067.
Thanks for the review.
So, does the loggy/non-loggy usage look OK?
- Julian
---------------------------------------------------------------------
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-05 13:23:19 CET