Re: [PATCH v2 04/11] svn-fe: add usage and unpositional arguments versions

Dmitry Ivankov wrote:

$ svn-fe --git-svn-id-url=url
does the same thing as
$ svn-fe url
i.e., url is used to generate git-svn-id: lines, if url is set.

Reasonable.  Let's see...

I'd prefer to see the -I flags as part of ALL_CFLAGS, though that's
somewhat orthogonal to this patch.

[...]

[...]

+static struct option svn_fe_options[] = {
+	OPT_STRING(0, "git-svn-id-url", &url, "url",
+		"append git-svn metadata line to commit messages"),

Hmm.  How about this?

	"add git-svn-id line to log messages, imitating git-svn"

[...]

+	argc = parse_options(argc, argv, NULL, svn_fe_options,
+						svn_fe_usage, 0);
+	if (argc == 1) {
+		if (url)
+			usage_msg_opt("git-svn-id-url is set twice: as a "
+					"--parameter and as a [parameter]",
+					svn_fe_usage, svn_fe_options);
+		url = argv[0];
+	} else if (argc)
+		usage_with_options(svn_fe_usage, svn_fe_options);

IMHO would be more readable with the simplest and exceptional case
first:

	if (argc > 1)
		usage_with_options(...);

This way, a person reading can be comforted with the knowledge that
argc <= 1 from then on.

	if (argc == 1) {
		if (url)
			...
	}

To sum up, the patch looks good, and the only improvements I can think
of are tiny nits. :)

With whatever changes mentioned above seem suitable,
Reviewed-by: Jonathan Nieder <redacted>

Thanks.
---
 contrib/svn-fe/Makefile |    4 ++--
 contrib/svn-fe/svn-fe.c |    8 +++++---
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/contrib/svn-fe/Makefile b/contrib/svn-fe/Makefile
index 14b07b5b..62a5c628 100644
--- a/contrib/svn-fe/Makefile
+++ b/contrib/svn-fe/Makefile

@@ -6,7 +6,7 @@ MV = mv
 
 CFLAGS = -g -O2 -Wall
 LDFLAGS =
-ALL_CFLAGS = $(CFLAGS)
+ALL_CFLAGS = -I../.. -I../../vcs-svn $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
 EXTLIBS = -lssl -lcrypto -lpcre -lz -lpthread
 

@@ -39,7 +39,7 @@ svn-fe$X: svn-fe.o $(VCSSVN_LIB) $(GIT_LIB)
 		$(ALL_LDFLAGS) $(LIBS)
 
 svn-fe.o: svn-fe.c ../../vcs-svn/svndump.h
-	$(QUIET_CC)$(CC) -I../../vcs-svn -I../.. -o $*.o -c $(ALL_CFLAGS) $<
+	$(QUIET_CC)$(CC) -o $@ -c $(ALL_CFLAGS) $<
 
 svn-fe.html: svn-fe.txt
 	$(QUIET_SUBDIR0)../../Documentation $(QUIET_SUBDIR1) \

diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index 7e829b91..a95e72f4 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c

@@ -16,7 +16,7 @@ static const char *url;
 
 static struct option svn_fe_options[] = {
 	OPT_STRING(0, "git-svn-id-url", &url, "url",
-		"append git-svn metadata line to commit messages"),
+		"add git-svn-id line to log messages, imitating git-svn"),
 	OPT_END()
 };
 

@@ -24,14 +24,16 @@ int main(int argc, const char **argv)
 {
 	argc = parse_options(argc, argv, NULL, svn_fe_options,
 						svn_fe_usage, 0);
+	if (argc > 1)
+		usage_with_options(svn_fe_usage, svn_fe_options);
+
 	if (argc == 1) {
 		if (url)
 			usage_msg_opt("git-svn-id-url is set twice: as a "
 					"--parameter and as a [parameter]",
 					svn_fe_usage, svn_fe_options);
 		url = argv[0];
-	} else if (argc)
-		usage_with_options(svn_fe_usage, svn_fe_options);
+	}
 	if (svndump_init(NULL))
 		return 1;
 	svndump_read(url);

-- 
1.7.6