Re: CVS update: subversion/subversion/tests/libsvn_delta xml-output-test.c

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/