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
Jonathan Nieder <jrnieder@gmail.com>
Date
Nov 30, 2010, 02:34 UTC
Message-ID
<20101130023416.GB19191@burratino>
In-Reply-To
<4CC55C4F.5020004@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.
Show 6 quoted lines
>> +"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.
[...]
Show 10 quoted lines
>> +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.

Show 9 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.

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.
Show 7 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.
Yep, needs a comment.
Show 14 quoted lines
>> +	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

Previous: Stephen BoydNext: Nguyen Thai Ngoc Duy
Message 19 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.