threads / patch / 15923

patchparse-opt: migrate builtin-checkout-index.

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

## tl;dr

9 messages between Oct 15, 2008 and Oct 19, 2008. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Miklos Vajna· Oct 15, 2008, 22:55 UTC · lore
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(-)
Show changes to builtin-checkout-index.c +82 −69
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
Pierre Habouzit· Oct 16, 2008, 08:23 UTC · re: Miklos Vajna · lore

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

On Wed, Oct 15, 2008 at 10:55:43PM +0000, Miklos Vajna wrote:
Show 8 quoted lines
> 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.

Show 22 quoted lines
> 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
Miklos Vajna· Oct 16, 2008, 13:28 UTC · re: Pierre Habouzit · lore

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

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(-)
Show changes to builtin-checkout-index.c +82 −69
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
Junio C Hamano· Oct 17, 2008, 23:58 UTC · re: Miklos Vajna · lore

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

Miklos Vajna <vmiklos@frugalware.org> writes:
Show 10 quoted lines
> +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 ;-).

Show 10 quoted lines
> +		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?

Miklos Vajna· Oct 18, 2008, 01:17 UTC · re: Junio C Hamano · lore
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:
Show 7 quoted lines
> > +           { 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.

Show 19 quoted lines
> > +           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.

Show changes to builtin-checkout-index.c +83 −69
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
Raphael Zimmerer· Oct 19, 2008, 19:29 UTC · re: Miklos Vajna · lore

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

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
Pierre Habouzit· Oct 19, 2008, 19:34 UTC · re: Raphael Zimmerer · lore

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

On Sun, Oct 19, 2008 at 07:29:08PM +0000, Raphael Zimmerer wrote:
Show 11 quoted lines
> 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
Junio C Hamano· Oct 19, 2008, 21:45 UTC · re: Miklos Vajna · lore

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

Miklos Vajna <vmiklos@frugalware.org> writes:
Show 15 quoted lines
>> > +   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.

Pierre Habouzit· Oct 18, 2008, 15:54 UTC · re: Junio C Hamano · lore

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

On Fri, Oct 17, 2008 at 11:58:23PM +0000, Junio C Hamano wrote:
Show 15 quoted lines
> 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

← back to recent threads