You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2008/11/05 03:52:36 UTC
Re: svn commit: r34043 - trunk/subversion/libsvn_wc
On Tue, Nov 4, 2008 at 8:10 AM, <ju...@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).
>...
> +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.
>...
> +++ 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)
Cheers,
-g
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r34043 - trunk/subversion/libsvn_wc
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Nov 5, 2008 at 4:22 AM, Julian Foad <ju...@btopenworld.com> wrote:
>...
> So, does the loggy/non-loggy usage look OK?
Sure. Would be nice to have a loggy run with larger scope, but that is
possible now with the __loggy_ form. Callers can use that to lump lots
of changes into one log accumulator.
Cheers,
-g
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r34043 - trunk/subversion/libsvn_wc
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2008-11-04 at 19:52 -0800, Greg Stein wrote:
> On Tue, Nov 4, 2008 at 8:10 AM, <ju...@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@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org