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

[Stephen Boyd <bebarino@gmail.com>]

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?