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