Re: [PATCH 1/9] vcs-svn: pass paths through to fast-import
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Mar 19, 2011, 07:50 UTC
- Message-ID
- <20110319075028.GB4063@elie>
- In-Reply-To
- <1300518231-20008-2-git-send-email-david.barr@cordelta.com>
Hi,
David Barr wrote:
Show 6 quoted lines
> 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)
[...]
Show 14 quoted lines
> 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.
[...]
Show 6 quoted lines
> - 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');[...]
Show 6 quoted lines
> - 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.
[...]
Show 8 quoted lines
> -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.
[...]
Show 8 quoted lines
> --- 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.
Show 19 quoted lines
> --- 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?
Show 6 quoted lines
> static struct line_buffer input = LINE_BUFFER_INIT;
>
> #define REPORT_FILENO 3
>
> -static char *log_copy(uint32_t length, const char *log)
> -{[...]
> -}
Likewise.
[...]
Show 6 quoted lines
> static struct {
> uint32_t revision, author;
> unsigned long timestamp;
> - char *log;
> + struct strbuf log;
> } rev_ctx;Likewise.
[... etc ...]
Show 7 quoted lines
> @@ -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?
Show 12 quoted lines
> @@ -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.