git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Stephen Boyd <bebarino@gmail.com>
Date
Oct 25, 2010, 10:30 UTC
Message-ID
<4CC55C4F.5020004@gmail.com>
In-Reply-To
<20101024081844.GE29630@burratino>
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?
Show 7 quoted lines
>  
> -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?
Show 7 quoted lines
> +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.
Show 6 quoted lines
> +	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.

Show 23 quoted lines
> +	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.

Show 5 quoted lines
> +	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?

Show 5 quoted lines
> +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.

Show 5 quoted lines
> +	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]
Show 7 quoted lines
> +		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?

Previous: Jonathan NiederNext: Jonathan Nieder
Message 18 of 45 in “setup_git_env: print the true $GIT_DIR for debugging”
  1. 1/4 setup_git_env: print the true $GIT_DIR for debuggingNguyễn Thái Ngọc Duy, Oct 20, 2010
  2. 2/4 run_builtin(): save "-h" detection result for later useNguyễn Thái Ngọc Duy, Oct 20, 2010
  3. Junio C HamanoOct 21, 2010
  4. Nguyen Thai Ngoc DuyOct 22, 2010
  5. 3/4 builtins: utilize startup_info->help where possibleNguyễn Thái Ngọc Duy, Oct 20, 2010
  6. 4/4 builtins: check for startup_info->help, print and exit earlyNguyễn Thái Ngọc Duy, Oct 20, 2010
  7. 0/7 Jonathan Nieder, Oct 22, 2010
  8. 1/7 branch -h: show usage even in an invalid repositoryJonathan Nieder, Oct 22, 2010
  9. Junio C HamanoOct 22, 2010
  10. Jonathan NiederOct 24, 2010
  11. 0/4 update-index: migrate to parse-options APIJonathan Nieder, Oct 24, 2010
  12. 1/4 parse-options: allow git commands to invent new option typesJonathan Nieder, Oct 24, 2010
  13. Stephen BoydOct 25, 2010
  14. Jonathan NiederNov 29, 2010
  15. 2/4 parse-options: make resuming easier after PARSE_OPT_STOP_AT_NON_OPTIONJonathan Nieder, Oct 24, 2010
  16. 3/4 setup: save prefix (original cwd relative to toplevel) in startup_infoJonathan Nieder, Oct 24, 2010
  17. 4/4 update-index: migrate to parse-options APIJonathan Nieder, Oct 24, 2010
  18. Stephen BoydOct 25, 2010
  19. Jonathan NiederNov 30, 2010
  20. Nguyen Thai Ngoc DuyOct 24, 2010
  21. Junio C HamanoOct 27, 2010
  22. 0/6 update-index: migrate to parse-options APIJonathan Nieder, Nov 30, 2010
  23. 1/6 parse-options: sanity check PARSE_OPT_NOARG flagJonathan Nieder, Nov 30, 2010
  24. Stephen BoydNov 30, 2010
  25. Junio C HamanoDec 1, 2010
  26. Stephen BoydDec 3, 2010
  27. Junio C HamanoDec 1, 2010
  28. 2/6 parse-options: do not infer PARSE_OPT_NOARG from option typeJonathan Nieder, Nov 30, 2010
  29. Stephen BoydNov 30, 2010
  30. 3/6 parse-options: allow git commands to invent new option typesJonathan Nieder, Nov 30, 2010
  31. 4/6 parse-options: make resuming easier after PARSE_OPT_STOP_AT_NON_OPTIONJonathan Nieder, Nov 30, 2010
  32. 5/6 setup: save prefix (original cwd relative to toplevel) in startup_infoJonathan Nieder, Nov 30, 2010
  33. 6/6 update-index: migrate to parse-options APIJonathan Nieder, Nov 30, 2010
  34. Stephen BoydNov 30, 2010
  35. parse-options: always show arghelp when LITERAL_ARGHELP is setJonathan Nieder, Nov 30, 2010
  36. 2/7 checkout-index -h: show usage even in an invalid repositoryJonathan Nieder, Oct 22, 2010
  37. 3/7 commit/status -h: show usage even with broken configurationJonathan Nieder, Oct 22, 2010
  38. 4/7 gc -h: show usage even with broken configurationJonathan Nieder, Oct 22, 2010
  39. 5/7 ls-files -h: show usage even with corrupt indexJonathan Nieder, Oct 22, 2010
  40. Junio C HamanoOct 22, 2010
  41. 6/7 merge -h: show usage even with corrupt indexJonathan Nieder, Oct 22, 2010
  42. Junio C HamanoOct 22, 2010
  43. 7/7 update-index -h: show usage even with corrupt indexJonathan Nieder, Oct 22, 2010
  44. 0/7 avoid repository access during "git <foo> -h"Jonathan Nieder, Oct 22, 2010
  45. Nguyen Thai Ngoc DuyOct 22, 2010

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.