[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH 03/13] Add debug editor from Subversion trunk

From: Jonathan Nieder <jrnieder_at_gmail.com>
Date: Wed, 7 Jul 2010 12:55:30 -0500

Hi again,

Ramkumar Ramachandra wrote:

> Add the debug editor from subversion/libsvn_delta/debug_editor.c along
> with a header to expose the svn_delta__get_debug_editor function.

The description does not tell what the debug editor is for. Is it
for tracing?

In what follows, I am going to pretend this is all new code, since
for someone unfamiliar to svn like me, that is easier than reviewing
the differences. Upshot: you can probably ignore most of what I say. :)

> +++ b/debug_editor.c
> @@ -0,0 +1,402 @@
> +/* Licensed under a two-clause BSD-style license.
> + * See LICENSE for details.
> + */

Is this true?

> +
> +#include "svn_pools.h"
> +#include "svn_cmdline.h"
> +#include "svn_client.h"
> +#include "svn_ra.h"
> +
> +#include "debug_editor.h"
> +
> +struct edit_baton
> +{
> + const svn_delta_editor_t *wrapped_editor;
> + void *wrapped_edit_baton;
> +
> + int indent_level;
> +
> + svn_stream_t *out;
> +};

This object represents context while describing changes in a revision.
The indent_level gets incremented whenever we move to a subdirectory,
wrapped_editor is a set of callbacks to actually do something with
the changes, wrapped_edit_baton its state cookie, and out a stream
to write the debugging information to.

> +
> +struct dir_baton
> +{
> + void *edit_baton;
> + void *wrapped_dir_baton;
> +};

Context when traversing a directory.

Maybe the debugger’s state should be consistently before or
consistently after the wrapped state. But this is just nitpicking.

Another nitpick: Is the prevailing style in subversion to use void *
for related context objects like edit_baton? If not, I would suggest
using struct edit_baton * to document that that is always its
type; but if so, nothing to see here, please carry on.

> +struct file_baton
> +{
> + void *edit_baton;
> + void *wrapped_file_baton;
> +};

Similar.

[...]
> +static svn_error_t *set_target_revision(void *edit_baton,
> + svn_revnum_t target_revision,
> + apr_pool_t *pool)
> +{
> + struct edit_baton *eb = edit_baton;
> +
> + SVN_ERR(write_indent(eb, pool));
> + SVN_ERR(svn_stream_printf(eb->out, pool, "set_target_revision : %ld\n",
> + target_revision));
> +
> + return eb->wrapped_editor->set_target_revision(eb->wrapped_edit_baton,
> + target_revision,
> + pool);
> +}

This is unfortunately long for how little it does.

C question (I’m just curious): would it be allowed to use

 static svn_Error_t *set_target_revision(struct edit_baton *eb,
        etc

In other words, does C allow a function with struct foo *
argument to be called through a pointer to function with void *
argument?

> +
> +static svn_error_t *open_root(void *edit_baton,
> + svn_revnum_t base_revision,
> + apr_pool_t *pool,
> + void **root_baton)
> +{
> + struct edit_baton *eb = edit_baton;
> + struct dir_baton *dir_baton = apr_palloc(pool, sizeof(*dir_baton));
> +
> + SVN_ERR(write_indent(eb, pool));
> + SVN_ERR(svn_stream_printf(eb->out, pool, "open_root : %ld\n",
> + base_revision));
> + eb->indent_level++;
> +
> + SVN_ERR(eb->wrapped_editor->open_root(eb->wrapped_edit_baton,
> + base_revision,
> + pool,
> + &dir_baton->wrapped_dir_baton));
> +
> + dir_baton->edit_baton = edit_baton;
> +
> + *root_baton = dir_baton;
> +
> + return SVN_NO_ERROR;
> +}

Similar. Maybe:

        static svn_error_t *open_root(...
        {
                struct edit_baton *eb = edit_baton;
                struct dir_baton *dir_baton;

                SVN_ERR(write_indent...
                SVN_ERR(svn_stream_printf...

                dir_baton = apr_palloc(...
                dir_baton->edit_baton = eb;
                SVN_ERR(eb->wrapped_editor->open_root(...

                *root_baton = dir_baton;
                eb->indent_level++;
                return SVN_NO_ERROR;
        }

[...]
> +static svn_error_t *add_directory(const char *path,
[...]
> +static svn_error_t *open_directory(const char *path,
[...]
> +static svn_error_t *add_file(const char *path,
[...]
> +static svn_error_t *open_file(const char *path,

Similar.

> +static svn_error_t *close_file(void *file_baton,
> + const char *text_checksum,
> + apr_pool_t *pool)
> +{
> + struct file_baton *fb = file_baton;
> + struct edit_baton *eb = fb->edit_baton;
> +
> + eb->indent_level--;
> +
> + SVN_ERR(write_indent(eb, pool));
> + SVN_ERR(svn_stream_printf(eb->out, pool, "close_file : %s\n",
> + text_checksum));
> +
> + SVN_ERR(eb->wrapped_editor->close_file(fb->wrapped_file_baton,
> + text_checksum, pool));
> +
> + return SVN_NO_ERROR;
> +}

The context pointers for each file and directory in each revision are
collected in a single pool and not freed, well, ever. I assume that
is not a problem in practice; if it is, one can always start making
subpools later.

> +svn_error_t *svn_delta__get_debug_editor(const svn_delta_editor_t **editor,
> + void **edit_baton,
> + const svn_delta_editor_t *wrapped_editor,
> + void *wrapped_edit_baton,
> + apr_pool_t *pool)
> +{
> + svn_delta_editor_t *tree_editor = svn_delta_default_editor(pool);
> + struct edit_baton *eb = apr_palloc(pool, sizeof(*eb));
> + apr_file_t *errfp;
> + svn_stream_t *out;
> +
> + apr_status_t apr_err = apr_file_open_stderr(&errfp, pool);
> + if (apr_err)
> + return svn_error_wrap_apr(apr_err, "Problem opening stderr");

Is there no function for this that returns svn_error_t *?

[...]
> +++ b/debug_editor.h
> @@ -0,0 +1,10 @@
> +#ifndef DEBUG_EDITOR_H_
> +#define DEBUG_EDITOR_H_
> +
> +svn_error_t *svn_delta__get_debug_editor(const svn_delta_editor_t **editor,
> + void **edit_baton,
> + const svn_delta_editor_t *wrapped_editor,
> + void *wrapped_edit_baton,
> + apr_pool_t *pool);

Usable from other code. Caller provides the pool. No example user
yet.

Well, it looks like it should work. :)
Received on 2010-07-07 19:57:53 CEST

This is an archived mail posted to the Subversion Dev mailing list.