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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Stephen Boyd wrote:
> 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?

That and added option descriptions.

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

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

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

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

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

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

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