[GSoC update] git-remote-svn: Week 10

[GSoC update] git-remote-svn: Week 10

[Ramkumar Ramachandra]

Hi,

After an extended weekend of hacking, I'm happy to report that I've
finally managed to finish the SVN client (now renamed svndumpr) and
validate it against 10,000 revisions of the ASF repository. It doesn't
look like I can go much further with the validation until the
dumpfilev3 parser is completed due to hardware limitations. However,
Avar has been kind enough to lend me access to a more powerful machine
on which I intend to run more tests over the next few days.

The last patch is a nice validation script; I would request everyone
who's interested in the project to test it against their repositories
so we can weed out any remaining bugs. The code is also available on
my GitHub [1]. Also, please review the series thoroughly and don't
hesitate to ask trivial questions: I'm new to developing with libsvn
myself.

Please note that it has been built and tested only against the
Subversion trunk: for Subversion 1.6, you can try using my
ra-svn-1.6. Also, there seems to be some unresolved issue on 64-bit
systems. We're working on fixing this.

Final note: I'll begin preparing to merge this into the Subversion
trunk soon.

Thanks for reading.

[1]: ra-svn and ra-svn-rollout branches of
github.com/artagnon/svn-dump-fast-export

-- Ram

Ramkumar Ramachandra (13):
  Add LICENSE
  Add skeleton SVN client and Makefile
  Add debug editor from Subversion trunk
  Add skeleton dump editor
  Drive the debug editor
  Dump the revprops at the start of every revision
  Implement open_root and close_edit
  Implement dump_node
  Implement directory-related functions
  Implement file-related functions
  Implement apply_textdelta
  Implement close_file
  Add a validation script

[PATCH 01/13] Add LICENSE

[Ramkumar Ramachandra]

License the project under a two-clause BSD-style license. A dual
license will be required later when attempting to merge into
Subversion.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 LICENSE |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)
 create mode 100644 LICENSE

diff --git a/LICENSE b/LICENSE
new file mode 100644
index 0000000..4367b7c
--- /dev/null
+++ b/LICENSE
@@ -0,0 +1,26 @@
+Copyright (C) 2010 Ramkumar Ramachandra
+All rights reserved.
+
+Redistribution and use in source and binary forms, with or without
+modification, are permitted provided that the following conditions
+are met:
+1. Redistributions of source code must retain the above copyright
+   notice(s), this list of conditions and the following disclaimer
+   unmodified other than the allowable addition of one or more
+   copyright notices.
+2. Redistributions in binary form must reproduce the above copyright
+   notice(s), this list of conditions and the following disclaimer in
+   the documentation and/or other materials provided with the
+   distribution.
+
+THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDER(S) ``AS IS'' AND ANY
+EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE COPYRIGHT HOLDER(S) BE
+LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY,
+WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE
+OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE,
+EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-- 
1.7.1

[PATCH 02/13] Add skeleton SVN client and Makefile

[Ramkumar Ramachandra]

Add a basic SVN command-line client along with a Makefile that does
just enough to establish a connection with the ASF subversion server;
it initializes a memory pool, sees that configuration files are in
order, builds up a context object, sets up an authentication baton,
and finally opens a session to the subversion server.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Makefile   |    8 +++++++
 svndumpr.c |   68 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 0 deletions(-)
 create mode 100644 Makefile
 create mode 100644 svndumpr.c

diff --git a/Makefile b/Makefile
new file mode 100644
index 0000000..a6022f7
--- /dev/null
+++ b/Makefile
@@ -0,0 +1,8 @@
+svndumpr: *.c *.h
+	$(CC) -Wall -Werror -DAPR_POOL_DEBUG -ggdb3 -O0 -o $@ svndumpr.c -lsvn_client-1 -I. -I/usr/include/subversion-1 -I/usr/include/apr-1.0
+
+svndumpr_bench: *.c *.h
+	$(CC) -O2 -o $@ svndumpr.c -lsvn_client-1 -I. -I/usr/include/subversion-1 -I/usr/include/apr-1.0
+
+clean:
+	$(RM) svndumpr svndumpr_bench
diff --git a/svndumpr.c b/svndumpr.c
new file mode 100644
index 0000000..737c4aa
--- /dev/null
+++ b/svndumpr.c
@@ -0,0 +1,68 @@
+/* Licensed under a two-clause BSD-style license.
+ * See LICENSE for details.
+ */
+
+#include "svn_pools.h"
+#include "svn_cmdline.h"
+#include "svn_client.h"
+#include "svn_ra.h"
+#include "svn_repos.h"
+
+static apr_pool_t *pool = NULL;
+static svn_client_ctx_t *ctx = NULL;
+static svn_ra_session_t *session = NULL;
+
+svn_error_t *populate_context()
+{
+	const char *http_library;
+	
+	SVN_ERR(svn_config_get_config(&(ctx->config), NULL, pool));
+	
+	http_library = getenv("SVN_HTTP_LIBRARY");
+	if (http_library)
+		svn_config_set(apr_hash_get(ctx->config, "servers", APR_HASH_KEY_STRING),
+		               "global", "http-library", http_library);
+	return SVN_NO_ERROR;
+}
+
+svn_error_t *open_connection(const char *url)
+{
+	SVN_ERR(svn_config_ensure (NULL, pool));
+	SVN_ERR(svn_client_create_context (&ctx, pool));
+	SVN_ERR(svn_ra_initialize(pool));
+
+#if defined(WIN32) || defined(__CYGWIN__)
+	if (getenv("SVN_ASP_DOT_NET_HACK"))
+		SVN_ERR(svn_wc_set_adm_dir("_svn", pool));
+#endif
+
+	SVN_ERR(populate_context());
+	SVN_ERR(svn_cmdline_create_auth_baton(&(ctx->auth_baton), TRUE,
+					      NULL, NULL, NULL, FALSE,
+					      FALSE, NULL, NULL, NULL,
+					      pool));
+	SVN_ERR(svn_client_open_ra_session(&session, url, ctx, pool));
+	return SVN_NO_ERROR;
+}
+
+svn_error_t *replay_range(svn_revnum_t start_revision, svn_revnum_t end_revision)
+{
+	return SVN_NO_ERROR;
+}
+
+int main()
+{
+	const char url[] = "http://svn.apache.org/repos/asf";
+	svn_revnum_t start_revision = 1, end_revision = 500;
+	if (svn_cmdline_init ("svndumpr", stderr) != EXIT_SUCCESS)
+		return 1;
+
+	pool = svn_pool_create(NULL);
+
+	SVN_INT_ERR(open_connection(url));
+	SVN_INT_ERR(replay_range(start_revision, end_revision));
+
+	svn_pool_destroy(pool);
+	
+	return 0;
+}
-- 
1.7.1

Re: [PATCH 02/13] Add skeleton SVN client and Makefile

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Add a basic SVN command-line client along with a Makefile that does
> just enough to establish a connection with the ASF subversion server;

Thanks for splitting this out.

Let’s see what’s needed to set up a connection:

> +++ b/Makefile
> @@ -0,0 +1,8 @@
> +svndumpr: *.c *.h
> +	$(CC) -Wall -Werror -DAPR_POOL_DEBUG -ggdb3 -O0 -o $@ svndumpr.c -lsvn_client-1 -I. -I/usr/include/subversion-1 -I/usr/include/apr-1.0

Links against libsvnclient-1.  Good.

I assume the details of the Makefile are not important, since it is
probably going to be revamped in the style of the svn build system
anyway.

> +++ b/svndumpr.c
> @@ -0,0 +1,68 @@
[...]
> +svn_error_t *populate_context()
[...]
> +svn_error_t *open_connection(const char *url)
[...]
> +svn_error_t *replay_range(svn_revnum_t start_revision, svn_revnum_t end_revision)

Why not static?

