Re: [PATCH 4/4] update-index: migrate to parse-options API

On 10/24/10 01:18, Jonathan Nieder wrote:

---
 builtin/update-index.c |  389 +++++++++++++++++++++++++++++------------------
 1 files changed, 240 insertions(+), 149 deletions(-)

I would suspect that the code would get smaller. Too many callbacks?

 
-static const char update_index_usage[] =
-"git update-index [-q] [--add] [--replace] [--remove] [--unmerged] [--refresh] [--really-refresh] [--cacheinfo] [--chmod=(+|-)x] [--assume-unchanged] [--skip-worktree|--no-skip-worktree] [--info-only] [--force-remove] [--stdin] [--index-info] [--unresolve] [--again | -g] [--ignore-missing] [-z] [--verbose] [--] [<file>...]";
+static const char * const update_index_usage[] = {
+"git update-index [-q] [--add] [--replace] [--remove] [--unmerged] [--refresh] [--really-refresh] [--cacheinfo] [--chmod=(+|-)x] [--assume-unchanged] [--skip-worktree|--no-skip-worktree] [--info-only] [--force-remove] [--stdin] [--index-info] [--unresolve] [--again | -g] [--ignore-missing] [-z] [--verbose] [--] [<file>...]",
+	NULL
+};

Please drop all this extraneous option stuff since it's already shown in
the -h output. The usage should list the different command modes. I know
that I've failed to do this in the past (and I should probably fix those).

Would

	git update-index [<options>] -- <file>

be enough?

+static int cacheinfo_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,
+			int flags)
+{
+	unsigned char sha1[20];
+	unsigned int mode;
+	if (flags & OPT_UNSET)
+		return error("--cacheinfo cannot be negated");

This shouldn't be possible right? I thought parse options made sure
NONEG options couldn't be negated... <goes and looks at patch 1>. Oh. It
seems like there will be a lot of duplicated code that way. Maybe we can
fixup patch 1 a bit so this isn't necessary.

+	if (ctx->opt)
+		return error("--cacheinfo does not accept an attached argument");

And this too.

+	if (ctx->argc <= 3)
+		return error("--cacheinfo requires three arguments");
+	if (strtoul_ui(*++ctx->argv, 8, &mode) ||
+	    get_sha1_hex(*++ctx->argv, sha1) ||
+	    add_cacheinfo(mode, sha1, *++ctx->argv, 0))
+		die("git update-index: --cacheinfo cannot add %s", *ctx->argv);

Hmm, it would be neat if die() always prefixed the death message with
the command in which it died.

+	ctx->argc -= 3;
+	return 0;
+}
+
+static int stdin_cacheinfo_cb(struct parse_opt_ctx_t *ctx,
+			      const struct option *opt, int flags)
+{
+	int *line_termination = opt->value;
+	if (ctx->argc != 1 || ctx->opt)
+		return error("--%s must be at the end", opt->long_name);
+	if (flags & OPT_UNSET)
+		return error("--%s cannot be negated", opt->long_name);
+	allow_add = allow_replace = allow_remove = 1;
+	read_index_info(*line_termination);
+	return 0;
+}
+
+static int last_arg_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,
+		       int flags)
+{
+	int *read_from_stdin = opt->value;
+	if (ctx->argc != 1)
+		return error("--%s must be at the end", opt->long_name);

Thinking out loud, this might be better served as an option flag
(PARSE_OPT_LAST_ARG?) to make it a bit more generic. Especially since
you use it twice.

+	if (flags & OPT_UNSET)
+		return error("--%s cannot be negated", opt->long_name);
+	*read_from_stdin = 1;
+	return 0;
+}

And then this callback would go away and you could use a custom
OPTION_SET_PTR (or probably OPTION_SET_INT) right?

+static int reupdate_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,
+		       int flags)
+{
+	int *has_errors = opt->value;
+	const char *prefix = startup_info->prefix;

Doesn't the context also contain this? I know this is why you included
patch 3, but it doesn't seem strictly necessary to use startup_info over
ctx.

+	setup_work_tree();
+	*has_errors = do_reupdate(ctx->argc, ctx->argv,
+				  prefix, !prefix ? 0 : strlen(prefix));
+	ctx->argv += ctx->argc - 1;
+	ctx->argc = 1;

At first I thought you forgot to make this a -= here. Then I realized
you're doing this to make parse options skip to the end of processing.
Perhaps you can just return PARSE_OPT_FINISH (equal to 1?) or something
to indicate to parse options that you're done parsing options entirely?
Or put a comment there so I don't get confused again.

+	struct option options[] = {
+		OPT_BIT('q', NULL, &refresh_args.flags,
+			"continue refresh even when index needs update",
+			REFRESH_QUIET),

[snip]

+		OPT_SET_INT(0, "verbose", &verbose,
+			"report actions to standard output", 1),
+		{OPTION_CALLBACK, 0, "clear-resolve-undo", NULL, NULL,
+			"(for porcelains) forget saved unresolved conflicts",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, resolve_undo_clear_cb},
+		OPT_END()
+	};

Any reason for OPT_SET_INT over OPT_BOOLEAN? Just curious.

-			if (!strcmp(path, "-h") || !strcmp(path, "--help"))
-				usage(update_index_usage);
-			die("unknown option %s", path);
+			trace_printf("trace: update-index %s\n", path);

Maybe useful; but doubtful considering we already get the whole command
line already. Debugging stuff?