[PATCH] parse-opt: migrate builtin-checkout-index.

[PATCH] parse-opt: migrate builtin-checkout-index.

[Miklos Vajna]

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

NOTE: I introduced the force/quiet/not_new helper variables because they
are originally bitfields, so passing their address is not possible. One
could say that introducing helper functions for those as well would be
nicer, but I think that would just make the code even longer with no
good reason.

 builtin-checkout-index.c |  151 +++++++++++++++++++++++++---------------------
 1 files changed, 82 insertions(+), 69 deletions(-)

diff --git a/builtin-checkout-index.c b/builtin-checkout-index.c
index 4ba2702..e241cd1 100644
--- a/builtin-checkout-index.c
+++ b/builtin-checkout-index.c
@@ -40,6 +40,7 @@
 #include "cache.h"
 #include "quote.h"
 #include "cache-tree.h"
+#include "parse-options.h"
 
 #define CHECKOUT_ALL 4
 static int line_termination = '\n';
@@ -153,11 +154,55 @@ static void checkout_all(const char *prefix, int prefix_length)
 		exit(128);
 }
 
-static const char checkout_cache_usage[] =
-"git checkout-index [-u] [-q] [-a] [-f] [-n] [--stage=[123]|all] [--prefix=<string>] [--temp] [--] <file>...";
+static const char * const builtin_checkout_index_usage[] = {
+	"git checkout-index [-u] [-q] [-a] [-f] [-n] [--stage=[123]|all] [--prefix=<string>] [--temp] [--] <file>...",
+	NULL
+};
 
 static struct lock_file lock_file;
 
