Re: [PATCH 04/13] Add skeleton dump editor

[Ramkumar Ramachandra <artagnon@gmail.com>]

Hi,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> 
> > Add a dump editor and write out skeleton callback functions according
> > to the API documentation of svn_delta_editor_t. Also expose
> > get_dump_editor through a header.
> 
> This commit message tells me nothing... Maybe:
> 
> 	Add a no-op svn_editor.  The function to retrieve it is called
> 	get_dump_editor because it is planned to tweak it to write a
> 	dumpfile.  But for now it is more useful when used with the
> 	debug_editor, to get a list of editor operations printed to
> 	stderr.

Fixed.

> It could make sense to squash this with patch 5 as a demo.

Hm, I'm not too competent with handling lots of conflicts during an
interactive rebase: I'll try this out in a new branch and let you know
how it went.

> > --- /dev/null
> > +++ b/dump_editor.c
> > @@ -0,0 +1,143 @@
> > +/* Licensed under a two-clause BSD-style license.
> > + * See LICENSE for details.
> > + */
> > +
> > +#include "svn_pools.h"
> > +#include "svn_error.h"
> > +#include "svn_iter.h"
> > +#include "svn_repos.h"
> > +#include "svn_string.h"
> > +#include "svn_dirent_uri.h"
> > +#include "svn_path.h"
> > +#include "svn_time.h"
> > +#include "svn_checksum.h"
> > +#include "svn_props.h"
> 
> Are these all needed?

I figured that adding some implies adding some others. Fixed.

> [...]
> > +svn_error_t *open_root(void *edit_baton,
> > +                       svn_revnum_t base_revision,
> > +                       apr_pool_t *pool,
> > +                       void **root_baton)
> > +{
> > +	return SVN_NO_ERROR;
> > +}
> 
> Might make sense to use
> 
> 	*root_baton = NULL;
> 
> for easier debugging.
> 
> [...]
> > +svn_error_t *add_directory(const char *path,
> [...]
> > +svn_error_t *open_directory(const char *path,
> [...]
> > +svn_error_t *add_file(const char *path,
> [...]
> > +svn_error_t *open_file(const char *path,
> [...]
> > +svn_error_t *apply_textdelta(void *file_baton, const char *base_checksum,
> 
> Likewise.

Fixed. Excellent suggestion.

> [...]
> > +++ b/dumpr_util.h
> > @@ -0,0 +1,29 @@
> > +#ifndef DUMPR_UTIL_H_
> > +#define DUMPR_UTIL_H_
> > +
> > +struct edit_baton {
> 
> A more specific name might be nice (or might not, depending on the
> prevailing style in svn; I ought to check but I am too lazy).
> 
> > +	/* The stream to dump to: stdout */
> > +	svn_stream_t *stream;
> > +
> > +	/* pool is for per-edit-session allocations */
> > +	apr_pool_t *pool;
> 
> Unused; probably should delay adding these until there is a user to
> explain them.
> 
> > +
> > +	svn_revnum_t current_rev;
> 
> Used.
> 
> > +	
> > +	/* Store the properties that changed */
> > +	apr_hash_t *properties;
> > +	apr_hash_t *del_properties; /* Value is always 0x1 */
> > +	svn_stringbuf_t *propstring;
> > +
> > +	/* Path of changed file */
> > +	const char *changed_path;
> > +
> > +	/* Was a copy command issued? */
> > +	svn_boolean_t is_copy;
> > +
> > +	/* Temporary file to write delta to along with its checksum */
> > +	char *temp_filepath;
> > +	svn_checksum_t *checksum;
> 
> All unused.
> 
> > +};

Here's the diff after your review. It took me quite a long time to get
the interactive rebase right.

Add skeleton dump editor

Add a no-op svn_editor and expose the function get_dump_editor; this
dump editor will later be filled in to write the dumpfile. Currently,
it is more useful to wrap it in the debug_editor to get a list of
editor operations printed to stderr.


diff --git a/dump_editor.c b/dump_editor.c
index 2fdf93c..e1f3fca 100644
--- a/dump_editor.c
+++ b/dump_editor.c
@@ -3,14 +3,8 @@
  */
 
 #include "svn_pools.h"
-#include "svn_error.h"
-#include "svn_iter.h"
 #include "svn_repos.h"
-#include "svn_string.h"
-#include "svn_dirent_uri.h"
 #include "svn_path.h"
-#include "svn_time.h"
-#include "svn_checksum.h"
 #include "svn_props.h"
 
 #include "dumpr_util.h"
@@ -20,6 +14,7 @@ svn_error_t *open_root(void *edit_baton,
                        apr_pool_t *pool,
                        void **root_baton)
 {
+	*root_baton = NULL;
 	return SVN_NO_ERROR;
 }
 
@@ -38,6 +33,7 @@ svn_error_t *add_directory(const char *path,
                            apr_pool_t *pool,
                            void **child_baton)
 {
+	*child_baton = NULL;
 	return SVN_NO_ERROR;
 }
 
@@ -47,6 +43,7 @@ svn_error_t *open_directory(const char *path,
                             apr_pool_t *pool,
                             void **child_baton)
 {
+	*child_baton = NULL;
 	return SVN_NO_ERROR;
 }
 
@@ -63,6 +60,7 @@ svn_error_t *add_file(const char *path,
                       apr_pool_t *pool,
                       void **file_baton)
 {
+	*file_baton = NULL;
 	return SVN_NO_ERROR;
 }
 
@@ -72,6 +70,7 @@ svn_error_t *open_file(const char *path,
                        apr_pool_t *pool,
                        void **file_baton)
 {
+	*file_baton = NULL;
 	return SVN_NO_ERROR;
 }
 
@@ -96,6 +95,8 @@ svn_error_t *apply_textdelta(void *file_baton, const char *base_checksum,
                              svn_txdelta_window_handler_t *handler,
                              void **handler_baton)
 {
+	*handler = svn_delta_noop_window_handler;
+	*handler_baton = NULL;
 	return SVN_NO_ERROR;
 }

diff --git a/dumpr_util.h b/dumpr_util.h
index d206c19..2906543 100644
--- a/dumpr_util.h
+++ b/dumpr_util.h
@@ -2,28 +2,8 @@
 #define DUMPR_UTIL_H_
 
 struct edit_baton {
-	/* The stream to dump to: stdout */
 	svn_stream_t *stream;
-
-	/* pool is for per-edit-session allocations */
-	apr_pool_t *pool;
-
 	svn_revnum_t current_rev;
-	
-	/* Store the properties that changed */
-	apr_hash_t *properties;
-	apr_hash_t *del_properties; /* Value is always 0x1 */
-	svn_stringbuf_t *propstring;
-
-	/* Path of changed file */
-	const char *changed_path;
-
-	/* Was a copy command issued? */
-	svn_boolean_t is_copy;
-
-	/* Temporary file to write delta to along with its checksum */
-	char *temp_filepath;
-	svn_checksum_t *checksum;
 };
 
 #endif