Re: [PATCH 1/9] vcs-svn: pass paths through to fast-import

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

David Barr wrote:

>  vcs-svn/fast_export.c |   47 ++++++++++++++++++------------------
>  vcs-svn/fast_export.h |    9 +++----
>  vcs-svn/repo_tree.c   |   20 +++++++-------
>  vcs-svn/repo_tree.h   |   13 ++++------
>  vcs-svn/svndump.c     |   63 +++++++++++++++++++++----------------------------
>  5 files changed, 70 insertions(+), 82 deletions(-)

Hoorah!  Simpler and more idiomatic.

> +++ b/vcs-svn/fast_export.c
> @@ -32,30 +34,30 @@ void fast_export_reset(void)
[...]
>  	buffer_reset(&report_buffer);
>  }
>  
> -void fast_export_delete(uint32_t depth, const uint32_t *path)
> +void fast_export_delete(const char *path)
>  {
> -	printf("D \"");
> -	pool_print_seq_q(depth, path, '/', stdout);
> -	printf("\"\n");
> +	putchar('D');
> +	putchar(' ');
> +	quote_c_style(path, NULL, stdout, 0);
> +	putchar('\n');
>  }

Functional change: if the path doesn't need quoting, this won't
surround it with quotation marks.  Luckily fast-import doesn't
mind.

[...]
> -	printf("M %06"PRIo32" %s \"", mode, dataref);
> -	pool_print_seq_q(depth, path, '/', stdout);
> -	printf("\"\n");
> +	printf("M %06"PRIo32" %s ", mode, dataref);
> +	quote_c_style(path, NULL, stdout, 0);
> +	putchar('\n');
[...]
> -	printf("ls :%"PRIu32" \"", rev);
> -	pool_print_seq_q(depth, path, '/', stdout);
> -	printf("\"\n");
> +	printf("ls :%"PRIu32" ", rev);
> +	quote_c_style(path, NULL, stdout, 0);
> +	putchar('\n');

Likewise.

[...]
> -static void ls_from_active_commit(uint32_t depth, const uint32_t *path)
> +static void ls_from_active_commit(const char *path)
>  {
>  	/* ls "path/to/file" */
>  	printf("ls \"");
> -	pool_print_seq_q(depth, path, '/', stdout);
> +	quote_c_style(path, NULL, stdout, 1);
>  	printf("\"\n");

Single-argument 'ls': quotes always present.  Phew.

[...]
> --- a/vcs-svn/repo_tree.h
> +++ b/vcs-svn/repo_tree.h
> @@ -8,15 +8,12 @@
>  #define REPO_MODE_EXE 0100755
>  #define REPO_MODE_LNK 0120000
>  
> -#define REPO_MAX_PATH_LEN 4096
> -#define REPO_MAX_PATH_DEPTH 1000

Yes.

> --- a/vcs-svn/svndump.c
> +++ b/vcs-svn/svndump.c
> @@ -11,8 +11,8 @@
>  #include "repo_tree.h"
>  #include "fast_export.h"
>  #include "line_buffer.h"
> -#include "obj_pool.h"
>  #include "string_pool.h"
> +#include "strbuf.h"
>  
>  #define REPORT_FILENO 3
>  
> @@ -31,32 +31,20 @@
>  #define LENGTH_UNKNOWN (~0)
>  #define DATE_RFC2822_LEN 31
>  
> -/* Create memory pool for log messages */
> -obj_pool_gen(log, char, 4096)
> -

Not a path. :)  Snuck in from a separate patch?

>  static struct line_buffer input = LINE_BUFFER_INIT;
>  
>  #define REPORT_FILENO 3
>  
> -static char *log_copy(uint32_t length, const char *log)
> -{
[...]
> -}

Likewise.

[...]
>  static struct {
>  	uint32_t revision, author;
>  	unsigned long timestamp;
> -	char *log;
> +	struct strbuf log;
>  } rev_ctx;

Likewise.

[... etc ...]
> @@ -406,6 +395,9 @@ int svndump_init(const char *filename)
>  	if (buffer_init(&input, filename))
>  		return error("cannot open %s: %s", filename, strerror(errno));
>  	fast_export_init(REPORT_FILENO);
> +	strbuf_init(&rev_ctx.log, 4096);
> +	strbuf_init(&node_ctx.src, 4096);
> +	strbuf_init(&node_ctx.dst, 4096);

4096 because PATH_MAX or some other reason?

> @@ -415,11 +407,13 @@ int svndump_init(const char *filename)
>
>  void svndump_deinit(void)
>  {
> -	log_reset();
>  	fast_export_deinit();
>  	reset_dump_ctx(~0);
>  	reset_rev_ctx(0);
>  	reset_node_ctx(NULL);
> +	strbuf_release(&rev_ctx.log);
> +	strbuf_release(&node_ctx.src);
> +	strbuf_release(&node_ctx.dst);

Side note: it's often not clear what should go in the "prepare for next
user" routine and what should go in the "shutting down for good".  I
suppose these should use strbuf_reset and the memory would be finally
freed in svndump_reset?  Does it make sense to have two distinct
routines like this without a user to demonstrate the trade-offs?

Except as noted above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks; I like where this is going.