Just a hedzup, Greg et al: Mike is workin' on all of these right now.
-K
Greg Stein <gstein@lyra.org> writes:
> On Wed, Feb 14, 2001 at 09:31:07PM -0000, cmpilato@tigris.org wrote:
> >...
> > (svn_delta_edit_fns_t->add_file, add_directory): Renamed
> > ancestor_revision and ancestor_path to base_revision and base_path
> > respectively.
>
> Euh... I thought these were to be called "copyfrom_path" and
> "copyfrom_revision". That is their true/used semantic.
>
> >...
> > +set_target_revision (void *edit_baton, svn_revnum_t target_revision)
> > {
> > struct edit_baton *eb = edit_baton;
> > svn_error_t *err;
> > +
> > + if (eb->editor_1->set_target_revision)
> > + {
> > + err = (* (eb->editor_1->set_target_revision)) (eb->edit_baton_1,
> > + target_revision);
> > + if (err)
> > + return err;
> > + }
> > +
> > + if (eb->editor_2->set_target_revision)
> > + {
> > + err = (* (eb->editor_2->set_target_revision)) (eb->edit_baton_2,
> > + target_revision);
> > + if (err)
> > + return err;
> > + }
>
> General comment across the board: since we now create a "default" editor and
> then fill in functions, that default should fill in no-op functions. Thus,
> we would never need to check for NULL function values.
>
> And note that we weren't consistent with NULL-checking anyway.
>
> Anyway: we should stop checking for NULL, assume all editor functions are
> filled in, and just always call them.
>
> >...
> > --- xml_output.c 2001/02/09 23:14:19 1.31
> > +++ xml_output.c 2001/02/14 21:31:05 1.32
> > @@ -63,6 +63,7 @@
> > elem_tree_delta, elem_file, or
> > elem_file_prop_delta. */
> > struct file_baton *curfile;
> > + svn_revnum_t target_revision;
> > apr_pool_t *pool;
> > int txdelta_id_counter;
> > };
> > @@ -229,7 +230,7 @@
> > static svn_error_t *
> > output_addreplace (struct edit_baton *eb, enum elemtype addreplace,
> > enum elemtype dirfile, svn_string_t *name,
> > - svn_string_t *ancestor_path, svn_revnum_t ancestor_revision)
> > + svn_string_t *base_path, svn_revnum_t base_revision)
> > {
> > svn_string_t *str;
> > apr_pool_t *pool = svn_pool_create (eb->pool);
> > @@ -240,22 +241,25 @@
> > SVN_DELTA__XML_TAG_ADD : SVN_DELTA__XML_TAG_REPLACE;
> > const char *innertag = (dirfile == elem_dir) ?
> > SVN_DELTA__XML_TAG_DIR : SVN_DELTA__XML_TAG_FILE;
> > + char buf[128];
> >
> > str = get_to_elem (eb, elem_tree_delta, pool);
> > svn_xml_make_open_tag (&str, pool, svn_xml_normal, outertag,
> > SVN_DELTA__XML_ATTR_NAME, name, NULL);
> >
> > att = apr_hash_make (pool);
> > - if (ancestor_path != NULL)
> > - {
> > - char buf[128];
> > - apr_hash_set (att, SVN_DELTA__XML_ATTR_ANCESTOR,
> > - strlen(SVN_DELTA__XML_ATTR_ANCESTOR), ancestor_path);
> > - sprintf (buf, "%lu", (unsigned long) ancestor_revision);
> > - apr_hash_set (att, SVN_DELTA__XML_ATTR_VER,
> > - strlen(SVN_DELTA__XML_ATTR_VER),
> > - svn_string_create (buf, pool));
> > - }
> > + if (base_path != NULL)
> > + {
> > + apr_hash_set (att, SVN_DELTA__XML_ATTR_BASE_PATH,
> > + strlen(SVN_DELTA__XML_ATTR_BASE_PATH), base_path);
> > + }
> > + if (SVN_IS_VALID_REVNUM(base_revision))
> > + {
> > + sprintf (buf, "%lu", (unsigned long) base_revision);
> > + apr_hash_set (att, SVN_DELTA__XML_ATTR_BASE_REV,
> > + strlen(SVN_DELTA__XML_ATTR_BASE_REV),
> > + svn_string_create (buf, pool));
> > + }
>
> I realize that "%lu" will always be less than 128, but just seeing a
> fixed-size buffer like that gives me pause. It will be easier on us if that
> svn_string_create(buf, pool) is simply switched to:
>
> svn_string_createf(pool, "%lu", (unsigned long) base_revision)
>
> Then toss the buf.
>
> >...
> > +static svn_error_t *
> > +replace_root (void *edit_baton, svn_revnum_t base_revision, void **dir_baton)
> > +{
> > + struct edit_baton *eb = (struct edit_baton *) edit_baton;
> > apr_pool_t *pool = svn_pool_create (eb->pool);
> > svn_string_t *str = NULL;
> > apr_size_t len;
> > + apr_hash_t *att;
> > svn_error_t *err;
> > + char buf[128];
> >
> > svn_xml_make_header (&str, pool);
> > - svn_xml_make_open_tag (&str, pool, svn_xml_normal,
> > - SVN_DELTA__XML_TAG_DELTA_PKG, NULL);
> > +
> > + att = apr_hash_make (pool);
> > + if (SVN_IS_VALID_REVNUM(base_revision))
> > + {
> > + sprintf (buf, "%lu", (unsigned long) base_revision);
> > + apr_hash_set (att, SVN_DELTA__XML_ATTR_BASE_REV,
> > + strlen(SVN_DELTA__XML_ATTR_BASE_REV),
> > + svn_string_create (buf, pool));
> > + }
> > + if (SVN_IS_VALID_REVNUM(eb->target_revision))
> > + {
> > + sprintf (buf, "%lu", (unsigned long) eb->target_revision);
> > + apr_hash_set (att, SVN_DELTA__XML_ATTR_TARGET_REV,
> > + strlen(SVN_DELTA__XML_ATTR_TARGET_REV),
> > + svn_string_create (buf, pool));
>
> Same here.
>
> >...
> > static const svn_delta_edit_fns_t tree_editor =
> > {
> > - begin_edit,
> > + set_target_revision,
> > + replace_root,
> > delete_entry,
> > add_directory,
> > replace_directory,
>
> Whoops. Looks like this isn't using the default_editor thingy.
>
> >...
>
> One last thing: on the replace_dir/file, a lot of the parameters were left
> as "ancestor_revision" ... should those be changed, but you just didn't feel
> like doing that work? If so, then could a task be recorded in STACK so that
> we don't forget to do it?
>
> Cheers,
> -g
>
> --
> Greg Stein, http://www.lyra.org/
Received on Sat Oct 21 14:36:22 2006