> +svn_error_t *populate_context()
> +{
> +	const char *http_library;
> +	
> +	SVN_ERR(svn_config_get_config(&(ctx->config), NULL, pool));
> +	
> +	http_library = getenv("SVN_HTTP_LIBRARY");
> +	if (http_library)
> +		svn_config_set(apr_hash_get(ctx->config, "servers", APR_HASH_KEY_STRING),
> +		               "global", "http-library", http_library);

I tried googling for this SVN_HTTP_LIBRARY setting, but no
useful hints.  I take it that this overrides the [global] http-library
setting from ~/.subversion/servers?  Do other commands honor this
environment variable or just svndumpr?

[...]
> +svn_error_t *open_connection(const char *url)
> +{
> +	SVN_ERR(svn_config_ensure (NULL, pool));
> +	SVN_ERR(svn_client_create_context (&ctx, pool));
> +	SVN_ERR(svn_ra_initialize(pool));
> +
> +#if defined(WIN32) || defined(__CYGWIN__)
> +	if (getenv("SVN_ASP_DOT_NET_HACK"))
> +		SVN_ERR(svn_wc_set_adm_dir("_svn", pool));
> +#endif

I guess it’s water under the bridge now (from 5 years ago), but why do
clients have to do this themselves?  It would not be so difficult for
libsvnclient to automatically set the admin dir according to whether
SVN_ASP_DOT_NET_HACK is set or not, or at least to provide a single
function to call and do so.

But that is not the topic for the moment.  I am tempted to suggest
checking SVN_ASP_DOT_NET_HACK unconditionally (i.e., on Unix, too),
just so the function is easier to scan.  Or there could be a separate
set_appropriate_adm_dir function in svndumpr.c:

	#if defined(WIN32) || ...
	static svn_error_t *set_appropriate_adm_dir(...)
	{
		if (getenv...
		...
	}
	#else
	static svn_error_t *set_appropriate_adm_dir(...
	{
		return SVN_NO_ERROR;
	}
	#endif

Feel free to ignore me here. :)

> +
> +	SVN_ERR(populate_context());
> +	SVN_ERR(svn_cmdline_create_auth_baton(&(ctx->auth_baton), TRUE,
> +					      NULL, NULL, NULL, FALSE,
> +					      FALSE, NULL, NULL, NULL,
> +					      pool));

Maybe comments would help, for the boolean arguments.

> +	SVN_ERR(svn_client_open_ra_session(&session, url, ctx, pool));
> +	return SVN_NO_ERROR;
> +}
> +
> +svn_error_t *replay_range(svn_revnum_t start_revision, svn_revnum_t end_revision)
> +{
> +	return SVN_NO_ERROR;
> +}

Might be more self-explanatory without this function, but that
is just nitpicking.

> +
> +int main()
> +{
> +	const char url[] = "http://svn.apache.org/repos/asf";
> +	svn_revnum_t start_revision = 1, end_revision = 500;
> +	if (svn_cmdline_init ("svndumpr", stderr) != EXIT_SUCCESS)
> +		return 1;
> +
> +	pool = svn_pool_create(NULL);
> +
> +	SVN_INT_ERR(open_connection(url));
> +	SVN_INT_ERR(replay_range(start_revision, end_revision));
> +
> +	svn_pool_destroy(pool);
> +	
> +	return 0;
> +}

So: this is an expensive no-op.

Thanks for the pleasant reading.

Jonathan

Re: [PATCH 02/13] Add skeleton SVN client and Makefile

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> 
> > Add a basic SVN command-line client along with a Makefile that does
> > just enough to establish a connection with the ASF subversion server;
> 
> Thanks for splitting this out.

Thanks for getting the review process started :)

> Let’s see what’s needed to set up a connection:
> 
> > +++ b/Makefile
> > @@ -0,0 +1,8 @@
> > +svndumpr: *.c *.h
> > +	$(CC) -Wall -Werror -DAPR_POOL_DEBUG -ggdb3 -O0 -o $@ svndumpr.c -lsvn_client-1 -I. -I/usr/include/subversion-1 -I/usr/include/apr-1.0
> 
> Links against libsvnclient-1.  Good.
> 
> I assume the details of the Makefile are not important, since it is
> probably going to be revamped in the style of the svn build system
> anyway.

Right.

> > +++ b/svndumpr.c
> > @@ -0,0 +1,68 @@
> [...]
> > +svn_error_t *populate_context()
> [...]
> > +svn_error_t *open_connection(const char *url)
> [...]
> > +svn_error_t *replay_range(svn_revnum_t start_revision, svn_revnum_t end_revision)
> 
> Why not static?

Changed (see end of email).

> > +svn_error_t *populate_context()
> > +{
> > +	const char *http_library;
> > +	
> > +	SVN_ERR(svn_config_get_config(&(ctx->config), NULL, pool));
> > +	
> > +	http_library = getenv("SVN_HTTP_LIBRARY");
> > +	if (http_library)
> > +		svn_config_set(apr_hash_get(ctx->config, "servers", APR_HASH_KEY_STRING),
> > +		               "global", "http-library", http_library);
> 
> I tried googling for this SVN_HTTP_LIBRARY setting, but no
> useful hints.  I take it that this overrides the [global] http-library
> setting from ~/.subversion/servers?  Do other commands honor this
> environment variable or just svndumpr?

I originally had this to switch between neon and serf libraries. It
seems that this is no more necessary.

> [...]
> > +svn_error_t *open_connection(const char *url)
> > +{
> > +	SVN_ERR(svn_config_ensure (NULL, pool));
> > +	SVN_ERR(svn_client_create_context (&ctx, pool));
> > +	SVN_ERR(svn_ra_initialize(pool));
> > +
> > +#if defined(WIN32) || defined(__CYGWIN__)
> > +	if (getenv("SVN_ASP_DOT_NET_HACK"))
> > +		SVN_ERR(svn_wc_set_adm_dir("_svn", pool));
> > +#endif
> 
> I guess it’s water under the bridge now (from 5 years ago), but why do
> clients have to do this themselves?  It would not be so difficult for
> libsvnclient to automatically set the admin dir according to whether
> SVN_ASP_DOT_NET_HACK is set or not, or at least to provide a single
> function to call and do so.

Good question. I copied this out from some legacy code in the
Subversion trunk. Additionally, since I don't have a working copy,
this is completely unnecessary. Removed now.

> > +
> > +	SVN_ERR(populate_context());
> > +	SVN_ERR(svn_cmdline_create_auth_baton(&(ctx->auth_baton), TRUE,
> > +					      NULL, NULL, NULL, FALSE,
> > +					      FALSE, NULL, NULL, NULL,
> > +					      pool));
> 
> Maybe comments would help, for the boolean arguments.

Fixed.

> > +	SVN_ERR(svn_client_open_ra_session(&session, url, ctx, pool));
> > +	return SVN_NO_ERROR;
> > +}
> > +
> > +svn_error_t *replay_range(svn_revnum_t start_revision, svn_revnum_t end_revision)
> > +{
> > +	return SVN_NO_ERROR;
> > +}
> 
> Might be more self-explanatory without this function, but that
> is just nitpicking.

I've filled in the function in a future patch. It's just to say that
I'm "opening a connection and then calling replay_range, in that
order". Yes, replay_range does nothing in this patch.

> > +
> > +int main()
> > +{
> > +	const char url[] = "http://svn.apache.org/repos/asf";
> > +	svn_revnum_t start_revision = 1, end_revision = 500;
> > +	if (svn_cmdline_init ("svndumpr", stderr) != EXIT_SUCCESS)
> > +		return 1;
> > +
> > +	pool = svn_pool_create(NULL);
> > +
> > +	SVN_INT_ERR(open_connection(url));
> > +	SVN_INT_ERR(replay_range(start_revision, end_revision));
> > +
> > +	svn_pool_destroy(pool);
> > +	
> > +	return 0;
> > +}
> 
> So: this is an expensive no-op.

Exactly. I built up the patches so that at every stage, the program
compiles and runs fine, implementing part of full functionality.

Here's a diff of the modifications I made after your review:

diff --git a/svndumpr.c b/svndumpr.c
index 011941f..f3117aa 100644
--- a/svndumpr.c
+++ b/svndumpr.c
@@ -76,31 +76,19 @@ static svn_error_t *replay_revend(svn_revnum_t revision,
 	return SVN_NO_ERROR;
 }
 
-svn_error_t *populate_context()
-{
-	const char *http_library;
-	
-	SVN_ERR(svn_config_get_config(&(ctx->config), NULL, pool));
-	
-	http_library = getenv("SVN_HTTP_LIBRARY");
-	if (http_library)
-		svn_config_set(apr_hash_get(ctx->config, "servers", APR_HASH_KEY_STRING),
-		               "global", "http-library", http_library);
-	return SVN_NO_ERROR;
-}
-
-svn_error_t *open_connection(const char *url)
+static svn_error_t *open_connection(const char *url)
 {
 	SVN_ERR(svn_config_ensure (NULL, pool));
 	SVN_ERR(svn_client_create_context (&ctx, pool));
 	SVN_ERR(svn_ra_initialize(pool));
 
-#if defined(WIN32) || defined(__CYGWIN__)
-	if (getenv("SVN_ASP_DOT_NET_HACK"))
-		SVN_ERR(svn_wc_set_adm_dir("_svn", pool));
-#endif
+	SVN_ERR(svn_config_get_config(&(ctx->config), NULL, pool));
 
-	SVN_ERR(populate_context());
+	/* Populte ctx->auth_baton with the auth baton
+	   non-interactively. Arguments 3, 4 and 5 are for username,
+	   password and config_dir which is NULL in this case. Set
+	   no_auth_cache and trust_serv_cert to FALSE, don't provide a
+	   config, and omit cancel_func/ cancel_baton */
 	SVN_ERR(svn_cmdline_create_auth_baton(&(ctx->auth_baton), TRUE,
 					      NULL, NULL, NULL, FALSE,
 					      FALSE, NULL, NULL, NULL,
@@ -109,7 +97,7 @@ svn_error_t *open_connection(const char *url)
 	return SVN_NO_ERROR;
 }
 
-svn_error_t *replay_range(svn_revnum_t start_revision, svn_revnum_t end_revision)
+static svn_error_t *replay_range(svn_revnum_t start_revision, svn_revnum_t end_revision)
 {
 	const svn_delta_editor_t *dump_editor, *debug_editor;
 	void *debug_baton, *dump_baton;


-- Ram

Re: [PATCH 02/13] Add skeleton SVN client and Makefile

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Here's a diff of the modifications I made after your review:

That’s quite helpful.

> +++ b/svndumpr.c
> @@ -76,31 +76,19 @@ static svn_error_t *replay_revend(svn_revnum_t revision,
[...]
> +	/* Populte ctx->auth_baton with the auth baton
> +	   non-interactively. Arguments 3, 4 and 5 are for username,
> +	   password and config_dir which is NULL in this case. Set
> +	   no_auth_cache and trust_serv_cert to FALSE, don't provide a
> +	   config, and omit cancel_func/ cancel_baton */
>  	SVN_ERR(svn_cmdline_create_auth_baton(&(ctx->auth_baton), TRUE,
>  					      NULL, NULL, NULL, FALSE,
>  					      FALSE, NULL, NULL, NULL,

I think you took my suggestion too seriously here.  Such a comment
probably will not help people much; instead, maybe a more focused
comment can help the curious avoid looking up
svn_cmdline_create_auth_baton:

	/* Default authentication providers for noninteractive
	   use. */
	SVN_ERR(svn_cmdline_create_auth_baton(...

Looking this up, I notice that function was added in svn 1.6.
Hopefully that is okay, since this code is destined for svn trunk.

Except as noted above,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

Re: [PATCH 02/13] Add skeleton SVN client and Makefile

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> 
> > Here's a diff of the modifications I made after your review:
> 
> That’s quite helpful.
> 
> > +++ b/svndumpr.c
> > @@ -76,31 +76,19 @@ static svn_error_t *replay_revend(svn_revnum_t revision,
> [...]
> > +	/* Populte ctx->auth_baton with the auth baton
> > +	   non-interactively. Arguments 3, 4 and 5 are for username,
> > +	   password and config_dir which is NULL in this case. Set
> > +	   no_auth_cache and trust_serv_cert to FALSE, don't provide a
> > +	   config, and omit cancel_func/ cancel_baton */
> >  	SVN_ERR(svn_cmdline_create_auth_baton(&(ctx->auth_baton), TRUE,
> >  					      NULL, NULL, NULL, FALSE,
> >  					      FALSE, NULL, NULL, NULL,
> 
> I think you took my suggestion too seriously here.  Such a comment
> probably will not help people much; instead, maybe a more focused
> comment can help the curious avoid looking up
> svn_cmdline_create_auth_baton:
> 
> 	/* Default authentication providers for noninteractive
> 	   use. */
> 	SVN_ERR(svn_cmdline_create_auth_baton(...

Fixed.

> Looking this up, I notice that function was added in svn 1.6.
> Hopefully that is okay, since this code is destined for svn trunk.

I have a working 1.6 fork now that I intend to merge into
git.git. When there's a new release of Subversion that includes my
patch, I'll remove it from git.git.

> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

-- Ram

Re: [PATCH 02/13] Add skeleton SVN client and Makefile

[Daniel Shahaf]

Jonathan Nieder wrote on Wed, 7 Jul 2010 at 11:25 -0500:
> Ramkumar Ramachandra wrote:
> > +svn_error_t *populate_context()
> > +{
> > +	const char *http_library;
> > +	
> > +	SVN_ERR(svn_config_get_config(&(ctx->config), NULL, pool));
> > +	
> > +	http_library = getenv("SVN_HTTP_LIBRARY");
> > +	if (http_library)
> > +		svn_config_set(apr_hash_get(ctx->config, "servers", APR_HASH_KEY_STRING),
> > +		               "global", "http-library", http_library);
> 
> I tried googling for this SVN_HTTP_LIBRARY setting, but no
> useful hints.  I take it that this overrides the [global] http-library
> setting from ~/.subversion/servers?  Do other commands honor this
> environment variable or just svndumpr?

Other commands use --config-option.  SVN_HTTP_LIBRARY is not recognized
by any released Subversion, nor by trunk@HEAD of Subversion.

It's a debugging relic, anyway.  Can be removed.  The functionality is 
provided by editing $CONFIG_DIR/servers.

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

[Ramkumar Ramachandra]

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

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Makefile       |    4 +-
 debug_editor.c |  402 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 debug_editor.h |   10 ++
 3 files changed, 414 insertions(+), 2 deletions(-)
 create mode 100644 debug_editor.c
 create mode 100644 debug_editor.h

diff --git a/Makefile b/Makefile
index a6022f7..e4d106e 100644
--- a/Makefile
+++ b/Makefile
@@ -1,8 +1,8 @@
 svndumpr: *.c *.h
-	$(CC) -Wall -Werror -DAPR_POOL_DEBUG -ggdb3 -O0 -o $@ svndumpr.c -lsvn_client-1 -I. -I/usr/include/subversion-1 -I/usr/include/apr-1.0
+	$(CC) -Wall -Werror -DAPR_POOL_DEBUG -ggdb3 -O0 -o $@ svndumpr.c debug_editor.c -lsvn_client-1 -I. -I/usr/local/include/subversion-1 -I/usr/include/apr-1.0
 
 svndumpr_bench: *.c *.h
-	$(CC) -O2 -o $@ svndumpr.c -lsvn_client-1 -I. -I/usr/include/subversion-1 -I/usr/include/apr-1.0
+	$(CC) -O2 -o $@ svndumpr.c debug_editor.c -lsvn_client-1 -I. -I/usr/local/include/subversion-1 -I/usr/include/apr-1.0
 
 clean:
 	$(RM) svndumpr svndumpr_bench
diff --git a/debug_editor.c b/debug_editor.c
new file mode 100644
index 0000000..8164477
--- /dev/null
+++ b/debug_editor.c
@@ -0,0 +1,402 @@
+/* Licensed under a two-clause BSD-style license.
+ * See LICENSE for details.
+ */
+
+#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;
+};
+
+struct dir_baton
+{
+	void *edit_baton;
+	void *wrapped_dir_baton;
+};
+
+struct file_baton
+{
+	void *edit_baton;
+	void *wrapped_file_baton;
+};
+
+static svn_error_t *write_indent(struct edit_baton *eb, apr_pool_t *pool)
+{
+	int i;
+
+	for (i = 0; i < eb->indent_level; ++i)
+		SVN_ERR(svn_stream_printf(eb->out, pool, " "));
+
+	return SVN_NO_ERROR;
+}
+
+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);
+}
+
+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;
+}
+
+static svn_error_t *delete_entry(const char *path,
+				 svn_revnum_t base_revision,
+				 void *parent_baton,
+				 apr_pool_t *pool)
+{
+	struct dir_baton *pb = parent_baton;
+	struct edit_baton *eb = pb->edit_baton;
+
+	SVN_ERR(write_indent(eb, pool));
+	SVN_ERR(svn_stream_printf(eb->out, pool, "delete_entry : %s:%ld\n",
+				  path, base_revision));
+
+	return eb->wrapped_editor->delete_entry(path,
+						base_revision,
+						pb->wrapped_dir_baton,
+						pool);
+}
+
+static svn_error_t *add_directory(const char *path,
+				  void *parent_baton,
+				  const char *copyfrom_path,
+				  svn_revnum_t copyfrom_revision,
+				  apr_pool_t *pool,
+				  void **child_baton)
+{
+	struct dir_baton *pb = parent_baton;
+	struct edit_baton *eb = pb->edit_baton;
+	struct dir_baton *b = apr_palloc(pool, sizeof(*b));
+
+	SVN_ERR(write_indent(eb, pool));
+	SVN_ERR(svn_stream_printf(eb->out, pool,
+				  "add_directory : '%s' [from '%s':%ld]\n",
+				  path, copyfrom_path, copyfrom_revision));
+	eb->indent_level++;
+
+	SVN_ERR(eb->wrapped_editor->add_directory(path,
+						  pb->wrapped_dir_baton,
+						  copyfrom_path,
+						  copyfrom_revision,
+						  pool,
+						  &b->wrapped_dir_baton));
+
+	b->edit_baton = eb;
+	*child_baton = b;
+
+	return SVN_NO_ERROR;
+}
+
+static svn_error_t *open_directory(const char *path,
+				   void *parent_baton,
+				   svn_revnum_t base_revision,
+				   apr_pool_t *pool,
+				   void **child_baton)
+{
+	struct dir_baton *pb = parent_baton;
+	struct edit_baton *eb = pb->edit_baton;
+	struct dir_baton *db = apr_palloc(pool, sizeof(*db));
+
+	SVN_ERR(write_indent(eb, pool));
+	SVN_ERR(svn_stream_printf(eb->out, pool, "open_directory : '%s':%ld\n",
+				  path, base_revision));
+	eb->indent_level++;
+
+	SVN_ERR(eb->wrapped_editor->open_directory(path,
+						   pb->wrapped_dir_baton,
+						   base_revision,
+						   pool,
+						   &db->wrapped_dir_baton));
+
+	db->edit_baton = eb;
+	*child_baton = db;
+
+	return SVN_NO_ERROR;
+}
+
+static svn_error_t *add_file(const char *path,
+			     void *parent_baton,
+			     const char *copyfrom_path,
+			     svn_revnum_t copyfrom_revision,
+			     apr_pool_t *pool,
+			     void **file_baton)
+{
+	struct dir_baton *pb = parent_baton;
+	struct edit_baton *eb = pb->edit_baton;
+	struct file_baton *fb = apr_palloc(pool, sizeof(*fb));
+
+	SVN_ERR(write_indent(eb, pool));
+	SVN_ERR(svn_stream_printf(eb->out, pool,
+				  "add_file : '%s' [from '%s':%ld]\n",
+				  path, copyfrom_path, copyfrom_revision));
+
+	eb->indent_level++;
+
+	SVN_ERR(eb->wrapped_editor->add_file(path,
+					     pb->wrapped_dir_baton,
+					     copyfrom_path,
+					     copyfrom_revision,
+					     pool,
+					     &fb->wrapped_file_baton));
+
+	fb->edit_baton = eb;
+	*file_baton = fb;
+
+	return SVN_NO_ERROR;
+}
+
+static svn_error_t *open_file(const char *path,
+			      void *parent_baton,
+			      svn_revnum_t base_revision,
+			      apr_pool_t *pool,
+			      void **file_baton)
+{
+	struct dir_baton *pb = parent_baton;
+	struct edit_baton *eb = pb->edit_baton;
+	struct file_baton *fb = apr_palloc(pool, sizeof(*fb));
+
+	SVN_ERR(write_indent(eb, pool));
+	SVN_ERR(svn_stream_printf(eb->out, pool, "open_file : '%s':%ld\n",
+				  path, base_revision));
+
+	eb->indent_level++;
+
+	SVN_ERR(eb->wrapped_editor->open_file(path,
+					      pb->wrapped_dir_baton,
+					      base_revision,
+					      pool,
+					      &fb->wrapped_file_baton));
+
+	fb->edit_baton = eb;
+	*file_baton = fb;
+
+	return SVN_NO_ERROR;
+}
+
+static svn_error_t *apply_textdelta(void *file_baton,
+				    const char *base_checksum,
+				    apr_pool_t *pool,
+				    svn_txdelta_window_handler_t *handler,
+				    void **handler_baton)
+{
+	struct file_baton *fb = file_baton;
+	struct edit_baton *eb = fb->edit_baton;
+
+	SVN_ERR(write_indent(eb, pool));
+	SVN_ERR(svn_stream_printf(eb->out, pool, "apply_textdelta : %s\n",
+				  base_checksum));
+
+	SVN_ERR(eb->wrapped_editor->apply_textdelta(fb->wrapped_file_baton,
+						    base_checksum,
+						    pool,
+						    handler,
+						    handler_baton));
+
+	return SVN_NO_ERROR;
+}
+
+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;
+}
+
+static svn_error_t *absent_file(const char *path,
+				void *file_baton,
+				apr_pool_t *pool)
+{
+	struct file_baton *fb = file_baton;
+	struct edit_baton *eb = fb->edit_baton;
+
+	SVN_ERR(write_indent(eb, pool));
+	SVN_ERR(svn_stream_printf(eb->out, pool, "absent_file : %s\n", path));
+
+	SVN_ERR(eb->wrapped_editor->absent_file(path, fb->wrapped_file_baton,
+						pool));
+
+	return SVN_NO_ERROR;
+}
+
+static svn_error_t *close_directory(void *dir_baton,
+				    apr_pool_t *pool)
+{
+	struct dir_baton *db = dir_baton;
+	struct edit_baton *eb = db->edit_baton;
+
+	eb->indent_level--;
+	SVN_ERR(write_indent(eb, pool));
+	SVN_ERR(svn_stream_printf(eb->out, pool, "close_directory\n"));
+
+	SVN_ERR(eb->wrapped_editor->close_directory(db->wrapped_dir_baton,
+						    pool));
+
+	return SVN_NO_ERROR;
+}
+
+static svn_error_t *absent_directory(const char *path,
+				     void *dir_baton,
+				     apr_pool_t *pool)
+{
+	struct dir_baton *db = dir_baton;
+	struct edit_baton *eb = db->edit_baton;
+
+	SVN_ERR(write_indent(eb, pool));
+	SVN_ERR(svn_stream_printf(eb->out, pool, "absent_directory : %s\n",
+				  path));
+
+	SVN_ERR(eb->wrapped_editor->absent_directory(path, db->wrapped_dir_baton,
+						     pool));
+
+	return SVN_NO_ERROR;
+}
+
+static svn_error_t *change_file_prop(void *file_baton,
+				     const char *name,
+				     const svn_string_t *value,
+				     apr_pool_t *pool)
+{
+	struct file_baton *fb = file_baton;
+	struct edit_baton *eb = fb->edit_baton;
+
+	SVN_ERR(write_indent(eb, pool));
+	SVN_ERR(svn_stream_printf(eb->out, pool, "change_file_prop : %s\n",
+				  name));
+
+	SVN_ERR(eb->wrapped_editor->change_file_prop(fb->wrapped_file_baton,
+						     name,
+						     value,
+						     pool));
+
+	return SVN_NO_ERROR;
+}
+
+static svn_error_t *change_dir_prop(void *dir_baton,
+				    const char *name,
+				    const svn_string_t *value,
+				    apr_pool_t *pool)
+{
+	struct dir_baton *db = dir_baton;
+	struct edit_baton *eb = db->edit_baton;
+
+	SVN_ERR(write_indent(eb, pool));
+	SVN_ERR(svn_stream_printf(eb->out, pool, "change_dir_prop : %s\n", name));
+
+	SVN_ERR(eb->wrapped_editor->change_dir_prop(db->wrapped_dir_baton,
+						    name,
+						    value,
+						    pool));
+
+	return SVN_NO_ERROR;
+}
+
+static svn_error_t *close_edit(void *edit_baton,
+			       apr_pool_t *pool)
+{
+	struct edit_baton *eb = edit_baton;
+
+	SVN_ERR(write_indent(eb, pool));
+	SVN_ERR(svn_stream_printf(eb->out, pool, "close_edit\n"));
+
+	SVN_ERR(eb->wrapped_editor->close_edit(eb->wrapped_edit_baton, pool));
+
+	return SVN_NO_ERROR;
+}
+
+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");
+
+	out = svn_stream_from_aprfile2(errfp, TRUE, pool);
+
+	tree_editor->set_target_revision = set_target_revision;
+	tree_editor->open_root = open_root;
+	tree_editor->delete_entry = delete_entry;
+	tree_editor->add_directory = add_directory;
+	tree_editor->open_directory = open_directory;
+	tree_editor->change_dir_prop = change_dir_prop;
+	tree_editor->close_directory = close_directory;
+	tree_editor->absent_directory = absent_directory;
+	tree_editor->add_file = add_file;
+	tree_editor->open_file = open_file;
+	tree_editor->apply_textdelta = apply_textdelta;
+	tree_editor->change_file_prop = change_file_prop;
+	tree_editor->close_file = close_file;
+	tree_editor->absent_file = absent_file;
+	tree_editor->close_edit = close_edit;
+
+	eb->wrapped_editor = wrapped_editor;
+	eb->wrapped_edit_baton = wrapped_edit_baton;
+	eb->out = out;
+	eb->indent_level = 0;
+
+	*editor = tree_editor;
+	*edit_baton = eb;
+
+	return SVN_NO_ERROR;
+}
diff --git a/debug_editor.h b/debug_editor.h
new file mode 100644
index 0000000..a0d412a
--- /dev/null
+++ 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);
+
+#endif
-- 
1.7.1

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

[Jonathan Nieder]

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

[PATCH 04/13] Add skeleton dump editor

[Ramkumar Ramachandra]

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.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Makefile      |    4 +-
 dump_editor.c |  143 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 dump_editor.h |    8 +++
 dumpr_util.h  |   29 ++++++++++++
 4 files changed, 182 insertions(+), 2 deletions(-)
 create mode 100644 dump_editor.c
 create mode 100644 dump_editor.h
 create mode 100644 dumpr_util.h

diff --git a/Makefile b/Makefile
index e4d106e..fea646e 100644
--- a/Makefile
+++ b/Makefile
@@ -1,8 +1,8 @@
 svndumpr: *.c *.h
-	$(CC) -Wall -Werror -DAPR_POOL_DEBUG -ggdb3 -O0 -o $@ svndumpr.c debug_editor.c -lsvn_client-1 -I. -I/usr/local/include/subversion-1 -I/usr/include/apr-1.0
+	$(CC) -Wall -Werror -DAPR_POOL_DEBUG -ggdb3 -O0 -o $@ svndumpr.c debug_editor.c dump_editor.c -lsvn_client-1 -I. -I/usr/local/include/subversion-1 -I/usr/include/apr-1.0
 
 svndumpr_bench: *.c *.h
-	$(CC) -O2 -o $@ svndumpr.c debug_editor.c -lsvn_client-1 -I. -I/usr/local/include/subversion-1 -I/usr/include/apr-1.0
+	$(CC) -O2 -o $@ svndumpr.c debug_editor.c dump_editor.c -lsvn_client-1 -I. -I/usr/local/include/subversion-1 -I/usr/include/apr-1.0
 
 clean:
 	$(RM) svndumpr svndumpr_bench
diff --git a/dump_editor.c b/dump_editor.c
new file mode 100644
index 0000000..2fdf93c
--- /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"
+
+#include "dumpr_util.h"
+
+svn_error_t *open_root(void *edit_baton,
+                       svn_revnum_t base_revision,
+                       apr_pool_t *pool,
+                       void **root_baton)
+{
+	return SVN_NO_ERROR;
+}
+
+svn_error_t *delete_entry(const char *path,
+                          svn_revnum_t revision,
+                          void *parent_baton,
+                          apr_pool_t *pool)
+{
+	return SVN_NO_ERROR;
+}
+
+svn_error_t *add_directory(const char *path,
+                           void *parent_baton,
+                           const char *copyfrom_path,
+                           svn_revnum_t copyfrom_rev,
+                           apr_pool_t *pool,
+                           void **child_baton)
+{
+	return SVN_NO_ERROR;
+}
+
+svn_error_t *open_directory(const char *path,
+                            void *parent_baton,
+                            svn_revnum_t base_revision,
+                            apr_pool_t *pool,
+                            void **child_baton)
+{
+	return SVN_NO_ERROR;
+}
+
+svn_error_t *close_directory(void *dir_baton,
+                             apr_pool_t *pool)
+{
+	return SVN_NO_ERROR;
+}
+
+svn_error_t *add_file(const char *path,
+                      void *parent_baton,
+                      const char *copyfrom_path,
+                      svn_revnum_t copyfrom_rev,
+                      apr_pool_t *pool,
+                      void **file_baton)
+{
+	return SVN_NO_ERROR;
+}
+
+svn_error_t *open_file(const char *path,
+                       void *parent_baton,
+                       svn_revnum_t ancestor_revision,
+                       apr_pool_t *pool,
+                       void **file_baton)
+{
+	return SVN_NO_ERROR;
+}
+
+svn_error_t *change_dir_prop(void *parent_baton,
+                             const char *name,
+                             const svn_string_t *value,
+                             apr_pool_t *pool)
+{
+	return SVN_NO_ERROR;
+}
+
+svn_error_t *change_file_prop(void *file_baton,
+                              const char *name,
+                              const svn_string_t *value,
+                              apr_pool_t *pool)
+{
+	return SVN_NO_ERROR;
+}
+
+svn_error_t *apply_textdelta(void *file_baton, const char *base_checksum,
+                             apr_pool_t *pool,
+                             svn_txdelta_window_handler_t *handler,
+                             void **handler_baton)
+{
+	return SVN_NO_ERROR;
+}
+
+svn_error_t *close_file(void *file_baton,
+			const char *text_checksum,
+			apr_pool_t *pool)
+{
+	return SVN_NO_ERROR;
+}
+
+svn_error_t *close_edit(void *edit_baton, apr_pool_t *pool)
+{
+	return SVN_NO_ERROR;
+}
+
+svn_error_t *get_dump_editor(const svn_delta_editor_t **editor,
+                             void **edit_baton,
+                             svn_revnum_t from_rev,
+                             apr_pool_t *pool)
+{
+	struct edit_baton *eb = apr_pcalloc(pool, sizeof(struct edit_baton));
+	eb->current_rev = from_rev;
+	SVN_ERR(svn_stream_for_stdout(&(eb->stream), pool));
+	svn_delta_editor_t *de = svn_delta_default_editor(pool);
+	
+	de->open_root = open_root;
+	de->delete_entry = delete_entry;
+	de->add_directory = add_directory;
+	de->open_directory = open_directory;
+	de->close_directory = close_directory;
+	de->change_dir_prop = change_dir_prop;
+	de->change_file_prop = change_file_prop;
+	de->apply_textdelta = apply_textdelta;
+	de->add_file = add_file;
+	de->open_file = open_file;
+	de->close_file = close_file;
+	de->close_edit = close_edit;
+
+	/* Set the edit_baton and editor */
+	*edit_baton = eb;
+	*editor = de;
+
+	return SVN_NO_ERROR;
+}
+ 
diff --git a/dump_editor.h b/dump_editor.h
new file mode 100644
index 0000000..9c70b74
--- /dev/null
+++ b/dump_editor.h
@@ -0,0 +1,8 @@
+#ifndef DUMP_EDITOR_H_
+#define DUMP_EDITOR_H_
+
+svn_error_t *get_dump_editor(const svn_delta_editor_t **editor,
+			     void **edit_baton,
+			     svn_revnum_t to_rev,
+			     apr_pool_t *pool);
+#endif
diff --git a/dumpr_util.h b/dumpr_util.h
new file mode 100644
index 0000000..d206c19
--- /dev/null
+++ b/dumpr_util.h
@@ -0,0 +1,29 @@
+#ifndef DUMPR_UTIL_H_
+#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
-- 
1.7.1

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

[Jonathan Nieder]

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.

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

> --- /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?

[...]
> +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.

[...]
> +++ 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.

> +};

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

[Ramkumar Ramachandra]

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

[PATCH 05/13] Drive the debug editor

[Ramkumar Ramachandra]

Wrap the dummy dump editor in the debug editor and drive the debug
editor to print out all the actions that occur during the replay to
stderr.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 dump_editor.c |    2 +-
 dumpr_util.h  |    5 +++++
 svndumpr.c    |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 1 deletions(-)

diff --git a/dump_editor.c b/dump_editor.c
index 2fdf93c..70d6c0b 100644
--- a/dump_editor.c
+++ b/dump_editor.c
@@ -128,7 +128,7 @@ svn_error_t *get_dump_editor(const svn_delta_editor_t **editor,
 	de->close_directory = close_directory;
 	de->change_dir_prop = change_dir_prop;
 	de->change_file_prop = change_file_prop;
-	de->apply_textdelta = apply_textdelta;
+	/* de->apply_textdelta = apply_textdelta; */
 	de->add_file = add_file;
 	de->open_file = open_file;
 	de->close_file = close_file;
diff --git a/dumpr_util.h b/dumpr_util.h
index d206c19..166e214 100644
--- a/dumpr_util.h
+++ b/dumpr_util.h
@@ -1,6 +1,11 @@
 #ifndef DUMPR_UTIL_H_
 #define DUMPR_UTIL_H_
 
+struct replay_baton {
+	const svn_delta_editor_t *editor;
+	void *baton;
+};
+
 struct edit_baton {
 	/* The stream to dump to: stdout */
 	svn_stream_t *stream;
diff --git a/svndumpr.c b/svndumpr.c
index 737c4aa..853facd 100644
--- a/svndumpr.c
+++ b/svndumpr.c
@@ -8,10 +8,40 @@
 #include "svn_ra.h"
 #include "svn_repos.h"
 
+#include "debug_editor.h"
+#include "dump_editor.h"
+#include "dumpr_util.h"
+
 static apr_pool_t *pool = NULL;
 static svn_client_ctx_t *ctx = NULL;
 static svn_ra_session_t *session = NULL;
 
+static svn_error_t *replay_revstart(svn_revnum_t revision,
+                                    void *replay_baton,
+                                    const svn_delta_editor_t **editor,
+                                    void **edit_baton,
+                                    apr_hash_t *rev_props,
+                                    apr_pool_t *pool)
+{
+	/* Extract editor and editor_baton from the replay_baton and
+	   set them so that the editor callbacks can use them */
+	struct replay_baton *rb = replay_baton;
+	*editor = rb->editor;
+	*edit_baton = rb->baton;
+
+	return SVN_NO_ERROR;
+}
+
+static svn_error_t *replay_revend(svn_revnum_t revision,
+                                  void *replay_baton,
+                                  const svn_delta_editor_t *editor,
+                                  void *edit_baton,
+                                  apr_hash_t *rev_props,
+                                  apr_pool_t *pool)
+{
+	return SVN_NO_ERROR;
+}
+
 svn_error_t *populate_context()
 {
 	const char *http_library;
@@ -47,6 +77,25 @@ svn_error_t *open_connection(const char *url)
 
 svn_error_t *replay_range(svn_revnum_t start_revision, svn_revnum_t end_revision)
 {
+	const svn_delta_editor_t *dump_editor, *debug_editor;
+	void *debug_baton, *dump_baton;
+
+	SVN_ERR(get_dump_editor(&dump_editor,
+	                        &dump_baton, start_revision, pool));
+
+	SVN_ERR(svn_delta__get_debug_editor(&debug_editor,
+	                                    &debug_baton,
+	                                    dump_editor,
+	                                    dump_baton, pool));
+
+	struct replay_baton *replay_baton = apr_palloc(pool, sizeof(struct replay_baton));
+	replay_baton->editor = debug_editor;
+	replay_baton->baton = debug_baton;
+	SVN_ERR(svn_cmdline_printf(pool, SVN_REPOS_DUMPFILE_MAGIC_HEADER ": %d\n",
+				   SVN_REPOS_DUMPFILE_FORMAT_VERSION));
+	SVN_ERR(svn_ra_replay_range(session, start_revision, end_revision,
+	                            0, TRUE, replay_revstart, replay_revend,
+	                            replay_baton, pool));
 	return SVN_NO_ERROR;
 }
 
-- 
1.7.1

Re: [PATCH 05/13] Drive the debug editor

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> +++ b/dump_editor.c
> @@ -128,7 +128,7 @@ svn_error_t *get_dump_editor(const svn_delta_editor_t **editor,
>  	de->close_directory = close_directory;
>  	de->change_dir_prop = change_dir_prop;
>  	de->change_file_prop = change_file_prop;
> -	de->apply_textdelta = apply_textdelta;
> +	/* de->apply_textdelta = apply_textdelta; */

Hmm...

[...]
> +++ b/dumpr_util.h
> @@ -1,6 +1,11 @@
>  #ifndef DUMPR_UTIL_H_
>  #define DUMPR_UTIL_H_
>  
> +struct replay_baton {
> +	const svn_delta_editor_t *editor;
> +	void *baton;
> +};
> +

Context during svnsync-like replay ops:

 - a diff replayer
 - its context object

Maybe "void *edit_baton" would be clearer.

>  struct edit_baton {

Which might involve renaming this to dump_edit_baton to avoid
confusion.

> +++ b/svndumpr.c
> @@ -8,10 +8,40 @@
[...]
> +static svn_error_t *replay_revstart(svn_revnum_t revision,
> +                                    void *replay_baton,
> +                                    const svn_delta_editor_t **editor,
> +                                    void **edit_baton,
> +                                    apr_hash_t *rev_props,
> +                                    apr_pool_t *pool)

This function is called to acquire an editor to replay one revision.

> +{
> +	/* Extract editor and editor_baton from the replay_baton and
> +	   set them so that the editor callbacks can use them */

This comment just paraphrases the code.  What in particular requires
explanation here?

> +	struct replay_baton *rb = replay_baton;
> +	*editor = rb->editor;
> +	*edit_baton = rb->baton;
> +
> +	return SVN_NO_ERROR;
> +}

[...]
> @@ -47,6 +77,25 @@ svn_error_t *open_connection(const char *url)
>  
>  svn_error_t *replay_range(svn_revnum_t start_revision, svn_revnum_t end_revision)
>  {
[...]
> +	SVN_ERR(svn_cmdline_printf(pool, SVN_REPOS_DUMPFILE_MAGIC_HEADER ": %d\n",
> +				   SVN_REPOS_DUMPFILE_FORMAT_VERSION));

Did this sneak in from a later patch?

> +	SVN_ERR(svn_ra_replay_range(session, start_revision, end_revision,
> +	                            0, TRUE, replay_revstart, replay_revend,
> +	                            replay_baton, pool));

Makes sense.

Re: [PATCH 05/13] Drive the debug editor

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> 
> > +++ b/dump_editor.c
> > @@ -128,7 +128,7 @@ svn_error_t *get_dump_editor(const svn_delta_editor_t **editor,
> >  	de->close_directory = close_directory;
> >  	de->change_dir_prop = change_dir_prop;
> >  	de->change_file_prop = change_file_prop;
> > -	de->apply_textdelta = apply_textdelta;
> > +	/* de->apply_textdelta = apply_textdelta; */
> 
> Hmm...

Without this, the program segfaults because the necessary setup for
applying a text delta hasn't been set up. Perhaps I should explain
this in my commit message?

> [...]
> > +++ b/dumpr_util.h
> > @@ -1,6 +1,11 @@
> >  #ifndef DUMPR_UTIL_H_
> >  #define DUMPR_UTIL_H_
> >  
> > +struct replay_baton {
> > +	const svn_delta_editor_t *editor;
> > +	void *baton;
> > +};
> > +
> 
> Context during svnsync-like replay ops:
> 
>  - a diff replayer
>  - its context object
> 
> Maybe "void *edit_baton" would be clearer.
> 
> >  struct edit_baton {
> 
> Which might involve renaming this to dump_edit_baton to avoid
> confusion.

Done. I renamed both.

> > +++ b/svndumpr.c
> > @@ -8,10 +8,40 @@
> [...]
> > +static svn_error_t *replay_revstart(svn_revnum_t revision,
> > +                                    void *replay_baton,
> > +                                    const svn_delta_editor_t **editor,
> > +                                    void **edit_baton,
> > +                                    apr_hash_t *rev_props,
> > +                                    apr_pool_t *pool)
> 
> This function is called to acquire an editor to replay one revision.
> 
> > +{
> > +	/* Extract editor and editor_baton from the replay_baton and
> > +	   set them so that the editor callbacks can use them */
> 
> This comment just paraphrases the code.  What in particular requires
> explanation here?

This concept took me some time to wrap my head around: I had to stuff
the replay_baton with the editor/ editor_baton so that I could set
them for use in the callback functions. Comment moved to a later
patch.

> > +	struct replay_baton *rb = replay_baton;
> > +	*editor = rb->editor;
> > +	*edit_baton = rb->baton;
> > +
> > +	return SVN_NO_ERROR;
> > +}
> 
> [...]
> > @@ -47,6 +77,25 @@ svn_error_t *open_connection(const char *url)
> >  
> >  svn_error_t *replay_range(svn_revnum_t start_revision, svn_revnum_t end_revision)
> >  {
> [...]
> > +	SVN_ERR(svn_cmdline_printf(pool, SVN_REPOS_DUMPFILE_MAGIC_HEADER ": %d\n",
> > +				   SVN_REPOS_DUMPFILE_FORMAT_VERSION));
> 
> Did this sneak in from a later patch?

Yes. Fixed now. I moved it this change to the next patch.

> > +	SVN_ERR(svn_ra_replay_range(session, start_revision, end_revision,
> > +	                            0, TRUE, replay_revstart, replay_revend,
> > +	                            replay_baton, pool));
> 
> Makes sense.

Thanks for the excellent review.

-- Ram

Re: [PATCH 05/13] Drive the debug editor

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Jonathan Nieder writes:
>> Ramkumar Ramachandra wrote:

>>> -	de->apply_textdelta = apply_textdelta;
>>> +	/* de->apply_textdelta = apply_textdelta; */
[...]
> Without this, the program segfaults because the necessary setup for
> applying a text delta hasn't been set up. Perhaps I should explain
> this in my commit message?

Is the default apply_textdelta not a no-op?  What work does it have to
do, and can the skeleton editor be convinced to do the same in patch 4?

>>> +{
>>> +	/* Extract editor and editor_baton from the replay_baton and
>>> +	   set them so that the editor callbacks can use them */
>>
>> This comment just paraphrases the code.  What in particular requires
>> explanation here?
>
> This concept took me some time to wrap my head around: I had to stuff
> the replay_baton with the editor/ editor_baton so that I could set
> them for use in the callback functions.

Ah, okay.  Then I suppose it belongs in the commit message.

Alternatively: why does the tree editor have to persist between calls
replaying the various revisions?  That information could help the
reader understand what is going on.

Re: [PATCH 05/13] Drive the debug editor

[Ramkumar Ramachandra]

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> > Jonathan Nieder writes:
> >> Ramkumar Ramachandra wrote:
> 
> >>> -	de->apply_textdelta = apply_textdelta;
> >>> +	/* de->apply_textdelta = apply_textdelta; */
> [...]
> > Without this, the program segfaults because the necessary setup for
> > applying a text delta hasn't been set up. Perhaps I should explain
> > this in my commit message?
> 
> Is the default apply_textdelta not a no-op?  What work does it have to
> do, and can the skeleton editor be convinced to do the same in patch 4?

The default editor does this:
  *handler = svn_delta_noop_window_handler;
  *handler_baton = NULL;
  return SVN_NO_ERROR;

Fixed.

> >>> +{
> >>> +	/* Extract editor and editor_baton from the replay_baton and
> >>> +	   set them so that the editor callbacks can use them */
> >>
> >> This comment just paraphrases the code.  What in particular requires
> >> explanation here?
> >
> > This concept took me some time to wrap my head around: I had to stuff
> > the replay_baton with the editor/ editor_baton so that I could set
> > them for use in the callback functions.
> 
> Ah, okay.  Then I suppose it belongs in the commit message.
>
> Alternatively: why does the tree editor have to persist between calls
> replaying the various revisions?  That information could help the
> reader understand what is going on.

Right. The editor_baton is the key item that's guaranteed to be passed
around. Fixed.

-- Ram

[PATCH 06/13] Dump the revprops at the start of every revision

[Ramkumar Ramachandra]

Fill in replay_revstart to dump the revprops at the start of every
revision. Add an additional write_hash_to_stringbuf helper function.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Makefile     |    4 +-
 dumpr_util.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 dumpr_util.h |    5 ++++
 svndumpr.c   |   38 ++++++++++++++++++++++++++++++++-
 4 files changed, 107 insertions(+), 4 deletions(-)
 create mode 100644 dumpr_util.c

diff --git a/Makefile b/Makefile
index fea646e..c0b5c8a 100644
--- a/Makefile
+++ b/Makefile
@@ -1,8 +1,8 @@
 svndumpr: *.c *.h
-	$(CC) -Wall -Werror -DAPR_POOL_DEBUG -ggdb3 -O0 -o $@ svndumpr.c debug_editor.c dump_editor.c -lsvn_client-1 -I. -I/usr/local/include/subversion-1 -I/usr/include/apr-1.0
+	$(CC) -Wall -Werror -DAPR_POOL_DEBUG -ggdb3 -O0 -o $@ svndumpr.c debug_editor.c dump_editor.c dumpr_util.c -lsvn_client-1 -I. -I/usr/local/include/subversion-1 -I/usr/include/apr-1.0
 
 svndumpr_bench: *.c *.h
-	$(CC) -O2 -o $@ svndumpr.c debug_editor.c dump_editor.c -lsvn_client-1 -I. -I/usr/local/include/subversion-1 -I/usr/include/apr-1.0
+	$(CC) -O2 -o $@ svndumpr.c debug_editor.c dump_editor.c dumpr_util.c -lsvn_client-1 -I. -I/usr/local/include/subversion-1 -I/usr/include/apr-1.0
 
 clean:
 	$(RM) svndumpr svndumpr_bench
diff --git a/dumpr_util.c b/dumpr_util.c
new file mode 100644
index 0000000..41940d4
--- /dev/null
+++ b/dumpr_util.c
@@ -0,0 +1,64 @@
+/* Licensed under a two-clause BSD-style license.
+ * See LICENSE for details.
+ */
+
+#include "svn_pools.h"
+#include "svn_cmdline.h"
+#include "svn_client.h"
+#include "svn_ra.h"
+#include "svn_repos.h"
+
+#include "dumpr_util.h"
+
+void write_hash_to_stringbuf(apr_hash_t *properties,
+			     svn_boolean_t deleted,
+			     svn_stringbuf_t **strbuf,
+			     apr_pool_t *pool)
+{
+	apr_hash_index_t *this;
+	const void *key;
+	void *val;
+	apr_ssize_t keylen;
+	svn_string_t *value;
+	
+	if (!deleted) {
+		for (this = apr_hash_first(pool, properties); this;
+		     this = apr_hash_next(this)) {
+			/* Get this key and val. */
+			apr_hash_this(this, &key, &keylen, &val);
+			value = val;
+
+			/* Output name length, then name. */
+			svn_stringbuf_appendcstr(*strbuf,
+						 apr_psprintf(pool, "K %" APR_SSIZE_T_FMT "\n",
+							      keylen));
+
+			svn_stringbuf_appendbytes(*strbuf, (const char *) key, keylen);
+			svn_stringbuf_appendbytes(*strbuf, "\n", 1);
+
+			/* Output value length, then value. */
+			svn_stringbuf_appendcstr(*strbuf,
+						 apr_psprintf(pool, "V %" APR_SIZE_T_FMT "\n",
+							      value->len));
+
+			svn_stringbuf_appendbytes(*strbuf, value->data, value->len);
+			svn_stringbuf_appendbytes(*strbuf, "\n", 1);
+		}
+	}
+	else {
+		/* Output a "D " entry for each deleted property */
+		for (this = apr_hash_first(pool, properties); this;
+		     this = apr_hash_next(this)) {
+			/* Get this key */
+			apr_hash_this(this, &key, &keylen, NULL);
+
+			/* Output name length, then name */
+			svn_stringbuf_appendcstr(*strbuf,
+						 apr_psprintf(pool, "D %" APR_SSIZE_T_FMT "\n",
+							      keylen));
+
+			svn_stringbuf_appendbytes(*strbuf, (const char *) key, keylen);
+			svn_stringbuf_appendbytes(*strbuf, "\n", 1);
+		}
+	}
+}
diff --git a/dumpr_util.h b/dumpr_util.h
index 166e214..1a5752b 100644
--- a/dumpr_util.h
+++ b/dumpr_util.h
@@ -31,4 +31,9 @@ struct edit_baton {
 	svn_checksum_t *checksum;
 };
 
+void write_hash_to_stringbuf(apr_hash_t *properties,
+			     svn_boolean_t deleted,
+			     svn_stringbuf_t **strbuf,
+			     apr_pool_t *pool);
+
 #endif
diff --git a/svndumpr.c b/svndumpr.c
index 853facd..011941f 100644
--- a/svndumpr.c
+++ b/svndumpr.c
@@ -23,6 +23,37 @@ static svn_error_t *replay_revstart(svn_revnum_t revision,
                                     apr_hash_t *rev_props,
                                     apr_pool_t *pool)
 {
+	/* Editing this revision has just started; dump the revprops
+	   before invoking the editor callbacks */
+	svn_stringbuf_t *propstring = svn_stringbuf_create("", pool);
+	svn_stream_t *stdout_stream;
+
+	/* Create an stdout stream */
+	svn_stream_for_stdout(&stdout_stream, pool);
+
+        /* Print revision number and prepare the propstring */
+	SVN_ERR(svn_stream_printf(stdout_stream, pool,
+				  SVN_REPOS_DUMPFILE_REVISION_NUMBER
+				  ": %ld\n", revision));
+	write_hash_to_stringbuf(rev_props, FALSE, &propstring, pool);
+	svn_stringbuf_appendbytes(propstring, "PROPS-END\n", 10);
+
+	/* prop-content-length header */
+	SVN_ERR(svn_stream_printf(stdout_stream, pool,
+				  SVN_REPOS_DUMPFILE_PROP_CONTENT_LENGTH
+				  ": %" APR_SIZE_T_FMT "\n", propstring->len));
+
+	/* content-length header */
+	SVN_ERR(svn_stream_printf(stdout_stream, pool,
+				  SVN_REPOS_DUMPFILE_CONTENT_LENGTH
+				  ": %" APR_SIZE_T_FMT "\n\n", propstring->len));
+
+	/* Print the revprops now */
+	SVN_ERR(svn_stream_write(stdout_stream, propstring->data,
+				 &(propstring->len)));
+
+	svn_stream_close(stdout_stream);
+
 	/* Extract editor and editor_baton from the replay_baton and
 	   set them so that the editor callbacks can use them */
 	struct replay_baton *rb = replay_baton;
@@ -39,6 +70,9 @@ static svn_error_t *replay_revend(svn_revnum_t revision,
                                   apr_hash_t *rev_props,
                                   apr_pool_t *pool)
 {
+	/* Editor has finished for this revision and close_edit has
+	   been called; do nothing: just continue to the next
+	   revision */
 	return SVN_NO_ERROR;
 }
 
@@ -89,8 +123,8 @@ svn_error_t *replay_range(svn_revnum_t start_revision, svn_revnum_t end_revision
 	                                    dump_baton, pool));
 
 	struct replay_baton *replay_baton = apr_palloc(pool, sizeof(struct replay_baton));
-	replay_baton->editor = debug_editor;
-	replay_baton->baton = debug_baton;
+	replay_baton->editor = dump_editor;
+	replay_baton->baton = dump_baton;
 	SVN_ERR(svn_cmdline_printf(pool, SVN_REPOS_DUMPFILE_MAGIC_HEADER ": %d\n",
 				   SVN_REPOS_DUMPFILE_FORMAT_VERSION));
 	SVN_ERR(svn_ra_replay_range(session, start_revision, end_revision,
-- 
1.7.1

Re: [PATCH 06/13] Dump the revprops at the start of every revision

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Fill in replay_revstart to dump the revprops at the start of every
> revision. Add an additional write_hash_to_stringbuf helper function.

	A write_hash_to_stringbuf helper does the work of
	converting the property hashtable to dumpfile format.

> +++ b/dumpr_util.c
[...]
> +void write_hash_to_stringbuf(apr_hash_t *properties,
> +			     svn_boolean_t deleted,
> +			     svn_stringbuf_t **strbuf,
> +			     apr_pool_t *pool)

This function looks like:

	void write_hash_to_stringbuf(...
	{
		if (!deleted) {
			for (each prop) {
				append the new value of the prop;
			}
		} else {
			for (each prop) {
				mention that it has been deleted;
			}
		}
	}

It would be simpler to put the body of the loop in its own function,
like this:

	static void write_prop_to_stringbuf(...
	{
		if (deleted) {
			append deletion notice;
			return;
		}
		append new value of prop;
	}

	void write_hash_to_stringbuf(...
	{
		for (each prop)
			write_prop_to_stringbuf(...
	}

Or even:

	static void write_prop(...
	static void write_deleted_prop(...

	void write_prop_data_to_stringbuf(...
	{
		for (each prop)
			write_prop(...
	}
	void write_deleted_prop_data_to_stringbuf(...
	{
		for (each prop)
			write_deleted_prop(...
	}

which would make the arguments from the caller less opaque.

I did not check whether the "return early in the simpler case" is
idiomatic for svn code.  Of course you should respect whatever
convention is prevalent.

> +{
> +	apr_hash_index_t *this;
> +	const void *key;
> +	void *val;
> +	apr_ssize_t keylen;
> +	svn_string_t *value;
> +	
> +	if (!deleted) {
> +		for (this = apr_hash_first(pool, properties); this;
> +		     this = apr_hash_next(this)) {
> +			/* Get this key and val. */
> +			apr_hash_this(this, &key, &keylen, &val);
> +			value = val;
> +
> +			/* Output name length, then name. */
> +			svn_stringbuf_appendcstr(*strbuf,
> +						 apr_psprintf(pool, "K %" APR_SSIZE_T_FMT "\n",
> +							      keylen));
> +
> +			svn_stringbuf_appendbytes(*strbuf, (const char *) key, keylen);

Is the cast needed?  (The answer might be "yes" if this code is meant
to be usable with C++ compilers.)

> +++ b/svndumpr.c
> @@ -23,6 +23,37 @@ static svn_error_t *replay_revstart(svn_revnum_t revision,
>                                      apr_hash_t *rev_props,
>                                      apr_pool_t *pool)
>  {
> +	/* Editing this revision has just started; dump the revprops
> +	   before invoking the editor callbacks */
> +	svn_stringbuf_t *propstring = svn_stringbuf_create("", pool);
> +	svn_stream_t *stdout_stream;

Style: better to say in comments what we are trying to do than what we
actually do.  So:

	/* First, dump revision properties. */

Maybe dumping revision properties should be its own function to make
that comment unnecessary (and make replay_revstart() less daunting as
it grows).

> +
> +	/* Create an stdout stream */
> +	svn_stream_for_stdout(&stdout_stream, pool);

Useless comment.

> +
> +        /* Print revision number and prepare the propstring */
> +	SVN_ERR(svn_stream_printf(stdout_stream, pool,
> +				  SVN_REPOS_DUMPFILE_REVISION_NUMBER
> +				  ": %ld\n", revision));
> +	write_hash_to_stringbuf(rev_props, FALSE, &propstring, pool);
> +	svn_stringbuf_appendbytes(propstring, "PROPS-END\n", 10);

Unhelpful comment.  Maybe:

	/* Revision-number: 19 */
	SVN_ERR(svn_stream_printf(stdout_stream, pool,
				  SVN_REPOS_DUMPFILE_REVISION_NUMBER
				  ": %ld\n", revision));

	write_hash_to_stringbuf(rev_props, FALSE, &propstring, pool);
	svn_stringbuf_appendbytes(propstring, "PROPS-END\n", 10);

	/* Prop-content-length: 13 */
	SVN_ERR(svn_stream_printf(stdout_stream, pool,
				  SVN_REPOS_DUMPFILE_PROP_CONTENT_LENGTH
				  ": %" APR_SIZE_T_FMT "\n", propstring->len));
	...

This would make it particularly easy to grep for a particular header
(even if grepping for SVN_REPOS_DUMPFILE_PROP_CONTENT_LENGTH is not
that hard).

[...]
> +	/* Print the revprops now */
> +	SVN_ERR(svn_stream_write(stdout_stream, propstring->data,
> +				 &(propstring->len)));

Maybe:

	/* Property data. */
	SVN_ERR(svn_stream_write(stdout_stream, propstring->data,
				 &(propstring->len)));

The whole function so far has been about printing revprops.

> +
> +	svn_stream_close(stdout_stream);

This does not actually fclose(stdout), does it?

> @@ -39,6 +70,9 @@ static svn_error_t *replay_revend(svn_revnum_t revision,
>                                    apr_hash_t *rev_props,
>                                    apr_pool_t *pool)
>  {
> +	/* Editor has finished for this revision and close_edit has
> +	   been called; do nothing: just continue to the next
> +	   revision */

I’d leave out the comment, or:

	/* No resources to free. */

HTH,
Jonathan

Re: [PATCH 06/13] Dump the revprops at the start of every revision

[Ramkumar Ramachandra]

Hi Jonathan,

I stashed this review away while working on some other important
changes. I finally got around to responding to this review- sorry that
it took so long.

Jonathan Nieder writes:
> > Fill in replay_revstart to dump the revprops at the start of every
> > revision. Add an additional write_hash_to_stringbuf helper function.
> 
> 	A write_hash_to_stringbuf helper does the work of
> 	converting the property hashtable to dumpfile format.
> 
> > +++ b/dumpr_util.c
> [...]
> > +void write_hash_to_stringbuf(apr_hash_t *properties,
> > +			     svn_boolean_t deleted,
> > +			     svn_stringbuf_t **strbuf,
> > +			     apr_pool_t *pool)
[...]

Fixed, but not exactly in the way you've suggested.

> > +			/* Output name length, then name. */
> > +			svn_stringbuf_appendcstr(*strbuf,
> > +						 apr_psprintf(pool, "K %" APR_SSIZE_T_FMT "\n",
> > +							      keylen));
> > +
> > +			svn_stringbuf_appendbytes(*strbuf, (const char *) key, keylen);
> 
> Is the cast needed?  (The answer might be "yes" if this code is meant
> to be usable with C++ compilers.)

These casts are all over in the source tree, so I'm guessing the
answer is "yes".

> Style: better to say in comments what we are trying to do than what we
> actually do.  So:
> 
> 	/* First, dump revision properties. */

I've fixed all the comments in the entire source tree. Thanks :)

-- Ram

Re: [PATCH 06/13] Dump the revprops at the start of every revision

[Julian Foad]

On Thu, 2010-07-22, Ramkumar Ramachandra wrote:
> Jonathan Nieder writes:
[...]
> > > +			/* Output name length, then name. */
> > > +			svn_stringbuf_appendcstr(*strbuf,
> > > +						 apr_psprintf(pool, "K %" APR_SSIZE_T_FMT "\n",
> > > +							      keylen));
> > > +
> > > +			svn_stringbuf_appendbytes(*strbuf, (const char *) key, keylen);
> > 
> > Is the cast needed?  (The answer might be "yes" if this code is meant
> > to be usable with C++ compilers.)
> 
> These casts are all over in the source tree, so I'm guessing the
> answer is "yes".

Actually no - Subversion C code is not intended to be compilable as C++
and that cast is not needed.  (Other casts that you see in Subversion
code are for different situations.)

- Julian

Re: [PATCH 06/13] Dump the revprops at the start of every revision

[Ramkumar Ramachandra]

Hi Julian,

Julian Foad writes:
> On Thu, 2010-07-22, Ramkumar Ramachandra wrote:
> > Jonathan Nieder writes:
> [...]
> > > > +			/* Output name length, then name. */
> > > > +			svn_stringbuf_appendcstr(*strbuf,
> > > > +						 apr_psprintf(pool, "K %" APR_SSIZE_T_FMT "\n",
> > > > +							      keylen));
> > > > +
> > > > +			svn_stringbuf_appendbytes(*strbuf, (const char *) key, keylen);
> > > 
> > > Is the cast needed?  (The answer might be "yes" if this code is meant
> > > to be usable with C++ compilers.)
> > 
> > These casts are all over in the source tree, so I'm guessing the
> > answer is "yes".
> 
> Actually no - Subversion C code is not intended to be compilable as C++
> and that cast is not needed.  (Other casts that you see in Subversion
> code are for different situations.)

Thanks for pointing that out. I'll fix them in my future commits.

-- Ram

[PATCH 07/13] Implement open_root and close_edit

[Ramkumar Ramachandra]

open_root first creates a special edit_baton pool, and then allocates
memory from that pool to various items in edit_baton. Then it creates
a new directory baton to set as the root_baton. close_edit destroys
the special edit_baton pool (hence destroying all the items that were
allocated there), and increments current_rev. Add related
make_dir_baton function and dir_baton_t structure.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 dump_editor.c |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 dumpr_util.h  |   30 ++++++++++++++++++++++++
 2 files changed, 100 insertions(+), 0 deletions(-)

diff --git a/dump_editor.c b/dump_editor.c
index 70d6c0b..6e9b0f5 100644
--- a/dump_editor.c
+++ b/dump_editor.c
@@ -15,11 +15,77 @@
 
 #include "dumpr_util.h"
 
+
+/* Make a directory baton to represent the directory was path
+   (relative to EDIT_BATON's path) is PATH.
+
+   CMP_PATH/CMP_REV are the path/revision against which this directory
+   should be compared for changes.  If either is omitted (NULL for the
+   path, SVN_INVALID_REVNUM for the rev), just compare this directory
+   PATH against itself in the previous revision.
+
+   PARENT_DIR_BATON is the directory baton of this directory's parent,
+   or NULL if this is the top-level directory of the edit.  ADDED
+   indicated if this directory is newly added in this revision.
+   Perform all allocations in POOL.  */
+struct dir_baton *make_dir_baton(const char *path,
+                                 const char *cmp_path,
+                                 svn_revnum_t cmp_rev,
+                                 void *edit_baton,
+                                 void *parent_dir_baton,
+                                 svn_boolean_t added,
+                                 apr_pool_t *pool)
+{
+	struct edit_baton *eb = edit_baton;
+	struct dir_baton *pb = parent_dir_baton;
+	struct dir_baton *new_db = apr_pcalloc(pool, sizeof(*new_db));
+	const char *full_path;
+	apr_array_header_t *compose_path = apr_array_make(pool, 2, sizeof(const char *));
+
+	/* A path relative to nothing?  I don't think so. */
+	SVN_ERR_ASSERT_NO_RETURN(!path || pb);
+
+	/* Construct the full path of this node. */
+	if (pb) {
+		APR_ARRAY_PUSH(compose_path, const char *) = "/";
+		APR_ARRAY_PUSH(compose_path, const char *) = path;
+		full_path = svn_path_compose(compose_path, pool);
+	}
+	else
+		full_path = apr_pstrdup(pool, "/");
+
+	/* Remove leading slashes from copyfrom paths. */
+	if (cmp_path)
+		cmp_path = ((*cmp_path == '/') ? cmp_path + 1 : cmp_path);
+
+	new_db->eb = eb;
+	new_db->parent_dir_baton = pb;
+	new_db->path = full_path;
+	new_db->cmp_path = cmp_path ? apr_pstrdup(pool, cmp_path) : NULL;
+	new_db->cmp_rev = cmp_rev;
+	new_db->added = added;
+	new_db->written_out = FALSE;
+	new_db->deleted_entries = apr_hash_make(pool);
+	new_db->pool = pool;
+
+	return new_db;
+}
 svn_error_t *open_root(void *edit_baton,
                        svn_revnum_t base_revision,
                        apr_pool_t *pool,
                        void **root_baton)
 {
+	/* Allocate a special pool for the edit_baton to avoid pool
+	   lifetime issues */
+	struct edit_baton *eb = edit_baton;
+	eb->pool = svn_pool_create(pool);
+	eb->properties = apr_hash_make(eb->pool);
+	eb->del_properties = apr_hash_make(eb->pool);
+	eb->propstring = svn_stringbuf_create("", eb->pool);
+	eb->is_copy = FALSE;
+
+	*root_baton = make_dir_baton(NULL, NULL, SVN_INVALID_REVNUM,
+	                             edit_baton, NULL, FALSE, pool);
 	return SVN_NO_ERROR;
 }
 
@@ -108,6 +174,10 @@ svn_error_t *close_file(void *file_baton,
 
 svn_error_t *close_edit(void *edit_baton, apr_pool_t *pool)
 {
+	struct edit_baton *eb = edit_baton;
+	svn_pool_destroy(eb->pool);
+	(eb->current_rev) ++;
+
 	return SVN_NO_ERROR;
 }
 
diff --git a/dumpr_util.h b/dumpr_util.h
index 1a5752b..96670ff 100644
--- a/dumpr_util.h
+++ b/dumpr_util.h
@@ -31,6 +31,36 @@ struct edit_baton {
 	svn_checksum_t *checksum;
 };
 
+struct dir_baton {
+	struct edit_baton *eb;
+	struct dir_baton *parent_dir_baton;
+
+	/* is this directory a new addition to this revision? */
+	svn_boolean_t added;
+
+	/* has this directory been written to the output stream? */
+	svn_boolean_t written_out;
+
+	/* the absolute path to this directory */
+	const char *path;
+
+	/* the comparison path and revision of this directory.  if both of
+	   these are valid, use them as a source against which to compare
+	   the directory instead of the default comparison source of PATH in
+	   the previous revision. */
+	const char *cmp_path;
+	svn_revnum_t cmp_rev;
+
+	/* hash of paths that need to be deleted, though some -might- be
+	   replaced.  maps const char * paths to this dir_baton.  (they're
+	   full paths, because that's what the editor driver gives us.  but
+	   really, they're all within this directory.) */
+	apr_hash_t *deleted_entries;
+
+	/* pool to be used for deleting the hash items */
+	apr_pool_t *pool;
+};
+
 void write_hash_to_stringbuf(apr_hash_t *properties,
 			     svn_boolean_t deleted,
 			     svn_stringbuf_t **strbuf,
-- 
1.7.1

[PATCH 08/13] Implement dump_node

[Ramkumar Ramachandra]

Add a new dump_node function to dump node headers Node-path,
Node-kind, Node-action and set the right variables to trigger dumping
properties. Copies are also handled appropriately using eb->is_copy,
copyfrom_path, copyfrom_rev informatino from the caller. Also add a
related dump_props helper function that uses write_hash_to_stringbuf
to dump properties.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 dump_editor.c |  132 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 dumpr_util.c  |   48 +++++++++++++++++++++
 dumpr_util.h  |    5 ++
 3 files changed, 185 insertions(+), 0 deletions(-)

diff --git a/dump_editor.c b/dump_editor.c
index 6e9b0f5..0f7d231 100644
--- a/dump_editor.c
+++ b/dump_editor.c
@@ -15,6 +15,10 @@
 
 #include "dumpr_util.h"
 
+#define ARE_VALID_COPY_ARGS(p,r) ((p) && SVN_IS_VALID_REVNUM(r))
+
+svn_boolean_t must_dump_props = FALSE, must_dump_text = FALSE,
+	dump_props_pending = FALSE;
 
 /* Make a directory baton to represent the directory was path
    (relative to EDIT_BATON's path) is PATH.
@@ -70,6 +74,134 @@ struct dir_baton *make_dir_baton(const char *path,
 
 	return new_db;
 }
+/*
+ * Write out a node record for PATH of type KIND under EB->FS_ROOT.
+ * ACTION describes what is happening to the node (see enum svn_node_action).
+ * Write record to writable EB->STREAM, using EB->BUFFER to write in chunks.
+ *
+ * If the node was itself copied, IS_COPY is TRUE and the
+ * path/revision of the copy source are in CMP_PATH/CMP_REV.  If
+ * IS_COPY is FALSE, yet CMP_PATH/CMP_REV are valid, this node is part
+ * of a copied subtree.
+ */
+svn_error_t *dump_node(struct edit_baton *eb,
+		       const char *path,    /* an absolute path. */
+		       svn_node_kind_t kind,
+		       enum svn_node_action action,
+		       const char *cmp_path,
+		       svn_revnum_t cmp_rev,
+		       apr_pool_t *pool)
+{
+	/* Some pending properties to dump? */
+	SVN_ERR(dump_props(eb, &dump_props_pending, TRUE, pool));
+
+	/* Write out metadata headers for this file node. */
+	SVN_ERR(svn_stream_printf(eb->stream, pool,
+				  SVN_REPOS_DUMPFILE_NODE_PATH ": %s\n",
+				  (*path == '/') ? path + 1 : path));
+
+	if (kind == svn_node_file)
+		SVN_ERR(svn_stream_printf(eb->stream, pool,
+		                          SVN_REPOS_DUMPFILE_NODE_KIND ": file\n"));
+	else if (kind == svn_node_dir)
+		SVN_ERR(svn_stream_printf(eb->stream, pool,
+		                          SVN_REPOS_DUMPFILE_NODE_KIND ": dir\n"));
+
+	/* Remove leading slashes from copyfrom paths. */
+	if (cmp_path)
+		cmp_path = ((*cmp_path == '/') ? cmp_path + 1 : cmp_path);
+
+	switch (action) {
+		/* Appropriately handle the four svn_node_action actions */
+
+	case svn_node_action_change:
+		SVN_ERR(svn_stream_printf(eb->stream, pool,
+		                          SVN_REPOS_DUMPFILE_NODE_ACTION
+		                          ": change\n"));
+		break;
+
+	case svn_node_action_replace:
+		if (!eb->is_copy) {
+			/* a simple delete+add, implied by a single 'replace' action. */
+			SVN_ERR(svn_stream_printf(eb->stream, pool,
+			                          SVN_REPOS_DUMPFILE_NODE_ACTION
+			                          ": replace\n"));
+
+			/* definitely need to dump all content for a replace. */
+			must_dump_props = TRUE;
+			break;
+		}
+		/* More complex case: eb->is_copy is true, and
+		   cmp_path/ cmp_rev are present: delete the original,
+		   and then re-add it */
+
+		/* the path & kind headers have already been printed;  just
+		   add a delete action, and end the current record.*/
+		SVN_ERR(svn_stream_printf(eb->stream, pool,
+		                          SVN_REPOS_DUMPFILE_NODE_ACTION
+		                          ": delete\n\n"));
+
+		/* recurse:  print an additional add-with-history record. */
+		SVN_ERR(dump_node(eb, path, kind, svn_node_action_add,
+		                  cmp_path, cmp_rev, pool));
+
+		/* we can leave this routine quietly now, don't need to dump
+		   any content;  that was already done in the second record. */
+		must_dump_props = FALSE;
+		eb->is_copy = FALSE;
+		break;
+
+	case svn_node_action_delete:
+		SVN_ERR(svn_stream_printf(eb->stream, pool,
+		                          SVN_REPOS_DUMPFILE_NODE_ACTION
+		                          ": delete\n"));
+
+		/* we can leave this routine quietly now, don't need to dump
+		   any content. */
+		SVN_ERR(svn_stream_printf(eb->stream, pool, "\n\n"));
+		must_dump_props = FALSE;
+		break;
+
+	case svn_node_action_add:
+		SVN_ERR(svn_stream_printf(eb->stream, pool,
+		                          SVN_REPOS_DUMPFILE_NODE_ACTION ": add\n"));
+
+		if (!eb->is_copy) {
+			/* If it's a file or directory not copied from
+			   somewhere, wait for change_file_prop or
+			   change_directory_prop */
+			dump_props_pending = TRUE;
+			break;
+		}
+
+		SVN_ERR(svn_stream_printf(eb->stream, pool,
+		                          SVN_REPOS_DUMPFILE_NODE_COPYFROM_REV
+		                          ": %ld\n"
+		                          SVN_REPOS_DUMPFILE_NODE_COPYFROM_PATH
+		                          ": %s\n",
+		                          cmp_rev, cmp_path));
+
+		/* Dump the text only if apply_textdelta sets
+		   must_dump_text */
+
+		/* UGLY hack: If a directory was copied from a
+		   previous revision, nothing else can be done, and
+		   close_file won't be called to write two blank
+		   lines; write them here */
+		if (kind == svn_node_dir)
+			SVN_ERR(svn_stream_printf(eb->stream, pool, "\n\n"));
+
+		eb->is_copy = FALSE;
+
+		break;
+	}
+
+	/* Dump property headers */
+	SVN_ERR(dump_props(eb, &must_dump_props, FALSE, pool));
+
+	return SVN_NO_ERROR;
+}
+
 svn_error_t *open_root(void *edit_baton,
                        svn_revnum_t base_revision,
                        apr_pool_t *pool,
diff --git a/dumpr_util.c b/dumpr_util.c
index 41940d4..a3328b6 100644
--- a/dumpr_util.c
+++ b/dumpr_util.c
@@ -62,3 +62,51 @@ void write_hash_to_stringbuf(apr_hash_t *properties,
 		}
 	}
 }
+
+svn_error_t *dump_props(struct edit_baton *eb,
+			svn_boolean_t *trigger_var,
+			svn_boolean_t dump_data_too,
+			apr_pool_t *pool)
+{
+	if (trigger_var && !*trigger_var)
+		return SVN_NO_ERROR;
+
+	/* Build a propstring to print */
+	svn_stringbuf_setempty(eb->propstring);
+	write_hash_to_stringbuf(eb->properties,
+				FALSE,
+				&(eb->propstring), eb->pool);
+	write_hash_to_stringbuf(eb->del_properties,
+				TRUE,
+				&(eb->propstring), eb->pool);
+	svn_stringbuf_appendbytes(eb->propstring, "PROPS-END\n", 10);
+
+	/* prop-delta header */
+	SVN_ERR(svn_stream_printf(eb->stream, pool,
+				  SVN_REPOS_DUMPFILE_PROP_DELTA
+				  ": true\n"));
+
+	/* prop-content-length header */
+	SVN_ERR(svn_stream_printf(eb->stream, pool,
+				  SVN_REPOS_DUMPFILE_PROP_CONTENT_LENGTH
+				  ": %" APR_SIZE_T_FMT "\n", eb->propstring->len));
+
+	if (dump_data_too) {
+		/* content-length header */
+		SVN_ERR(svn_stream_printf(eb->stream, pool,
+					  SVN_REPOS_DUMPFILE_CONTENT_LENGTH
+					  ": %" APR_SIZE_T_FMT "\n\n",
+					  eb->propstring->len));
+
+		/* the properties themselves */
+		SVN_ERR(svn_stream_write(eb->stream, eb->propstring->data,
+					 &(eb->propstring->len)));
+
+		/* Cleanup so that data is never dumped twice */
+		apr_hash_clear(eb->properties);
+		apr_hash_clear(eb->del_properties);
+		if (trigger_var)
+			*trigger_var = FALSE;
+	}
+	return SVN_NO_ERROR;
+}
diff --git a/dumpr_util.h b/dumpr_util.h
index 96670ff..79de1ab 100644
--- a/dumpr_util.h
+++ b/dumpr_util.h
@@ -66,4 +66,9 @@ void write_hash_to_stringbuf(apr_hash_t *properties,
 			     svn_stringbuf_t **strbuf,
 			     apr_pool_t *pool);
 
+svn_error_t *dump_props(struct edit_baton *eb,
+			svn_boolean_t *trigger_var,
+			svn_boolean_t dump_data_too,
+			apr_pool_t *pool);
+
 #endif
-- 
1.7.1

[PATCH 09/13] Implement directory-related functions

[Ramkumar Ramachandra]

Implement open_directory, add_directory, change_dir_prop and
close_directory. All of them involve adding and removing entries from
the directory_baton and dumping the related node information. Note
that open_directory doesn't need any corresponding node information to
be printed.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 dump_editor.c |  101 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 101 insertions(+), 0 deletions(-)

diff --git a/dump_editor.c b/dump_editor.c
index 0f7d231..6077e5c 100644
--- a/dump_editor.c
+++ b/dump_editor.c
@@ -226,6 +226,12 @@ svn_error_t *delete_entry(const char *path,
                           void *parent_baton,
                           apr_pool_t *pool)
 {
+	struct dir_baton *pb = parent_baton;
+	const char *mypath = apr_pstrdup(pb->pool, path);
+
+	/* remember this path needs to be deleted */
+	apr_hash_set(pb->deleted_entries, mypath, APR_HASH_KEY_STRING, pb);
+
 	return SVN_NO_ERROR;
 }
 
@@ -236,6 +242,32 @@ svn_error_t *add_directory(const char *path,
                            apr_pool_t *pool,
                            void **child_baton)
 {
+	struct dir_baton *pb = parent_baton;
+	void *val;
+	struct dir_baton *new_db
+		= make_dir_baton(path, copyfrom_path, copyfrom_rev, pb->eb, pb, TRUE, pool);
+
+	/* This might be a replacement -- is the path already deleted? */
+	val = apr_hash_get(pb->deleted_entries, path, APR_HASH_KEY_STRING);
+
+	/* Detect an add-with-history */
+	pb->eb->is_copy = ARE_VALID_COPY_ARGS(copyfrom_path, copyfrom_rev);
+
+	/* Dump the node */
+	SVN_ERR(dump_node(pb->eb, path,
+	                  svn_node_dir,
+	                  val ? svn_node_action_replace : svn_node_action_add,
+	                  pb->eb->is_copy ? copyfrom_path : NULL,
+	                  pb->eb->is_copy ? copyfrom_rev : SVN_INVALID_REVNUM,
+	                  pool));
+
+	if (val)
+		/* Delete the path, it's now been dumped */
+		apr_hash_set(pb->deleted_entries, path, APR_HASH_KEY_STRING, NULL);
+
+	new_db->written_out = TRUE;
+
+	*child_baton = new_db;
 	return SVN_NO_ERROR;
 }
 
@@ -245,12 +277,53 @@ svn_error_t *open_directory(const char *path,
                             apr_pool_t *pool,
                             void **child_baton)
 {
+	struct dir_baton *pb = parent_baton;
+	struct dir_baton *new_db;
+	const char *cmp_path = NULL;
+	svn_revnum_t cmp_rev = SVN_INVALID_REVNUM;
+	apr_array_header_t *compose_path = apr_array_make(pool, 2, sizeof(const char *));
+
+	/* If the parent directory has explicit comparison path and rev,
+	   record the same for this one. */
+	if (pb && ARE_VALID_COPY_ARGS(pb->cmp_path, pb->cmp_rev)) {
+		APR_ARRAY_PUSH(compose_path, const char *) = pb->cmp_path;
+		APR_ARRAY_PUSH(compose_path, const char *) = svn_dirent_basename(path, pool);
+		cmp_path = svn_path_compose(compose_path, pool);
+		cmp_rev = pb->cmp_rev;
+	}
+
+	new_db = make_dir_baton(path, cmp_path, cmp_rev, pb->eb, pb, FALSE, pool);
+	*child_baton = new_db;
 	return SVN_NO_ERROR;
 }
 
 svn_error_t *close_directory(void *dir_baton,
                              apr_pool_t *pool)
 {
+	struct dir_baton *db = dir_baton;
+	struct edit_baton *eb = db->eb;
+	apr_hash_index_t *hi;
+	apr_pool_t *subpool = svn_pool_create(pool);
+
+	/* Some pending properties to dump? */
+	SVN_ERR(dump_props(eb, &dump_props_pending, TRUE, pool));
+
+	/* Dump the directory entries */
+	for (hi = apr_hash_first(pool, db->deleted_entries); hi;
+	     hi = apr_hash_next(hi)) {
+		const void *key;
+		const char *path;
+		apr_hash_this(hi, &key, NULL, NULL);
+		path = key;
+
+		svn_pool_clear(subpool);
+
+		SVN_ERR(dump_node(db->eb, path,
+		                  svn_node_unknown, svn_node_action_delete,
+		                  NULL, SVN_INVALID_REVNUM, subpool));
+	}
+
+	svn_pool_destroy(subpool);
 	return SVN_NO_ERROR;
 }
 
@@ -278,6 +351,34 @@ svn_error_t *change_dir_prop(void *parent_baton,
                              const svn_string_t *value,
                              apr_pool_t *pool)
 {
+	struct dir_baton *db = parent_baton;
+
+	if (svn_property_kind(NULL, name) != svn_prop_regular_kind)
+		return SVN_NO_ERROR;
+
+	value ? apr_hash_set(db->eb->properties, apr_pstrdup(pool, name),
+	                     APR_HASH_KEY_STRING, svn_string_dup(value, pool)) :
+		apr_hash_set(db->eb->del_properties, apr_pstrdup(pool, name),
+		             APR_HASH_KEY_STRING, (void *)0x1);
+
+	/* This function is what distinguishes between a directory that is
+	   opened to merely get somewhere, vs. one that is opened because it
+	   actually changed by itself  */
+	if (! db->written_out) {
+		SVN_ERR(dump_node(db->eb, db->path,
+		                  svn_node_dir, svn_node_action_change,
+		                  db->cmp_path, db->cmp_rev, pool));
+
+		/* If dump_props_pending was set, it means that the
+		   node information corresponding to add_directory has already
+		   been written; just don't unset it and dump_node will dump
+		   the properties before doing anything else. If it wasn't
+		   set, node information hasn't been written yet: so dump the
+		   node itself before dumping the props */
+
+		SVN_ERR(dump_props(db->eb, NULL, TRUE, pool));
+		db->written_out = TRUE;
+	}
 	return SVN_NO_ERROR;
 }
 
-- 
1.7.1

[PATCH 10/13] Implement file-related functions

[Ramkumar Ramachandra]

Implement add_file, open_file and change_file_prop. All of them
involve dumping the corresponding node information and setting up the
file_baton for apply_textdelta and close_file to use.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 dump_editor.c |   62 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/dump_editor.c b/dump_editor.c
index 6077e5c..7006a2c 100644
--- a/dump_editor.c
+++ b/dump_editor.c
@@ -334,6 +334,31 @@ svn_error_t *add_file(const char *path,
                       apr_pool_t *pool,
                       void **file_baton)
 {
+	struct dir_baton *pb = parent_baton;
+	void *val;
+
+	/* This might be a replacement -- is the path already deleted? */
+	val = apr_hash_get(pb->deleted_entries, path, APR_HASH_KEY_STRING);
+
+	/* Detect add-with-history. */
+	pb->eb->is_copy = ARE_VALID_COPY_ARGS(copyfrom_path, copyfrom_rev);
+
+	/* Dump the node. */
+	SVN_ERR(dump_node(pb->eb, path,
+	                  svn_node_file,
+	                  val ? svn_node_action_replace : svn_node_action_add,
+	                  pb->eb->is_copy ? copyfrom_path : NULL,
+	                  pb->eb->is_copy ? copyfrom_rev : SVN_INVALID_REVNUM,
+	                  pool));
+
+	if (val)
+		/* delete the path, it's now been dumped. */
+		apr_hash_set(pb->deleted_entries, path, APR_HASH_KEY_STRING, NULL);
+
+	/* Build a nice file baton to pass to change_file_prop and apply_textdelta */
+	pb->eb->changed_path = path;
+	*file_baton = pb->eb;
+
 	return SVN_NO_ERROR;
 }
 
@@ -343,6 +368,28 @@ svn_error_t *open_file(const char *path,
                        apr_pool_t *pool,
                        void **file_baton)
 {
+	struct dir_baton *pb = parent_baton;
+	const char *cmp_path = NULL;
+	svn_revnum_t cmp_rev = SVN_INVALID_REVNUM;
+
+	apr_array_header_t *compose_path = apr_array_make(pool, 2, sizeof(const char *));
+	/* If the parent directory has explicit comparison path and rev,
+	   record the same for this one. */
+	if (pb && ARE_VALID_COPY_ARGS(pb->cmp_path, pb->cmp_rev)) {
+		APR_ARRAY_PUSH(compose_path, const char *) = pb->cmp_path;
+		APR_ARRAY_PUSH(compose_path, const char *) = svn_dirent_basename(path, pool);
+		cmp_path = svn_path_compose(compose_path, pool);
+		cmp_rev = pb->cmp_rev;
+	}
+
+	SVN_ERR(dump_node(pb->eb, path,
+	                  svn_node_file, svn_node_action_change,
+	                  cmp_path, cmp_rev, pool));
+
+	/* Build a nice file baton to pass to change_file_prop and apply_textdelta */
+	pb->eb->changed_path = path;
+	*file_baton = pb->eb;
+
 	return SVN_NO_ERROR;
 }
 
@@ -387,6 +434,21 @@ svn_error_t *change_file_prop(void *file_baton,
                               const svn_string_t *value,
                               apr_pool_t *pool)
 {
+	struct edit_baton *eb = file_baton;
+
+	if (svn_property_kind(NULL, name) != svn_prop_regular_kind)
+		return SVN_NO_ERROR;
+
+	value ? apr_hash_set(eb->properties, apr_pstrdup(pool, name),
+	                     APR_HASH_KEY_STRING, svn_string_dup(value, pool)) :
+		apr_hash_set(eb->del_properties, apr_pstrdup(pool, name),
+		             APR_HASH_KEY_STRING, (void *)0x1);
+
+	/* Dump the property headers and wait; close_file might need
+	   to write text headers too depending on whether
+	   apply_textdelta is called */
+	dump_props_pending = TRUE;
+
 	return SVN_NO_ERROR;
 }
 
-- 
1.7.1

[PATCH 11/13] Implement apply_textdelta

[Ramkumar Ramachandra]

apply_textdelta picks up information from the file_baton (set by
add_file or open_file) and sets handler/ handler_baton to write a text
delta. It uses a temporary file because the content length of the
delta needs to be determined. Also add a related window_handler helper
function gets the path of the temporary file written and cleans up
after apply_textdelta. The text and prop deltas will finally be dumped
in close_file.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 dump_editor.c |   60 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 dumpr_util.h  |   15 ++++++++++++++
 2 files changed, 74 insertions(+), 1 deletions(-)

diff --git a/dump_editor.c b/dump_editor.c
index 7006a2c..51d5ebe 100644
--- a/dump_editor.c
+++ b/dump_editor.c
@@ -452,11 +452,69 @@ svn_error_t *change_file_prop(void *file_baton,
 	return SVN_NO_ERROR;
 }
 
+svn_error_t *window_handler(svn_txdelta_window_t *window, void *baton)
+{
+	struct handler_baton *hb = baton;
+	struct edit_baton *eb = hb->eb;
+	svn_error_t *err;
+
+	err = hb->apply_handler(window, hb->apply_baton);
+	if (window != NULL && !err)
+		return SVN_NO_ERROR;
+
+	if (err)
+		SVN_ERR(err);
+
+	/* Write information about the filepath to hb->eb */
+	eb->temp_filepath = apr_pstrdup(eb->pool,
+					hb->temp_filepath);
+
+	/* Cleanup */
+	SVN_ERR(svn_io_file_close(hb->temp_file, hb->pool));
+	SVN_ERR(svn_stream_close(hb->temp_filestream));
+	svn_pool_destroy(hb->pool);
+	return SVN_NO_ERROR;
+}
+
 svn_error_t *apply_textdelta(void *file_baton, const char *base_checksum,
                              apr_pool_t *pool,
                              svn_txdelta_window_handler_t *handler,
                              void **handler_baton)
 {
+	svn_stream_t *source_stream;
+	struct edit_baton *eb = file_baton;
+	apr_status_t apr_err;
+
+	/* Custom handler_baton allocated in a separate pool */
+	apr_pool_t *handler_pool = svn_pool_create(pool);
+	struct handler_baton *hb = apr_pcalloc(handler_pool, sizeof(*hb));
+	hb->pool = handler_pool;
+	hb->eb = eb;
+
+	/* Unset both handlers: To be set by two different functions later */
+	hb->apply_handler = NULL;
+
+	/* Use a temporary file to measure the text-content-length */
+	hb->temp_filepath = apr_psprintf(eb->pool, "/tmp/svn-fe/XXXXXX");
+	apr_err = apr_file_mktemp(&(hb->temp_file), hb->temp_filepath,
+				  APR_CREATE | APR_READ | APR_WRITE | APR_EXCL,
+				  hb->pool);
+	if (apr_err != APR_SUCCESS)
+		SVN_ERR(svn_error_wrap_apr(apr_err, NULL));
+
+	hb->temp_filestream = svn_stream_from_aprfile2(hb->temp_file, TRUE, hb->pool);
+	source_stream = svn_stream_empty(hb->pool);
+
+	/* Prepare to write the delta to the temporary file */
+	svn_txdelta_to_svndiff2(&(hb->apply_handler), &(hb->apply_baton),
+	                        hb->temp_filestream, 0, hb->pool);
+	must_dump_text = TRUE;
+
+	/* The actual writing takes place when this function has finished */
+	/* Set the handler and handler_baton */
+	*handler = window_handler;
+	*handler_baton = hb;
+
 	return SVN_NO_ERROR;
 }
 
@@ -493,7 +551,7 @@ svn_error_t *get_dump_editor(const svn_delta_editor_t **editor,
 	de->close_directory = close_directory;
 	de->change_dir_prop = change_dir_prop;
 	de->change_file_prop = change_file_prop;
-	/* de->apply_textdelta = apply_textdelta; */
+	de->apply_textdelta = apply_textdelta;
 	de->add_file = add_file;
 	de->open_file = open_file;
 	de->close_file = close_file;
diff --git a/dumpr_util.h b/dumpr_util.h
index 79de1ab..3830a1d 100644
--- a/dumpr_util.h
+++ b/dumpr_util.h
@@ -61,6 +61,21 @@ struct dir_baton {
 	apr_pool_t *pool;
 };
 
+struct handler_baton
+{
+	svn_txdelta_window_handler_t apply_handler;
+	void *apply_baton;
+	apr_pool_t *pool;
+
+	/* Information about the path of the tempoarary file used */
+	char *temp_filepath;
+	apr_file_t *temp_file;
+	svn_stream_t *temp_filestream;
+
+	/* To fill in the edit baton fields */
+	struct edit_baton *eb;
+};
+
 void write_hash_to_stringbuf(apr_hash_t *properties,
 			     svn_boolean_t deleted,
 			     svn_stringbuf_t **strbuf,
-- 
1.7.1

[PATCH 12/13] Implement close_file

[Ramkumar Ramachandra]

close_file measures the length of the temporary file to write text
headers and full text before cleaning up the temporary file. It also
writes props and prop deltas if necessary.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 dump_editor.c |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 76 insertions(+), 0 deletions(-)

diff --git a/dump_editor.c b/dump_editor.c
index 51d5ebe..6f74af9 100644
--- a/dump_editor.c
+++ b/dump_editor.c
@@ -522,6 +522,82 @@ svn_error_t *close_file(void *file_baton,
 			const char *text_checksum,
 			apr_pool_t *pool)
 {
+	struct edit_baton *eb = file_baton;
+	apr_file_t *temp_file;
+	svn_stream_t *temp_filestream;
+	apr_finfo_t *info = apr_pcalloc(pool, sizeof(apr_finfo_t));
+
+	/* We didn't write the property headers because we were
+	   waiting for file_prop_change; write them now */
+	SVN_ERR(dump_props(eb, &dump_props_pending, FALSE, pool));
+
+	/* The prop headers have already been dumped in dump_node */
+	/* Dump the text headers */
+	if (must_dump_text) {
+		/* text-delta header */
+		SVN_ERR(svn_stream_printf(eb->stream, pool,
+					  SVN_REPOS_DUMPFILE_TEXT_DELTA
+					  ": true\n"));
+
+		/* Measure the length */
+		SVN_ERR(svn_io_stat(info, eb->temp_filepath, APR_FINFO_SIZE, pool));
+
+		/* text-content-length header */
+		SVN_ERR(svn_stream_printf(eb->stream, pool,
+					  SVN_REPOS_DUMPFILE_TEXT_CONTENT_LENGTH
+					  ": %lu\n",
+					  (unsigned long)info->size));
+		/* text-content-md5 header */
+		SVN_ERR(svn_stream_printf(eb->stream, pool,
+					  SVN_REPOS_DUMPFILE_TEXT_CONTENT_MD5
+					   ": %s\n",
+					  text_checksum));
+	}
+
+	/* content-length header: if both text and props are absent,
+	   skip this block */
+	if (must_dump_props || dump_props_pending)
+		SVN_ERR(svn_stream_printf(eb->stream, pool,
+					  SVN_REPOS_DUMPFILE_CONTENT_LENGTH
+					  ": %ld\n\n",
+					  (unsigned long)info->size + eb->propstring->len));
+	else if (must_dump_text)
+		SVN_ERR(svn_stream_printf(eb->stream, pool,
+					  SVN_REPOS_DUMPFILE_CONTENT_LENGTH
+					  ": %ld\n\n",
+					  (unsigned long)info->size));
+
+	/* Dump the props; the propstring should have already been
+	   written in dump_node or above */
+	if (must_dump_props || dump_props_pending) {
+		SVN_ERR(svn_stream_write(eb->stream, eb->propstring->data,
+					 &(eb->propstring->len)));
+
+		/* Cleanup */
+		must_dump_props = dump_props_pending = FALSE;
+		apr_hash_clear(eb->properties);
+		apr_hash_clear(eb->del_properties);
+	}
+
+	/* Dump the text */
+	if (must_dump_text) {
+
+		/* Open the temporary file, map it to a stream, copy
+		   the stream to eb->stream, close and delete the
+		   file */
+		SVN_ERR(svn_io_file_open(&temp_file, eb->temp_filepath, APR_READ, 0600, pool));
+		temp_filestream = svn_stream_from_aprfile2(temp_file, TRUE, pool);
+		SVN_ERR(svn_stream_copy3(temp_filestream, eb->stream, NULL, NULL, pool));
+
+		/* Cleanup */
+		SVN_ERR(svn_io_file_close(temp_file, pool));
+		SVN_ERR(svn_stream_close(temp_filestream));
+		SVN_ERR(svn_io_remove_file2(eb->temp_filepath, TRUE, pool));
+		must_dump_text = FALSE;
+	}
+
+	SVN_ERR(svn_stream_printf(eb->stream, pool, "\n\n"));
+
 	return SVN_NO_ERROR;
 }
 
-- 
1.7.1

[PATCH 13/13] Add a validation script

[Ramkumar Ramachandra]

Add a validation script along with a .gitignore. Using an existing
dump known to be correct (possibly generated using `svnsync` and
`svnadmin dump --deltas`), it compares the outputs produced by
`svnadmin load` when fed with this dump and the dump from the program.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 .gitignore  |    4 ++++
 validate.sh |   50 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 0 deletions(-)
 create mode 100644 .gitignore
 create mode 100755 validate.sh

diff --git a/.gitignore b/.gitignore
new file mode 100644
index 0000000..3e9b906
--- /dev/null
+++ b/.gitignore
@@ -0,0 +1,4 @@
+*~
+t/
+svndumpr
+svndumpr_bench
diff --git a/validate.sh b/validate.sh
new file mode 100755
index 0000000..7b25db6
--- /dev/null
+++ b/validate.sh
@@ -0,0 +1,50 @@
+#!/bin/sh
+# asf.dump must exist in t/
+# Compile the program with end_revision = $2 when using the second branch
+
+case $1 in
+    generate)
+	[ -z $2 ] && { echo "Usage: $0 $1 <revision>"; exit 1; } || echo "Starting generation ...";
+	if test -e "t/asf.dump"
+	    then :;
+	else
+	    echo "Need t/asf.dump (dumpfile v3) first. Generate it yourself or steal it from someone.";
+	    exit 1;
+	fi
+	rm -rf t/repo;
+	mkdir t/repo;
+	svnadmin create t/repo;
+	gawk "/Revision-number: $(($2 + 1))/ { exit 1 }; { print \$0 };" t/asf.dump > "t/asf-$2.dump";
+	[ $? = 1 ] && echo "Cut $2 succeeded!" || { echo "Cut $2 failed. Check t/asf.dump and validate.sh."; exit 1; }
+	svnadmin load t/repo < "t/asf-$2.dump" 1>"t/asf-$2-import.log" 2>"t/asf-$2-import.error";
+	[ $? = 0 ] && echo "Load $2 succeeded!" || { echo "Load $2 failed. See t/asf-$2-import.debug for details."; exit 1; }
+	echo "Successfully generated asf-$2.dump and asf-$2-import.log. You can now run validate.sh validate $2"
+	exit 0;;
+    validate)
+	[ -z $2 ] && { echo "Usage: $0 $1 <revision>"; exit 1; } || echo "Starting validation ...";
+	if test -e "t/asf-$2.dump" && test -e "t/asf-$2-import.log"
+	    then :;
+	else
+	    echo "Run validate.sh genereate $2 first.";
+	    exit 1;
+	fi
+	rm -rf /tmp/svn-fe;
+	mkdir /tmp/svn-fe;
+	rm -rf t/repo;
+	mkdir t/repo;
+	svnadmin create t/repo;
+	make svndumpr > /dev/null;
+	[ $? = 0 ] && echo "Make succeeded!" || { echo "Make failed. Check the program."; exit 1; }
+	./svndumpr 1>t/asf-mine.dump;
+	[ $? = 0 ] && echo "Run succeeded!" || { echo "Run failed. See t/asf.debug for details."; exit 1; }
+	diff -au "t/asf-$2.dump" t/asf-mine.dump > t/dump-diff.error;
+	gawk '$0 !~ "Prop-delta: true|Text-delta-base-|sha1|Text-copy-source-|^-$" && $0 ~ "^+|^-" { print; }' t/dump-diff.error > t/dump-diff-filtered.error;
+	svnadmin load t/repo < t/asf-mine.dump 1>t/asf-mine-import.log 2>t/asf-mine-import.error;
+	[ $? = 0 ] && echo "Load $2 succeeded!" || { echo "Load failed. See t/asf-mine-import.error, t/dump-diff.error, and t/dump-diff-filtered.error for details."; exit 1; }
+	diff -au "t/asf-$2-import.log" t/asf-mine-import.log > t/import-diff.error;
+	[ $? = 0 ] && echo "Validation $2 succeeded!" || { echo "Validation failed. See t/import-diff.error for details."; exit 1; }
+	exit 0;;
+    *)
+	echo "Usage: $0 <operation> <revision>";
+	exit 1;;
+esac
-- 
1.7.1

Re: [GSoC update] git-remote-svn: Week 10

[Ramkumar Ramachandra]

Hi,

Ramkumar Ramachandra writes:
> Please note that it has been built and tested only against the
> Subversion trunk: for Subversion 1.6, you can try using my
> ra-svn-1.6. Also, there seems to be some unresolved issue on 64-bit
> systems. We're working on fixing this.

This is fixed. I'm currently running a large validation on Avar's
server.

NOTE to reviewers: The must_dump_props and dump_props_pending
variables in patches 7-12 are very confusing. I've fixed this and
added a long explanatory note in the latest commits. Please ignore it
for the moment and review the rest of the series.

-- Ram