Re: [PATCH 03/13] Add debug editor from Subversion trunk
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Jul 7, 2010, 17:55 UTC
- Message-ID
- <20100707175530.GA2480@burratino>
- In-Reply-To
- <1278461693-3828-4-git-send-email-artagnon@gmail.com>
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. :)
Show 5 quoted lines
> +++ b/debug_editor.c > @@ -0,0 +1,402 @@ > +/* Licensed under a two-clause BSD-style license. > + * See LICENSE for details. > + */
Is this true?
Show 17 quoted lines
> +
> +#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.
Show 6 quoted lines
> +
> +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.
Show 5 quoted lines
> +struct file_baton
> +{
> + void *edit_baton;
> + void *wrapped_file_baton;
> +};Similar.
[...]
Show 14 quoted lines
> +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?
Show 25 quoted lines
> +
> +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.
Show 18 quoted lines
> +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.
Show 14 quoted lines
> +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 *?
[...]
Show 10 quoted lines
> +++ 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. :)