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

Hi,

Stephen Boyd wrote:

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

quoted

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

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

That and added option descriptions.

quoted

+"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.

Good idea.  I put

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

even though that doesn't explain the actual syntax (especially
oddities like --stdin and --unresolve) at all. :)

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.

Fixed.

[...]

quoted

+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.

Agreed but I'm not doing it now, since I don't want to encourage other
commands.  Will reconsider.

quoted

+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.

I figure patch 3 is inevitable anyway.  It didn't seem right to
peek at the context, which only includes it as an implementation
detail of OPTION_FILENAME support.

Maybe eliminating ctx->prefix would be a good follow-up patch.

quoted

+	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.

Yep, needs a comment.

quoted

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

[snip]

quoted

+		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.

OPT_BOOLEAN means OPT_INCR and there is only one level of verbosity
here.

quoted

+			trace_printf("trace: update-index %s\n", path);

[...]

              Debugging stuff?

Yes, good catch.

Thanks again for all the comments, and sorry to take so long to get
back to this.

Regards,
Jonathan