Re: [PATCH v2 06/11] vcs-svn: move commit parameters logic to svndump.c
From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:51:38
Subsystem:
the rest · Maintainer:
Linus Torvalds
Dmitry Ivankov wrote:
fast_export.c had logic to set up commit ref, author name, email, parent commit, import mark and git-svn-id: line based on both it's own state (current import batch history) and the arguments passed. Lift the decision on these parameters to the caller.
Again I find myself getting lost. I think this is another internal API change, with the intent being to make the fast_export lib more intuitive by making it focus on communicating with fast-import and the delta applier instead of taking care of so much svn-fe-specific logic. In other words, the idea would be to avoid a few layering violations. Is that right? If so:
quoted hunk ↗ jump to hunk
--- a/vcs-svn/fast_export.c +++ b/vcs-svn/fast_export.c@@ -13,9 +13,6 @@
[...]
-static uint32_t first_commit_done;
This is a good change. [...]
quoted hunk ↗ jump to hunk
@@ -73,42 +69,30 @@ void fast_export_modify(const char *path, uint32_t mode, const char *dataref) putchar('\n'); } -static char gitsvnline[MAX_GITSVN_LINE_LEN]; -void fast_export_begin_commit(uint32_t revision, const char *author, - const struct strbuf *log, - const char *uuid, const char *url, - unsigned long timestamp) +void fast_export_begin_commit(uint32_t set_mark, const char *committer_name, + const char *committer_login, const char *committer_domain, + const struct strbuf *log, const char *gitsvnline, + unsigned long timestamp, uint32_t from_mark, + const char *dst_ref)
The argument list is getting scary. [...]
void fast_export_end_commit(uint32_t revision)
{
- printf("progress Imported commit %"PRIu32".\n\n", revision);
}This change leaves fast_export_end_commit empty. Why not remove it? (Later patches that want to insert code there could reintroduce the function.) [...]
quoted hunk ↗ jump to hunk
--- a/vcs-svn/svndump.c +++ b/vcs-svn/svndump.c@@ -37,6 +37,8 @@ #define LENGTH_UNKNOWN (~0) #define DATE_RFC2822_LEN 31 +#define MAX_GITSVN_LINE_LEN 4096 + static struct line_buffer input = LINE_BUFFER_INIT; static struct {@@ -54,6 +56,7 @@ static struct { static struct { uint32_t version; struct strbuf uuid, url; + int first_commit_done;
Sneaky: changing the type from uint32_t to int. Good. :) [...]
quoted hunk ↗ jump to hunk
@@ -299,19 +303,37 @@ static void handle_node(void)
[...]
+ fast_export_begin_commit(rev_ctx.revision, author, author, domain, + &rev_ctx.log, gitsvnline, rev_ctx.timestamp, + from_mark);
This doesn't compile for me (missing "ref" argument).
}
static void end_revision(void)
{
- if (rev_ctx.revision)
+ if (rev_ctx.revision) {
fast_export_end_commit(rev_ctx.revision);
+ printf("progress Imported commit %"PRIu32".\n\n", rev_ctx.revision);Until now, svndump.c did not have to know about the fast-import format (e.g., the existence of a "progress" command). Is that worth changing? Quick sketch with suggestions. What do you think? -- >8 -- Subject: squash! vcs-svn: move commit parameters logic to svndump.c The previous commit doesn't build because we forgot to pass the new ref name argument to the fast_export API. While fixing that, simplify fast_export_begin_commit to be more intuitive by using a set of parameters closer to what gets written to fast-import. The actual impact of this patch would be to run a little slower, since we needlessly copy the author name into a temporary buffer for an email address. That is a small per-commit rather than per-path cost so the loss in speed might be worth the gain in readability. Signed-off-by: Jonathan Nieder <redacted> --- vcs-svn/fast_export.c | 35 ++++++++++++++++------------------- vcs-svn/fast_export.h | 10 ++++------ vcs-svn/svndump.c | 41 ++++++++++++++++++++++++----------------- 3 files changed, 44 insertions(+), 42 deletions(-)
diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 04001b83..f61113b4 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c@@ -69,30 +69,27 @@ void fast_export_modify(const char *path, uint32_t mode, const char *dataref) putchar('\n'); } -void fast_export_begin_commit(uint32_t set_mark, const char *committer_name, - const char *committer_login, const char *committer_domain, - const struct strbuf *log, const char *gitsvnline, - unsigned long timestamp, uint32_t from_mark, - const char *dst_ref) +void fast_export_begin_commit(const char *ref, uint32_t mark, uint32_t prev_mark, + const char *author_name, const char *author_email, + const struct strbuf *log, unsigned long timestamp) { - if (!gitsvnline) - gitsvnline = ""; - printf("commit %s\n", dst_ref); - if (set_mark) - printf("mark :%"PRIu32"\n", set_mark); - printf("committer %s <%s@%s> %ld +0000\n", - committer_name, committer_login, committer_domain, - timestamp); - printf("data %"PRIuMAX"\n", - (uintmax_t) (log->len + strlen(gitsvnline))); + if (!ref) + ref = "refs/heads/master"; + printf("commit %s\n", ref); + if (mark) + printf("mark :%"PRIu32"\n", mark); + printf("committer %s <%s> %ld +0000\n", + author_name, author_email, timestamp); + printf("data %"PRIuMAX"\n", (uintmax_t) log->len); fwrite(log->buf, log->len, 1, stdout); - printf("%s\n", gitsvnline); - if (from_mark) - printf("from :%"PRIu32"\n", from_mark); + putchar('\n'); + if (prev_mark) + printf("from :%"PRIu32"\n", prev_mark); } -void fast_export_end_commit(uint32_t revision) +void fast_export_progress(uint32_t revision) { + printf("progress Imported commit %"PRIu32".\n\n", revision); } static void ls_from_rev(uint32_t rev, const char *path)
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index 6c1c2be9..f2baf99d 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h@@ -10,12 +10,10 @@ void fast_export_reset(void); void fast_export_delete(const char *path); void fast_export_modify(const char *path, uint32_t mode, const char *dataref); -void fast_export_begin_commit(uint32_t set_mark, const char *committer_name, - const char *committer_login, const char *committer_domain, - const struct strbuf *log, const char *gitsvnline, - unsigned long timestamp, uint32_t from_mark, - const char *dst_ref); -void fast_export_end_commit(uint32_t revision); +void fast_export_begin_commit(const char *ref, uint32_t mark, uint32_t prev_mark, + const char *author_name, const char *author_email, + const struct strbuf *log, unsigned long timestamp); +void fast_export_progress(uint32_t revision); void fast_export_data(uint32_t mode, uint32_t len, struct line_buffer *input); void fast_export_blob_delta(uint32_t mode, uint32_t old_mode, const char *old_data,
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 97d5fdb7..c562cdaa 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c@@ -302,35 +302,42 @@ static void handle_node(void) node_ctx.textLength, &input); } -static char gitsvnline[MAX_GITSVN_LINE_LEN]; +static void add_metadata_trailer(struct strbuf *buf) +{ + if (*dump_ctx.uuid.buf && *dump_ctx.url.buf) + strbuf_addf(buf, "\n\ngit-svn-id: %s@%"PRIu32" %s\n", + dump_ctx.url.buf, rev_ctx.revision, dump_ctx.uuid.buf); +} + static void begin_revision(void) { - int from_mark; + static struct strbuf email; const char *author; - const char *domain; + uint32_t prev; + if (!rev_ctx.revision) /* revision 0 gets no git commit. */ return; - if (*dump_ctx.uuid.buf && *dump_ctx.url.buf) { - snprintf(gitsvnline, MAX_GITSVN_LINE_LEN, - "\n\ngit-svn-id: %s@%"PRIu32" %s\n", - dump_ctx.url.buf, rev_ctx.revision, dump_ctx.uuid.buf); - } else { - *gitsvnline = 0; - } - from_mark = dump_ctx.first_commit_done ? rev_ctx.revision - 1 : 0; + prev = dump_ctx.first_commit_done ? rev_ctx.revision - 1 : 0; author = *rev_ctx.author.buf ? rev_ctx.author.buf : "nobody"; - domain = *dump_ctx.uuid.buf ? dump_ctx.uuid.buf : "local"; - fast_export_begin_commit(rev_ctx.revision, author, author, domain, - &rev_ctx.log, gitsvnline, rev_ctx.timestamp, - from_mark); + strbuf_reset(&email); + strbuf_addstr(&email, author); + strbuf_addch(&email, '@'); + if (*dump_ctx.uuid.buf) + strbuf_addstr(&email, dump_ctx.uuid.buf); + else + strbuf_addstr(&email, "local"); + + add_metadata_trailer(&rev_ctx.log); + + fast_export_begin_commit(NULL, rev_ctx.revision, prev, + author, email.buf, &rev_ctx.log, rev_ctx.timestamp); } static void end_revision(void) { if (rev_ctx.revision) { - fast_export_end_commit(rev_ctx.revision); - printf("progress Imported commit %"PRIu32".\n\n", rev_ctx.revision); + fast_export_progress(rev_ctx.revision); dump_ctx.first_commit_done = 1; } }
--
1.7.6