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