Re: [PATCH v2 05/11] vcs-svn: move url parameter from _read to _init

Dmitry Ivankov wrote:

svndump_read takes a url parameter that is used in git-svn-id: lines

[...]

Move url parameter to svndump_init so that reset_dump_ctx is done
once per dump and in the same place as other resets. Wrap all _init

[...]

I'm getting lost seeing the forest for the trees, so let me try to
summarize.

Before:

	if (svndump_init(url))
		die("svndump_init failed");
	svndump_read(dumpfile);

After:

	struct svndump_args opts;

	memset(&opts, 0, sizeof(opts));
	opts.url = url;
	opts.filename = dumpfile;

	if (svndump_init(&opts))
		die("svndump_init failed");
	svndump_read();

Using an options struct instead of a list of arguments means each
option is optional and has a descriptive name mentioned at the call
site, and means it is easy to add new arguments in the future.

The patch still keeps the init/read distinction even though we don't
need it anywhere (i.e., all call sites look the same) to minimize its
invasiveness.

Do I understand correctly?

If so, it sounds like a good idea, and I have only minor nitpicks:

 - It's tempting to call the struct svndump_options, by analogy
   with struct merge_options from merge-recursive.h.

 - Now that we're making the name of the "url" argument part of the
   public API, maybe we should emphasize that the url is only for
   show and git will never try to contact it.  Maybe something like
   "metadata_url"?  (Sorry, I'm not so great at coming up with names.)

 - Likewise, the "filename" argument could be made more
   self-explanatory.  Maybe "dumpfile"?

 - Now that the filename argument is passed at init time instead of
   read time, there is some uncertainty about when the file is going
   to be opened.  A comment could help, or merging the two functions
   could help. :)

Thanks, and hope that helps.
Jonathan
---
 contrib/svn-fe/svn-fe.c |    4 ++--
 test-svn-fe.c           |    4 ++--
 vcs-svn/svndump.c       |   10 ++++++----
 vcs-svn/svndump.h       |    7 ++++---
 4 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index 22d4cc14..88fca6a3 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c

@@ -9,10 +9,10 @@
 
 int main(int argc, char **argv)
 {
-	struct svndump_args args;
+	struct svndump_options args;
 
 	memset(&args, 0, sizeof(args));
-	args.url = argc > 1 ? argv[1] : NULL;
+	args.metadata_url = argc > 1 ? argv[1] : NULL;
 	if (svndump_init(&args))
 		return 1;
 	svndump_read();

diff --git a/test-svn-fe.c b/test-svn-fe.c
index d80734fd..12a5c3ab 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c

@@ -39,11 +39,11 @@ static int apply_delta(int argc, char *argv[])
 
 int main(int argc, char *argv[])
 {
-	struct svndump_args args;
+	struct svndump_options args;
 
 	memset(&args, 0, sizeof(args));
 	if (argc == 2) {
-		args.filename = argv[1];
+		args.dumpfile = argv[1];
 		if (svndump_init(&args))
 			return 1;
 		svndump_read();

diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 60cccadc..805c94b6 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c

@@ -455,10 +455,12 @@ void svndump_read(void)
 		end_revision();
 }
 
-int svndump_init(const struct svndump_args *args)
+int svndump_init(const struct svndump_options *opts)
 {
-	if (buffer_init(&input, args->filename))
-		return error("cannot open %s: %s", args->filename, strerror(errno));
+	const char *filename = opts->dumpfile;
+
+	if (buffer_init(&input, filename))
+		return error("cannot open %s: %s", filename, strerror(errno));
 	fast_export_init(REPORT_FILENO);
 	strbuf_init(&dump_ctx.uuid, 4096);
 	strbuf_init(&dump_ctx.url, 4096);

@@ -466,7 +468,7 @@ int svndump_init(const struct svndump_args *args)
 	strbuf_init(&rev_ctx.author, 4096);
 	strbuf_init(&node_ctx.src, 4096);
 	strbuf_init(&node_ctx.dst, 4096);
-	reset_dump_ctx(args->url);
+	reset_dump_ctx(opts->metadata_url);
 	reset_rev_ctx(0);
 	reset_node_ctx(NULL);
 	return 0;

diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h
index b3dbf24e..b8f52172 100644
--- a/vcs-svn/svndump.h
+++ b/vcs-svn/svndump.h

@@ -1,11 +1,12 @@
 #ifndef SVNDUMP_H_
 #define SVNDUMP_H_
 
-struct svndump_args {
-	const char *filename, *url;
+struct svndump_options {
+	const char *dumpfile;
+	const char *metadata_url;
 };
 
-int svndump_init(const struct svndump_args *args);
+int svndump_init(const struct svndump_options *o);
 void svndump_read(void);
 void svndump_deinit(void);
 void svndump_reset(void);

-- 
1.7.6