Re: [PATCH v2 02/11] test-svn-fe: use parse-options

Hi,

Sorry for the slow response.

Dmitry Ivankov wrote:

There was custom options parsing. As more options arise it will
be easier to add and document new options with parse-options api.

In particular this gives us a "test-svn-fe -h" command --- sounds
good.  Might make sense to combine this with the patch that
parsoptifies contrib/svn-fe/svn-fe.c.

With this API, we're allowed to print multiple usage strings.  Might as
well take advantage of that for clarity:

 static const char * const test_svnfe_usage[] = {
	"test-svn-fe <dumpfile>",
	"test-svn-fe -d <preimage> <delta> <len>",
	NULL
 };

 
+static int d;
+

The variable name is not so memorable.  Maybe something like
"apply_delta" would do.

-static int apply_delta(int argc, char *argv[])
+static struct option test_svnfe_options[] = {
+	OPT_SET_INT('d', NULL, &d, "test apply_delta", 1),
+	OPT_END()
+};

Might make sense to take the opportunity to add a mnemonic long
option name while at it:

	OPT_SET_INT('d', "apply-delta", ...

[...]

Probably easier to read with the simple and exceptional case first.

	if (apply_delta_instead)
		return apply_delta(argc, argv);
	if (argc != 1)
		usage_with_options(...);

	if (svndump_init(argv[0]))
		return 1;
	...

	

Except for the minor nits noted above (in particular, hopefully this
can be squashed with the corresponding svn-fe patch),

Reviewed-by: Jonathan Nieder <redacted>
---
 test-svn-fe.c |   29 +++++++++++++++--------------
 1 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/test-svn-fe.c b/test-svn-fe.c
index 0aab2450..db56b6ba 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c

@@ -3,21 +3,23 @@
  */
 
 #include "git-compat-util.h"
-#include "parse-options.h"
 #include "vcs-svn/svndump.h"
 #include "vcs-svn/svndiff.h"
 #include "vcs-svn/sliding_window.h"
 #include "vcs-svn/line_buffer.h"
+#include "parse-options.h"
 
 static const char * const test_svnfe_usage[] = {
-	"test-svn-fe (<dumpfile> | -d <preimage> <delta> <len>)",
+	"test-svn-fe <dumpfile>",
+	"test-svn-fe -d <preimage> <delta> <len>",
 	NULL
 };
 
-static int d;
+static int apply_delta_instead;
 
 static struct option test_svnfe_options[] = {
-	OPT_SET_INT('d', NULL, &d, "test apply_delta", 1),
+	OPT_SET_INT('d', "apply-delta",
+		&apply_delta_instead, "apply a subversion-format delta", 1),
 	OPT_END()
 };
 

@@ -52,17 +54,16 @@ int main(int argc, const char *argv[])
 	argc = parse_options(argc, argv, NULL, test_svnfe_options,
 						test_svnfe_usage, 0);
 
-	if (d)
+	if (apply_delta_instead)
 		return apply_delta(argc, argv);
 
-	if (argc == 1) {
-		if (svndump_init(argv[0]))
-			return 1;
-		svndump_read(NULL);
-		svndump_deinit();
-		svndump_reset();
-		return 0;
-	}
+	if (argc != 1)
+		usage_with_options(test_svnfe_usage, test_svnfe_options);
 
-	usage_with_options(test_svnfe_usage, test_svnfe_options);
+	if (svndump_init(argv[0]))
+		return 1;
+	svndump_read(NULL);
+	svndump_deinit();
+	svndump_reset();
+	return 0;
 }

-- 
1.7.6