git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 06/11] vcs-svn: move commit parameters logic to svndump.c

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Jul 24, 2011, 13:58 UTC
Message-ID
<20110724135811.GG2934@elie.dc0b.debconf.org>
In-Reply-To
<1310559673-5026-7-git-send-email-divanorama@gmail.com>
Dmitry Ivankov wrote:
Show 5 quoted lines
> 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:
> --- 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.
[...]
Show 14 quoted lines
> @@ -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.)

[...]
Show 16 quoted lines
> --- 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. :)
[...]
> @@ -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).
Show 8 quoted lines
>  }
>  
>  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 <jrnieder@gmail.com>
---
 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
Previous: Dmitry IvankovNext: Dmitry Ivankov
Message 13 of 24 in “vcs-svn,svn-fe add a couple of options”
  1. 00/11 vcs-svn,svn-fe add a couple of optionsDmitry Ivankov, Jul 13, 2011
  2. 01/11 svn-fe: add man target to MakefileDmitry Ivankov, Jul 13, 2011
  3. Jonathan NiederJul 24, 2011
  4. 02/11 test-svn-fe: use parse-optionsDmitry Ivankov, Jul 13, 2011
  5. Jonathan NiederJul 24, 2011
  6. 03/11 svn-fe: add EXTLIBS needed for parse-optionsDmitry Ivankov, Jul 13, 2011
  7. Jonathan NiederJul 24, 2011
  8. 04/11 svn-fe: add usage and unpositional arguments versionsDmitry Ivankov, Jul 13, 2011
  9. Jonathan NiederJul 24, 2011
  10. 05/11 vcs-svn: move url parameter from _read to _initDmitry Ivankov, Jul 13, 2011
  11. Jonathan NiederJul 24, 2011
  12. 06/11 vcs-svn: move commit parameters logic to svndump.cDmitry Ivankov, Jul 13, 2011
  13. Jonathan NiederJul 24, 2011
  14. 07/11 vcs-svn,svn-fe: allow to specify dump destination refDmitry Ivankov, Jul 13, 2011
  15. Jonathan NiederJul 25, 2011
  16. 08/11 vcs-svn,svn-fe: convert REPORT_FILENO to an optionDmitry Ivankov, Jul 13, 2011
  17. Jonathan NiederJul 25, 2011
  18. 09/11 vcs-svn,svn-fe: allow to disable 'progress' linesDmitry Ivankov, Jul 13, 2011
  19. 10/11 vcs-svn,svn-fe: add --incremental optionDmitry Ivankov, Jul 13, 2011
  20. Jonathan NiederJul 25, 2011
  21. 11/11 vcs-svn,svn-fe: add an option to write svnrev notesDmitry Ivankov, Jul 13, 2011
  22. Jonathan NiederJul 25, 2011
  23. Dmitry IvankovJul 28, 2011
  24. Jonathan NiederJul 28, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.