+static int option_parse_u(const struct option *opt,
+			      const char *arg, int unset)
+{
+	int *newfd = opt->value;
+
+	state.refresh_cache = 1;
+	if (*newfd < 0)
+		*newfd = hold_locked_index(&lock_file, 1);
+	return 0;
+}
+
+static int option_parse_z(const struct option *opt,
+			  const char *arg, int unset)
+{
+	line_termination = unset;
+	return 0;
+}
+
+static int option_parse_prefix(const struct option *opt,
+			       const char *arg, int unset)
+{
+	state.base_dir = arg;
+	state.base_dir_len = strlen(arg);
+	return 0;
+}
+
+static int option_parse_stage(const struct option *opt,
+			      const char *arg, int unset)
+{
+	if (!strcmp(arg, "all")) {
+		to_tempfile = 1;
+		checkout_stage = CHECKOUT_ALL;
+	} else {
+		int ch = arg[0];
+		if ('1' <= ch && ch <= '3')
+			checkout_stage = arg[0] - '0';
+		else
+			die("stage should be between 1 and 3 or all");
+	}
+	return 0;
+}
+
 int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 {
 	int i;
@@ -165,6 +210,33 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	int all = 0;
 	int read_from_stdin = 0;
 	int prefix_length;
+	int force = 0, quiet = 0, not_new = 0;
+	struct option builtin_checkout_index_options[] = {
+		OPT_BOOLEAN('a', "all", &all,
+			"checks out all files in the index"),
+		OPT_BOOLEAN('f', "force", &force,
+			"forces overwrite of existing files"),
+		OPT__QUIET(&quiet),
+		OPT_BOOLEAN('n', "no-create", &not_new,
+			"don't checkout new files"),
+		{ OPTION_CALLBACK, 'u', "index", &newfd, NULL,
+			"update stat information in the index file",
+			PARSE_OPT_NOARG, option_parse_u },
+		{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,
+			"paths are separated with NUL character",
+			PARSE_OPT_NOARG, option_parse_z },
+		OPT_BOOLEAN(0, "stdin", &read_from_stdin,
+			"read list of paths from the standard input"),
+		OPT_BOOLEAN(0, "temp", &to_tempfile,
+			"write the content to temporary files"),
+		OPT_CALLBACK(0, "prefix", NULL, "string",
+			"when creating files, prepend <string>",
+			option_parse_prefix),
+		OPT_CALLBACK(0, "stage", NULL, NULL,
+			"copy out the files from named stage",
+			option_parse_stage),
+		OPT_END()
+	};
 
 	git_config(git_default_config, NULL);
 	state.base_dir = "";
@@ -174,72 +246,13 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		die("invalid cache");
 	}
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-
-		if (!strcmp(arg, "--")) {
-			i++;
-			break;
-		}
-		if (!strcmp(arg, "-a") || !strcmp(arg, "--all")) {
-			all = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-f") || !strcmp(arg, "--force")) {
-			state.force = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-q") || !strcmp(arg, "--quiet")) {
-			state.quiet = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-n") || !strcmp(arg, "--no-create")) {
-			state.not_new = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-u") || !strcmp(arg, "--index")) {
-			state.refresh_cache = 1;
-			if (newfd < 0)
-				newfd = hold_locked_index(&lock_file, 1);
-			continue;
-		}
-		if (!strcmp(arg, "-z")) {
-			line_termination = 0;
-			continue;
-		}
-		if (!strcmp(arg, "--stdin")) {
-			if (i != argc - 1)
-				die("--stdin must be at the end");
-			read_from_stdin = 1;
-			i++; /* do not consider arg as a file name */
-			break;
-		}
-		if (!strcmp(arg, "--temp")) {
-			to_tempfile = 1;
-			continue;
-		}
-		if (!prefixcmp(arg, "--prefix=")) {
-			state.base_dir = arg+9;
-			state.base_dir_len = strlen(state.base_dir);
-			continue;
-		}
-		if (!prefixcmp(arg, "--stage=")) {
-			if (!strcmp(arg + 8, "all")) {
-				to_tempfile = 1;
-				checkout_stage = CHECKOUT_ALL;
-			} else {
-				int ch = arg[8];
-				if ('1' <= ch && ch <= '3')
-					checkout_stage = arg[8] - '0';
-				else
-					die("stage should be between 1 and 3 or all");
-			}
-			continue;
-		}
-		if (arg[0] == '-')
-			usage(checkout_cache_usage);
-		break;
-	}
+	argc = parse_options(argc, argv, builtin_checkout_index_options,
+			builtin_checkout_index_usage, 0);
+	state.force = force;
+	state.quiet = quiet;
+	state.not_new = not_new;
+	if (argc && read_from_stdin)
+		die("--stdin must be at the end");
 
 	if (state.base_dir_len || to_tempfile) {
 		/* when --prefix is specified we do not
@@ -253,7 +266,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	}
 
 	/* Check out named files first */
-	for ( ; i < argc; i++) {
+	for (i = 0; i < argc; i++) {
 		const char *arg = argv[i];
 		const char *p;
 
-- 
1.6.0.2

Re: [PATCH] parse-opt: migrate builtin-checkout-index.

[Pierre Habouzit]

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

On Wed, Oct 15, 2008 at 10:55:43PM +0000, Miklos Vajna wrote:
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> ---
> 
> NOTE: I introduced the force/quiet/not_new helper variables because they
> are originally bitfields, so passing their address is not possible. One
> could say that introducing helper functions for those as well would be
> nicer, but I think that would just make the code even longer with no
> good reason.

Well the alternative is to replace the bitfields with an enum, but it's
not always that nice as a result. C bit-fields sucks when it comes to
address them, it's silly not to be able to have the same as offsetof in
"bits" for a structure, but oh well.

> diff --git a/builtin-checkout-index.c b/builtin-checkout-index.c
> index 4ba2702..e241cd1 100644
> --- a/builtin-checkout-index.c
> +++ b/builtin-checkout-index.c
> @@ -40,6 +40,7 @@
>  #include "cache.h"
>  #include "quote.h"
>  #include "cache-tree.h"
> +#include "parse-options.h"
>  
>  #define CHECKOUT_ALL 4
>  static int line_termination = '\n';
> @@ -153,11 +154,55 @@ static void checkout_all(const char *prefix, int prefix_length)
>  		exit(128);
>  }
>  
> -static const char checkout_cache_usage[] =
> -"git checkout-index [-u] [-q] [-a] [-f] [-n] [--stage=[123]|all] [--prefix=<string>] [--temp] [--] <file>...";
> +static const char * const builtin_checkout_index_usage[] = {
> +	"git checkout-index [-u] [-q] [-a] [-f] [-n] [--stage=[123]|all] [--prefix=<string>] [--temp] [--] <file>...",
> +	NULL
> +};

Since git checkout-index -h will show you all the options, I usually
prefer to use "[options] [--] <file>...", it's 10x as readable, and the
user will have the [options] detail just below.

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

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

[PATCH v2] parse-opt: migrate builtin-checkout-index.

[Miklos Vajna]

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Thu, Oct 16, 2008 at 10:23:40AM +0200, Pierre Habouzit <madcoder@debian.org> wrote:
> Since git checkout-index -h will show you all the options, I usually
> prefer to use "[options] [--] <file>...", it's 10x as readable, and the
> user will have the [options] detail just below.

OK, changed.

 builtin-checkout-index.c |  151 +++++++++++++++++++++++++---------------------
 1 files changed, 82 insertions(+), 69 deletions(-)

diff --git a/builtin-checkout-index.c b/builtin-checkout-index.c
index 4ba2702..bc91f94 100644
--- a/builtin-checkout-index.c
+++ b/builtin-checkout-index.c
@@ -40,6 +40,7 @@
 #include "cache.h"
 #include "quote.h"
 #include "cache-tree.h"
+#include "parse-options.h"
 
 #define CHECKOUT_ALL 4
 static int line_termination = '\n';
@@ -153,11 +154,55 @@ static void checkout_all(const char *prefix, int prefix_length)
 		exit(128);
 }
 
-static const char checkout_cache_usage[] =
-"git checkout-index [-u] [-q] [-a] [-f] [-n] [--stage=[123]|all] [--prefix=<string>] [--temp] [--] <file>...";
+static const char * const builtin_checkout_index_usage[] = {
+	"git checkout-index [options] [--] <file>...",
+	NULL
+};
 
 static struct lock_file lock_file;
 
+static int option_parse_u(const struct option *opt,
+			      const char *arg, int unset)
+{
+	int *newfd = opt->value;
+
+	state.refresh_cache = 1;
+	if (*newfd < 0)
+		*newfd = hold_locked_index(&lock_file, 1);
+	return 0;
+}
+
+static int option_parse_z(const struct option *opt,
+			  const char *arg, int unset)
+{
+	line_termination = unset;
+	return 0;
+}
+
+static int option_parse_prefix(const struct option *opt,
+			       const char *arg, int unset)
+{
+	state.base_dir = arg;
+	state.base_dir_len = strlen(arg);
+	return 0;
+}
+
+static int option_parse_stage(const struct option *opt,
+			      const char *arg, int unset)
+{
+	if (!strcmp(arg, "all")) {
+		to_tempfile = 1;
+		checkout_stage = CHECKOUT_ALL;
+	} else {
+		int ch = arg[0];
+		if ('1' <= ch && ch <= '3')
+			checkout_stage = arg[0] - '0';
+		else
+			die("stage should be between 1 and 3 or all");
+	}
+	return 0;
+}
+
 int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 {
 	int i;
@@ -165,6 +210,33 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	int all = 0;
 	int read_from_stdin = 0;
 	int prefix_length;
+	int force = 0, quiet = 0, not_new = 0;
+	struct option builtin_checkout_index_options[] = {
+		OPT_BOOLEAN('a', "all", &all,
+			"checks out all files in the index"),
+		OPT_BOOLEAN('f', "force", &force,
+			"forces overwrite of existing files"),
+		OPT__QUIET(&quiet),
+		OPT_BOOLEAN('n', "no-create", &not_new,
+			"don't checkout new files"),
+		{ OPTION_CALLBACK, 'u', "index", &newfd, NULL,
+			"update stat information in the index file",
+			PARSE_OPT_NOARG, option_parse_u },
+		{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,
+			"paths are separated with NUL character",
+			PARSE_OPT_NOARG, option_parse_z },
+		OPT_BOOLEAN(0, "stdin", &read_from_stdin,
+			"read list of paths from the standard input"),
+		OPT_BOOLEAN(0, "temp", &to_tempfile,
+			"write the content to temporary files"),
+		OPT_CALLBACK(0, "prefix", NULL, "string",
+			"when creating files, prepend <string>",
+			option_parse_prefix),
+		OPT_CALLBACK(0, "stage", NULL, NULL,
+			"copy out the files from named stage",
+			option_parse_stage),
+		OPT_END()
+	};
 
 	git_config(git_default_config, NULL);
 	state.base_dir = "";
@@ -174,72 +246,13 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		die("invalid cache");
 	}
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-
-		if (!strcmp(arg, "--")) {
-			i++;
-			break;
-		}
-		if (!strcmp(arg, "-a") || !strcmp(arg, "--all")) {
-			all = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-f") || !strcmp(arg, "--force")) {
-			state.force = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-q") || !strcmp(arg, "--quiet")) {
-			state.quiet = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-n") || !strcmp(arg, "--no-create")) {
-			state.not_new = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-u") || !strcmp(arg, "--index")) {
-			state.refresh_cache = 1;
-			if (newfd < 0)
-				newfd = hold_locked_index(&lock_file, 1);
-			continue;
-		}
-		if (!strcmp(arg, "-z")) {
-			line_termination = 0;
-			continue;
-		}
-		if (!strcmp(arg, "--stdin")) {
-			if (i != argc - 1)
-				die("--stdin must be at the end");
-			read_from_stdin = 1;
-			i++; /* do not consider arg as a file name */
-			break;
-		}
-		if (!strcmp(arg, "--temp")) {
-			to_tempfile = 1;
-			continue;
-		}
-		if (!prefixcmp(arg, "--prefix=")) {
-			state.base_dir = arg+9;
-			state.base_dir_len = strlen(state.base_dir);
-			continue;
-		}
-		if (!prefixcmp(arg, "--stage=")) {
-			if (!strcmp(arg + 8, "all")) {
-				to_tempfile = 1;
-				checkout_stage = CHECKOUT_ALL;
-			} else {
-				int ch = arg[8];
-				if ('1' <= ch && ch <= '3')
-					checkout_stage = arg[8] - '0';
-				else
-					die("stage should be between 1 and 3 or all");
-			}
-			continue;
-		}
-		if (arg[0] == '-')
-			usage(checkout_cache_usage);
-		break;
-	}
+	argc = parse_options(argc, argv, builtin_checkout_index_options,
+			builtin_checkout_index_usage, 0);
+	state.force = force;
+	state.quiet = quiet;
+	state.not_new = not_new;
+	if (argc && read_from_stdin)
+		die("--stdin must be at the end");
 
 	if (state.base_dir_len || to_tempfile) {
 		/* when --prefix is specified we do not
@@ -253,7 +266,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	}
 
 	/* Check out named files first */
-	for ( ; i < argc; i++) {
+	for (i = 0; i < argc; i++) {
 		const char *arg = argv[i];
 		const char *p;
 
-- 
1.6.0.2

Re: [PATCH v2] parse-opt: migrate builtin-checkout-index.

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> +static int option_parse_z(const struct option *opt,
> +			  const char *arg, int unset)
> +{
> +	line_termination = unset;
> +	return 0;
> +}
> ...
> +		{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,
> +			"paths are separated with NUL character",
> +			PARSE_OPT_NOARG, option_parse_z },

This adds a new feature to say --no-z from the command line, doesn't it?
And I suspect the feature is broken ;-).

> +		OPT_BOOLEAN(0, "stdin", &read_from_stdin,
> +			"read list of paths from the standard input"),
> ...
> +	argc = parse_options(argc, argv, builtin_checkout_index_options,
> +			builtin_checkout_index_usage, 0);
> +	state.force = force;
> +	state.quiet = quiet;
> +	state.not_new = not_new;
> +	if (argc && read_from_stdin)
> +		die("--stdin must be at the end");

Is this comment still correct?  Do the original and your version act the
same way when the user says "checkout --stdin -f", for example?  I suspect
the original refused it and yours take it (and do much more sensible
thing), which would be an improvement, but then the error message should
be reworded perhaps?

[PATCH] parse-opt: migrate builtin-checkout-index.

[Miklos Vajna]

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---
 builtin-checkout-index.c |  152 +++++++++++++++++++++++++---------------------
 1 files changed, 83 insertions(+), 69 deletions(-)

On Fri, Oct 17, 2008 at 04:58:23PM -0700, Junio C Hamano <gitster@pobox.com> wrote:
> > +           { OPTION_CALLBACK, 'z', NULL, NULL, NULL,
> > +                   "paths are separated with NUL character",
> > +                   PARSE_OPT_NOARG, option_parse_z },
>
> This adds a new feature to say --no-z from the command line, doesn't
> it?
> And I suspect the feature is broken ;-).

Right, I fixed this in option_parse_z(). --no-z should set
line_termination to \n instead of 1.

> > +           OPT_BOOLEAN(0, "stdin", &read_from_stdin,
> > +                   "read list of paths from the standard input"),
> > ...
> > +   argc = parse_options(argc, argv, builtin_checkout_index_options,
> > +                   builtin_checkout_index_usage, 0);
> > +   state.force = force;
> > +   state.quiet = quiet;
> > +   state.not_new = not_new;
> > +   if (argc && read_from_stdin)
> > +           die("--stdin must be at the end");
>
> Is this comment still correct?  Do the original and your version act
> the
> same way when the user says "checkout --stdin -f", for example?  I
> suspect
> the original refused it and yours take it (and do much more sensible
> thing), which would be an improvement, but then the error message
> should
> be reworded perhaps?

Unless I missed something, that was a limitation of the option parser.
checkout-index --stdin -f works fine for me after removing those two
lines, so I left them out from the updated patch.

diff --git a/builtin-checkout-index.c b/builtin-checkout-index.c
index 4ba2702..0d534bc 100644
--- a/builtin-checkout-index.c
+++ b/builtin-checkout-index.c
@@ -40,6 +40,7 @@
 #include "cache.h"
 #include "quote.h"
 #include "cache-tree.h"
+#include "parse-options.h"
 
 #define CHECKOUT_ALL 4
 static int line_termination = '\n';
@@ -153,11 +154,58 @@ static void checkout_all(const char *prefix, int prefix_length)
 		exit(128);
 }
 
