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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html