[PATCH] gc: use parse_options

[PATCH] gc: use parse_options

[James Bowes]

Signed-off-by: James Bowes <jbowes@dangerouslyinc.com>
---
 builtin-gc.c |   42 ++++++++++++++++++++----------------------
 1 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/builtin-gc.c b/builtin-gc.c
index 3a2ca4f..7bb873c 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c
@@ -12,11 +12,15 @@
 
 #include "builtin.h"
 #include "cache.h"
+#include "parse-options.h"
 #include "run-command.h"
 
 #define FAILED_RUN "failed to run %s"
 
-static const char builtin_gc_usage[] = "git-gc [--prune] [--aggressive]";
+static const char * const builtin_gc_usage[] = {
+	"git-gc [options]",
+	NULL
+};
 
 static int pack_refs = 1;
 static int aggressive_window = -1;
@@ -165,38 +169,32 @@ static int need_to_gc(void)
 
 int cmd_gc(int argc, const char **argv, const char *prefix)
 {
-	int i;
 	int prune = 0;
+	int aggressive = 0;
 	int auto_gc = 0;
 	char buf[80];
 
+	struct option builtin_gc_options[] = {
+		OPT_BOOLEAN(0, "prune", &prune, "prune unused objects"),
+		OPT_BOOLEAN(0, "aggressive", &aggressive, "be more thorough (increased runtime)"),
+		OPT_BOOLEAN(0, "auto", &auto_gc, "enable auto-gc mode"),
+		OPT_END()
+	};
+
 	git_config(gc_config);
 
 	if (pack_refs < 0)
 		pack_refs = !is_bare_repository();
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-		if (!strcmp(arg, "--prune")) {
-			prune = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--aggressive")) {
-			append_option(argv_repack, "-f", MAX_ADD);
-			if (aggressive_window > 0) {
-				sprintf(buf, "--window=%d", aggressive_window);
-				append_option(argv_repack, buf, MAX_ADD);
-			}
-			continue;
-		}
-		if (!strcmp(arg, "--auto")) {
-			auto_gc = 1;
-			continue;
+	parse_options(argc, argv, builtin_gc_options, builtin_gc_usage, 0);
+
+	if (aggressive) {
+		append_option(argv_repack, "-f", MAX_ADD);
+		if (aggressive_window > 0) {
+			sprintf(buf, "--window=%d", aggressive_window);
+			append_option(argv_repack, buf, MAX_ADD);
 		}
-		break;
 	}
-	if (i != argc)
-		usage(builtin_gc_usage);
 
 	if (auto_gc) {
 		/*
-- 
1.5.3.4.1481.g854da

Re: [PATCH] gc: use parse_options

[Junio C Hamano]

James Bowes <jbowes@dangerouslyinc.com> writes:

> +	struct option builtin_gc_options[] = {
> +		OPT_BOOLEAN(0, "prune", &prune, "prune unused objects"),

I would write "unreferenced loose" instead of "unused" here...

> +		OPT_BOOLEAN(0, "aggressive", &aggressive, "be more thorough (increased runtime)"),
> +		OPT_BOOLEAN(0, "auto", &auto_gc, "enable auto-gc mode"),
> +		OPT_END()
> +	};
> +
>  	git_config(gc_config);
>  
>  	if (pack_refs < 0)
>  		pack_refs = !is_bare_repository();
>  
> +	parse_options(argc, argv, builtin_gc_options, builtin_gc_usage, 0);
> +
> +	if (aggressive) {
> +		append_option(argv_repack, "-f", MAX_ADD);
> +		if (aggressive_window > 0) {
> +			sprintf(buf, "--window=%d", aggressive_window);
> +			append_option(argv_repack, buf, MAX_ADD);
>  		}
>  	}
> -	if (i != argc)
> -		usage(builtin_gc_usage);

Now, what makes the command report error when the user says:

	$ git gc unwanted parameter

Other than that, this is a good thing to have, I think.

[PATCH] gc: use parse_options

[James Bowes]

Signed-off-by: James Bowes <jbowes@dangerouslyinc.com>
---

Junio C Hamano <gitster@pobox.com> writes:
> Now, what makes the command report error when the user says:
>
>	$ git gc unwanted parameter

Ah yes. I forgot about that :)

This version of the patch errors out with extra args, and calls them
'unreferenced loose objects' rather than unused.

-James

 builtin-gc.c |   44 ++++++++++++++++++++++----------------------
 1 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/builtin-gc.c b/builtin-gc.c
index 3a2ca4f..c5bce89 100644
--- a/builtin-gc.c
+++ b/builtin-gc.c
@@ -12,11 +12,15 @@
 
 #include "builtin.h"
 #include "cache.h"
+#include "parse-options.h"
 #include "run-command.h"
 
 #define FAILED_RUN "failed to run %s"
 
-static const char builtin_gc_usage[] = "git-gc [--prune] [--aggressive]";
+static const char * const builtin_gc_usage[] = {
+	"git-gc [options]",
+	NULL
+};
 
 static int pack_refs = 1;
 static int aggressive_window = -1;
@@ -165,38 +169,34 @@ static int need_to_gc(void)
 
 int cmd_gc(int argc, const char **argv, const char *prefix)
 {
-	int i;
 	int prune = 0;
+	int aggressive = 0;
 	int auto_gc = 0;
 	char buf[80];
 
+	struct option builtin_gc_options[] = {
+		OPT_BOOLEAN(0, "prune", &prune, "prune unreferenced loose objects"),
+		OPT_BOOLEAN(0, "aggressive", &aggressive, "be more thorough (increased runtime)"),
+		OPT_BOOLEAN(0, "auto", &auto_gc, "enable auto-gc mode"),
+		OPT_END()
+	};
+
 	git_config(gc_config);
 
 	if (pack_refs < 0)
 		pack_refs = !is_bare_repository();
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-		if (!strcmp(arg, "--prune")) {
-			prune = 1;
-			continue;
-		}
-		if (!strcmp(arg, "--aggressive")) {
-			append_option(argv_repack, "-f", MAX_ADD);
-			if (aggressive_window > 0) {
-				sprintf(buf, "--window=%d", aggressive_window);
-				append_option(argv_repack, buf, MAX_ADD);
-			}
-			continue;
-		}
-		if (!strcmp(arg, "--auto")) {
-			auto_gc = 1;
-			continue;
+	argc = parse_options(argc, argv, builtin_gc_options, builtin_gc_usage, 0);
+	if (argc > 0)
+		usage_with_options(builtin_gc_usage, builtin_gc_options);
+
+	if (aggressive) {
+		append_option(argv_repack, "-f", MAX_ADD);
+		if (aggressive_window > 0) {
+			sprintf(buf, "--window=%d", aggressive_window);
+			append_option(argv_repack, buf, MAX_ADD);
 		}
-		break;
 	}
-	if (i != argc)
-		usage(builtin_gc_usage);
 
 	if (auto_gc) {
 		/*
-- 
1.5.3.4.1481.g854da

Re: [PATCH] gc: use parse_options

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1497 bytes --]

On Fri, Nov 02, 2007 at 12:49:25AM +0000, Junio C Hamano wrote:
> James Bowes <jbowes@dangerouslyinc.com> writes:
> 
> > +	struct option builtin_gc_options[] = {
> > +		OPT_BOOLEAN(0, "prune", &prune, "prune unused objects"),
> 
> I would write "unreferenced loose" instead of "unused" here...
> 
> > +		OPT_BOOLEAN(0, "aggressive", &aggressive, "be more thorough (increased runtime)"),
> > +		OPT_BOOLEAN(0, "auto", &auto_gc, "enable auto-gc mode"),
> > +		OPT_END()
> > +	};
> > +
> >  	git_config(gc_config);
> >  
> >  	if (pack_refs < 0)
> >  		pack_refs = !is_bare_repository();
> >  
> > +	parse_options(argc, argv, builtin_gc_options, builtin_gc_usage, 0);
> > +
> > +	if (aggressive) {
> > +		append_option(argv_repack, "-f", MAX_ADD);
> > +		if (aggressive_window > 0) {
> > +			sprintf(buf, "--window=%d", aggressive_window);
> > +			append_option(argv_repack, buf, MAX_ADD);
> >  		}
> >  	}
> > -	if (i != argc)
> > -		usage(builtin_gc_usage);
> 
> Now, what makes the command report error when the user says:
> 
> 	$ git gc unwanted parameter

the commands works fine, because no additionnal checks were made. To
"fix" this, that should be done:

argc = parse_options(argc, argv, builtin_gc_options, builtin_gc_usage, 0);
if (argc)
    usage_with_options(......);

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] gc: use parse_options

[Brandon Casey]

Junio C Hamano wrote:
> James Bowes <jbowes@dangerouslyinc.com> writes:
> 
>> +	struct option builtin_gc_options[] = {
>> +		OPT_BOOLEAN(0, "prune", &prune, "prune unused objects"),
> 
> I would write "unreferenced loose" instead of "unused" here...

It is not just "loose" objects here, but also unreferenced objects in packs,
since the "-a" option to repack is now only used when --prune is specified.
Without --prune, "-A" is supplied to repack instead.

So maybe the message should just be "prune unreferenced objects"

-brandon

Re: [PATCH] gc: use parse_options

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> Junio C Hamano wrote:
>> James Bowes <jbowes@dangerouslyinc.com> writes:
>> 
>>> +	struct option builtin_gc_options[] = {
>>> +		OPT_BOOLEAN(0, "prune", &prune, "prune unused objects"),
>> 
>> I would write "unreferenced loose" instead of "unused" here...
>
> It is not just "loose" objects here, but also unreferenced objects in packs,
> since the "-a" option to repack is now only used when --prune is specified.
> Without --prune, "-A" is supplied to repack instead.
>
> So maybe the message should just be "prune unreferenced objects"

Fair enough, will do.