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@xxxxxxxxx>
Thanks; I like where this is going.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html