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...
> --- a/contrib/svn-fe/Makefile
> +++ b/contrib/svn-fe/Makefile
> @@ -41,7 +41,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 -o $*.o -c $(ALL_CFLAGS) $<
> + $(QUIET_CC)$(CC) -I../../vcs-svn -I../.. -o $*.o -c $(ALL_CFLAGS) $<
I'd prefer to see the -I flags as part of ALL_CFLAGS, though that's
somewhat orthogonal to this patch.
[...]
> --- a/contrib/svn-fe/svn-fe.c
> +++ b/contrib/svn-fe/svn-fe.c
> @@ -3,14 +3,38 @@
[...]
> +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 <jrnieder@xxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html