-static const char checkout_cache_usage[] =
-"git checkout-index [-u] [-q] [-a] [-f] [-n] [--stage=[123]|all] [--prefix=<string>] [--temp] [--] <file>...";
+static const char * const builtin_checkout_index_usage[] = {
+	"git checkout-index [options] [--] <file>...",
+	NULL
+};
 
 static struct lock_file lock_file;
 
+static int option_parse_u(const struct option *opt,
+			      const char *arg, int unset)
+{
+	int *newfd = opt->value;
+
+	state.refresh_cache = 1;
+	if (*newfd < 0)
+		*newfd = hold_locked_index(&lock_file, 1);
+	return 0;
+}
+
+static int option_parse_z(const struct option *opt,
+			  const char *arg, int unset)
+{
+	if (unset)
+		line_termination = '\n';
+	else
+		line_termination = 0;
+	return 0;
+}
+
+static int option_parse_prefix(const struct option *opt,
+			       const char *arg, int unset)
+{
+	state.base_dir = arg;
+	state.base_dir_len = strlen(arg);
+	return 0;
+}
+
+static int option_parse_stage(const struct option *opt,
+			      const char *arg, int unset)
+{
+	if (!strcmp(arg, "all")) {
+		to_tempfile = 1;
+		checkout_stage = CHECKOUT_ALL;
+	} else {
+		int ch = arg[0];
+		if ('1' <= ch && ch <= '3')
+			checkout_stage = arg[0] - '0';
+		else
+			die("stage should be between 1 and 3 or all");
+	}
+	return 0;
+}
+
 int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 {
 	int i;
@@ -165,6 +213,33 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	int all = 0;
 	int read_from_stdin = 0;
 	int prefix_length;
+	int force = 0, quiet = 0, not_new = 0;
+	struct option builtin_checkout_index_options[] = {
+		OPT_BOOLEAN('a', "all", &all,
+			"checks out all files in the index"),
+		OPT_BOOLEAN('f', "force", &force,
+			"forces overwrite of existing files"),
+		OPT__QUIET(&quiet),
+		OPT_BOOLEAN('n', "no-create", &not_new,
+			"don't checkout new files"),
+		{ OPTION_CALLBACK, 'u', "index", &newfd, NULL,
+			"update stat information in the index file",
+			PARSE_OPT_NOARG, option_parse_u },
+		{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,
+			"paths are separated with NUL character",
+			PARSE_OPT_NOARG, option_parse_z },
+		OPT_BOOLEAN(0, "stdin", &read_from_stdin,
+			"read list of paths from the standard input"),
+		OPT_BOOLEAN(0, "temp", &to_tempfile,
+			"write the content to temporary files"),
+		OPT_CALLBACK(0, "prefix", NULL, "string",
+			"when creating files, prepend <string>",
+			option_parse_prefix),
+		OPT_CALLBACK(0, "stage", NULL, NULL,
+			"copy out the files from named stage",
+			option_parse_stage),
+		OPT_END()
+	};
 
 	git_config(git_default_config, NULL);
 	state.base_dir = "";
@@ -174,72 +249,11 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		die("invalid cache");
 	}
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-
-		if (!strcmp(arg, "--")) {
-			i++;
-			break;
-		}
-		if (!strcmp(arg, "-a") || !strcmp(arg, "--all")) {
-			all = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-f") || !strcmp(arg, "--force")) {
-			state.force = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-q") || !strcmp(arg, "--quiet")) {
-			state.quiet = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-n") || !strcmp(arg, "--no-create")) {
-			state.not_new = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-u") || !strcmp(arg, "--index")) {
-			state.refresh_cache = 1;
-			if (newfd < 0)
-				newfd = hold_locked_index(&lock_file, 1);
-			continue;
-		}
-		if (!strcmp(arg, "-z")) {
-			line_termination = 0;
-			continue;
-		}
-		if (!strcmp(arg, "--stdin")) {
-			if (i != argc - 1)
-				die("--stdin must be at the end");
-			read_from_stdin = 1;
-			i++; /* do not consider arg as a file name */
-			break;
-		}
-		if (!strcmp(arg, "--temp")) {
-			to_tempfile = 1;
-			continue;
-		}
-		if (!prefixcmp(arg, "--prefix=")) {
-			state.base_dir = arg+9;
-			state.base_dir_len = strlen(state.base_dir);
-			continue;
-		}
-		if (!prefixcmp(arg, "--stage=")) {
-			if (!strcmp(arg + 8, "all")) {
-				to_tempfile = 1;
-				checkout_stage = CHECKOUT_ALL;
-			} else {
-				int ch = arg[8];
-				if ('1' <= ch && ch <= '3')
-					checkout_stage = arg[8] - '0';
-				else
-					die("stage should be between 1 and 3 or all");
-			}
-			continue;
-		}
-		if (arg[0] == '-')
-			usage(checkout_cache_usage);
-		break;
-	}
+	argc = parse_options(argc, argv, builtin_checkout_index_options,
+			builtin_checkout_index_usage, 0);
+	state.force = force;
+	state.quiet = quiet;
+	state.not_new = not_new;
 
 	if (state.base_dir_len || to_tempfile) {
 		/* when --prefix is specified we do not
@@ -253,7 +267,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	}
 
 	/* Check out named files first */
-	for ( ; i < argc; i++) {
+	for (i = 0; i < argc; i++) {
 		const char *arg = argv[i];
 		const char *p;
 
-- 
1.6.0.2

[PATCH] parse-opt: migrate builtin-checkout-index.

[Miklos Vajna]

Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
---

On Sat, Oct 18, 2008 at 03:17:23AM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:
> > This adds a new feature to say --no-z from the command line, doesn't
> > it?
> > And I suspect the feature is broken ;-).
>
> Right, I fixed this in option_parse_z(). --no-z should set
> line_termination to \n instead of 1.

Originally in option_parse_z() I had

        line_termination = unset;

which is in fact right, because (as Pierre pointed out) unset for short
options are always false, but I changed it to

        line_termination = 0;

to make it more readable.

> > Is this comment still correct?  Do the original and your version act
> > the
> > same way when the user says "checkout --stdin -f", for example?  I
> > suspect
> > the original refused it and yours take it (and do much more sensible
> > thing), which would be an improvement, but then the error message
> > should
> > be reworded perhaps?
>
> Unless I missed something, that was a limitation of the option parser.
> checkout-index --stdin -f works fine for me after removing those two
> lines, so I left them out from the updated patch.

I still think that check is necessary, but the reasoning was false: what
we need to check is the usage of --stdin and filenames togother. But a
bit later there is a

        die("git checkout-index: don't mix '--stdin' and explicit filenames");

so the check I removed was unnecessary.

Here is a patch with the cleaned up option_parse_z().

 builtin-checkout-index.c |  149 +++++++++++++++++++++++++---------------------
 1 files changed, 80 insertions(+), 69 deletions(-)

diff --git a/builtin-checkout-index.c b/builtin-checkout-index.c
index 4ba2702..2c558dd 100644
--- a/builtin-checkout-index.c
+++ b/builtin-checkout-index.c
@@ -40,6 +40,7 @@
 #include "cache.h"
 #include "quote.h"
 #include "cache-tree.h"
+#include "parse-options.h"
 
 #define CHECKOUT_ALL 4
 static int line_termination = '\n';
@@ -153,11 +154,55 @@ static void checkout_all(const char *prefix, int prefix_length)
 		exit(128);
 }
 
-static const char checkout_cache_usage[] =
-"git checkout-index [-u] [-q] [-a] [-f] [-n] [--stage=[123]|all] [--prefix=<string>] [--temp] [--] <file>...";
+static const char * const builtin_checkout_index_usage[] = {
+	"git checkout-index [options] [--] <file>...",
+	NULL
+};
 
 static struct lock_file lock_file;
 
+static int option_parse_u(const struct option *opt,
+			      const char *arg, int unset)
+{
+	int *newfd = opt->value;
+
+	state.refresh_cache = 1;
+	if (*newfd < 0)
+		*newfd = hold_locked_index(&lock_file, 1);
+	return 0;
+}
+
+static int option_parse_z(const struct option *opt,
+			  const char *arg, int unset)
+{
+	line_termination = 0;
+	return 0;
+}
+
+static int option_parse_prefix(const struct option *opt,
+			       const char *arg, int unset)
+{
+	state.base_dir = arg;
+	state.base_dir_len = strlen(arg);
+	return 0;
+}
+
+static int option_parse_stage(const struct option *opt,
+			      const char *arg, int unset)
+{
+	if (!strcmp(arg, "all")) {
+		to_tempfile = 1;
+		checkout_stage = CHECKOUT_ALL;
+	} else {
+		int ch = arg[0];
+		if ('1' <= ch && ch <= '3')
+			checkout_stage = arg[0] - '0';
+		else
+			die("stage should be between 1 and 3 or all");
+	}
+	return 0;
+}
+
 int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 {
 	int i;
@@ -165,6 +210,33 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	int all = 0;
 	int read_from_stdin = 0;
 	int prefix_length;
+	int force = 0, quiet = 0, not_new = 0;
+	struct option builtin_checkout_index_options[] = {
+		OPT_BOOLEAN('a', "all", &all,
+			"checks out all files in the index"),
+		OPT_BOOLEAN('f', "force", &force,
+			"forces overwrite of existing files"),
+		OPT__QUIET(&quiet),
+		OPT_BOOLEAN('n', "no-create", &not_new,
+			"don't checkout new files"),
+		{ OPTION_CALLBACK, 'u', "index", &newfd, NULL,
+			"update stat information in the index file",
+			PARSE_OPT_NOARG, option_parse_u },
+		{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,
+			"paths are separated with NUL character",
+			PARSE_OPT_NOARG, option_parse_z },
+		OPT_BOOLEAN(0, "stdin", &read_from_stdin,
+			"read list of paths from the standard input"),
+		OPT_BOOLEAN(0, "temp", &to_tempfile,
+			"write the content to temporary files"),
+		OPT_CALLBACK(0, "prefix", NULL, "string",
+			"when creating files, prepend <string>",
+			option_parse_prefix),
+		OPT_CALLBACK(0, "stage", NULL, NULL,
+			"copy out the files from named stage",
+			option_parse_stage),
+		OPT_END()
+	};
 
 	git_config(git_default_config, NULL);
 	state.base_dir = "";
@@ -174,72 +246,11 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		die("invalid cache");
 	}
 
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-
-		if (!strcmp(arg, "--")) {
-			i++;
-			break;
-		}
-		if (!strcmp(arg, "-a") || !strcmp(arg, "--all")) {
-			all = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-f") || !strcmp(arg, "--force")) {
-			state.force = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-q") || !strcmp(arg, "--quiet")) {
-			state.quiet = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-n") || !strcmp(arg, "--no-create")) {
-			state.not_new = 1;
-			continue;
-		}
-		if (!strcmp(arg, "-u") || !strcmp(arg, "--index")) {
-			state.refresh_cache = 1;
-			if (newfd < 0)
-				newfd = hold_locked_index(&lock_file, 1);
-			continue;
-		}
-		if (!strcmp(arg, "-z")) {
-			line_termination = 0;
-			continue;
-		}
-		if (!strcmp(arg, "--stdin")) {
-			if (i != argc - 1)
-				die("--stdin must be at the end");
-			read_from_stdin = 1;
-			i++; /* do not consider arg as a file name */
-			break;
-		}
-		if (!strcmp(arg, "--temp")) {
-			to_tempfile = 1;
-			continue;
-		}
-		if (!prefixcmp(arg, "--prefix=")) {
-			state.base_dir = arg+9;
-			state.base_dir_len = strlen(state.base_dir);
-			continue;
-		}
-		if (!prefixcmp(arg, "--stage=")) {
-			if (!strcmp(arg + 8, "all")) {
-				to_tempfile = 1;
-				checkout_stage = CHECKOUT_ALL;
-			} else {
-				int ch = arg[8];
-				if ('1' <= ch && ch <= '3')
-					checkout_stage = arg[8] - '0';
-				else
-					die("stage should be between 1 and 3 or all");
-			}
-			continue;
-		}
-		if (arg[0] == '-')
-			usage(checkout_cache_usage);
-		break;
-	}
+	argc = parse_options(argc, argv, builtin_checkout_index_options,
+			builtin_checkout_index_usage, 0);
+	state.force = force;
+	state.quiet = quiet;
+	state.not_new = not_new;
 
 	if (state.base_dir_len || to_tempfile) {
 		/* when --prefix is specified we do not
@@ -253,7 +264,7 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 	}
 
 	/* Check out named files first */
-	for ( ; i < argc; i++) {
+	for (i = 0; i < argc; i++) {
 		const char *arg = argv[i];
 		const char *p;
 
-- 
1.6.0.2

Re: [PATCH] parse-opt: migrate builtin-checkout-index.

[Miklos Vajna]

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

On Sun, Oct 19, 2008 at 08:30:40PM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:
> I still think that check is necessary, but the reasoning was false: what

                              ^ unnecessary

Sorry for not reading over my mail before sending it.

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

Re: [PATCH] parse-opt: migrate builtin-checkout-index.

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> ---
>
> On Sat, Oct 18, 2008 at 03:17:23AM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:
>> > This adds a new feature to say --no-z from the command line, doesn't
>> > it?
>> > And I suspect the feature is broken ;-).
>>
>> Right, I fixed this in option_parse_z(). --no-z should set
>> line_termination to \n instead of 1.
>
> Originally in option_parse_z() I had
>
>         line_termination = unset;
>
> which is in fact right, because (as Pierre pointed out) unset for short
> options are always false, but I changed it to
>
>         line_termination = 0;
>
> to make it more readable.

I think Pierre's comment is short-sighted.  Think of what would happen
when somebody adds "--nul" as a longer equivalent to "-z", since it is
extremely easy to do things like that with the use of parse-opt API?

Re: [PATCH] parse-opt: migrate builtin-checkout-index.

[Pierre Habouzit]

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

On Sun, Oct 19, 2008 at 08:11:31PM +0000, Junio C Hamano wrote:
> Miklos Vajna <vmiklos@frugalware.org> writes:
> 
> > Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> > ---
> >
> > On Sat, Oct 18, 2008 at 03:17:23AM +0200, Miklos Vajna <vmiklos@frugalware.org> wrote:
> >> > This adds a new feature to say --no-z from the command line, doesn't
> >> > it?
> >> > And I suspect the feature is broken ;-).
> >>
> >> Right, I fixed this in option_parse_z(). --no-z should set
> >> line_termination to \n instead of 1.
> >
> > Originally in option_parse_z() I had
> >
> >         line_termination = unset;
> >
> > which is in fact right, because (as Pierre pointed out) unset for short
> > options are always false, but I changed it to
> >
> >         line_termination = 0;
> >
> > to make it more readable.
> 
> I think Pierre's comment is short-sighted.  Think of what would happen
> when somebody adds "--nul" as a longer equivalent to "-z", since it is
> extremely easy to do things like that with the use of parse-opt API?

Err I was only pointing out that --no-z would no nothing, I actually
didn't really read the argument :)  I didn't say having --null was a bad
idea, and I think we have -z/--null at a couple of places already, and
it's probably a good thing to actually _add_ --null.

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

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

Re: [PATCH] parse-opt: migrate builtin-checkout-index.

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

> On Sun, Oct 19, 2008 at 08:11:31PM +0000, Junio C Hamano wrote:
>> Miklos Vajna <vmiklos@frugalware.org> writes:
>
>> >> Right, I fixed this in option_parse_z(). --no-z should set
>> >> line_termination to \n instead of 1.
>> >
>> > Originally in option_parse_z() I had
>> >
>> >         line_termination = unset;
>> >
>> > which is in fact right, because (as Pierre pointed out) unset for short
>> > options are always false, but I changed it to
>> >
>> >         line_termination = 0;
>> >
>> > to make it more readable.
>> 
>> I think Pierre's comment is short-sighted.  Think of what would happen
>> when somebody adds "--nul" as a longer equivalent to "-z", since it is
>> extremely easy to do things like that with the use of parse-opt API?
>
> Err I was only pointing out that --no-z would no nothing, I actually
> didn't really read the argument :)  I didn't say having --null was a bad
> idea,...

It is a good practice to anticipate potential future breakages, assess the
cost to avoid them, avoid the ones that can be avoided with minimum cost
(and document the others you know will be broken).  That's the key to
produce maintainable piece of code.  The change necessary to Miklos's
original code to do this is quite simple (i.e. flip between '\0' and
'\n'), and I didn't see any room for argument like "short option won't let
you say --no-z".  That's where my comment about "short-sighted"-ness comes
from.

Re: [PATCH] parse-opt: migrate builtin-checkout-index.

[Raphael Zimmerer]

On Sat, Oct 18, 2008 at 03:17:23AM +0200, Miklos Vajna wrote:
> Right, I fixed this in option_parse_z(). --no-z should set
> line_termination to \n instead of 1.

How about "--no-null"?

The long option name for "-z" is "--null", as used in git-config and
git-grep. So I suggest to use that as the long option name for "-z",
as the enhanced option parser automatically will recongnize
"--no-null", when used. That helps avoid further confusion with git
option names.

 - Raphael

Re: [PATCH] parse-opt: migrate builtin-checkout-index.

[Pierre Habouzit]

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

On Sun, Oct 19, 2008 at 07:29:08PM +0000, Raphael Zimmerer wrote:
> On Sat, Oct 18, 2008 at 03:17:23AM +0200, Miklos Vajna wrote:
> > Right, I fixed this in option_parse_z(). --no-z should set
> > line_termination to \n instead of 1.
> 
> How about "--no-null"?
> 
> The long option name for "-z" is "--null", as used in git-config and
> git-grep. So I suggest to use that as the long option name for "-z",
> as the enhanced option parser automatically will recongnize
> "--no-null", when used. That helps avoid further confusion with git
> option names.

git checkout-index has no --null option. If you make --null be the long
form for -z then --no-null will be "autogenerated".
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

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

Re: [PATCH] parse-opt: migrate builtin-checkout-index.

[Junio C Hamano]

Miklos Vajna <vmiklos@frugalware.org> writes:

>> > +   if (argc && read_from_stdin)
>> > +           die("--stdin must be at the end");
>>
>> Is this comment still correct?  Do the original and your version act
>> the
>> same way when the user says "checkout --stdin -f", for example?  I
>> suspect
>> the original refused it and yours take it (and do much more sensible
>> thing), which would be an improvement, but then the error message
>> should
>> be reworded perhaps?
>
> Unless I missed something, that was a limitation of the option parser.
> checkout-index --stdin -f works fine for me after removing those two
> lines, so I left them out from the updated patch.

Thanks.  I think you got what I meant and dropping the part is right.

"--stdin -f" was rejected by the original code, and you improved to take
it with the new parser.  In fact, the above quoted if() statement should
not trigger when "--stdin -f" is given, due to the way the new option
parser is structured.  The original had an explicit "break" in the loop
when it saw "--stdin".  The above would still trigger if "--stdin foo" is
given, but there is a code to catch that already, so it is not necessary.

Re: [PATCH v2] parse-opt: migrate builtin-checkout-index.

[Pierre Habouzit]

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

On Fri, Oct 17, 2008 at 11:58:23PM +0000, Junio C Hamano wrote:
> Miklos Vajna <vmiklos@frugalware.org> writes:
> 
> > +static int option_parse_z(const struct option *opt,
> > +			  const char *arg, int unset)
> > +{
> > +	line_termination = unset;
> > +	return 0;
> > +}
> > ...
> > +		{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,
> > +			"paths are separated with NUL character",
> > +			PARSE_OPT_NOARG, option_parse_z },
> 
> This adds a new feature to say --no-z from the command line, doesn't it?
> And I suspect the feature is broken ;-).

No it doesn't, --no-foo is only active for long options.

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

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