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

[Jonathan Nieder <jrnieder@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. :)

> +++ 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. :)