Re: [PATCH v2 04/11] svn-fe: add usage and unpositional arguments versions
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Jul 24, 2011, 13:25 UTC
- Message-ID
- <20110724132551.GD2934@elie.dc0b.debconf.org>
- In-Reply-To
- <1310559673-5026-5-git-send-email-divanorama@gmail.com>
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...
Show 8 quoted lines
> --- 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"
[...]
Show 10 quoted lines
> + 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@gmail.com>
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