setup_git_env: print the true $GIT_DIR for debugging
PATCH v2 1/4 setup_git_env: print the true $GIT_DIR for debugging
Ideally, GIT_DIR_ENVIRONMENT should be set before setup_git_env() is called and "setup: GIT_DIR defaults to .git" should never be printed out. If it is printed, $GIT_DIR is set up automatically, unexpectedly, elsewhere.
Checking for that line is a good way to know if setup code works properly.
---
Second try. Better tests.
environment.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
environment.c +5 -1
@@ -91,8 +91,12 @@ static void setup_git_env(void) git_dir =read_gitfile_gently (DEFAULT_GIT_DIR_ENVIRONMENT ); git_dir = git_dir ?xstrdup (git_dir) :NULL ; } -if (!git_dir) +if (!git_dir) { +trace_printf ("setup: GIT_DIR defaults to .git\n" ); git_dir =DEFAULT_GIT_DIR_ENVIRONMENT ; + } +else +trace_printf ("setup: GIT_DIR set to %s\n" , git_dir); git_object_dir =getenv (DB_ENVIRONMENT );if (!git_object_dir) { git_object_dir =xmalloc (strlen (git_dir) +9 );
Signature · 1 line
1.7.0.2.445.gcbdb3
PATCH v2 2/4 run_builtin(): save "-h" detection result for later use
When run_builtin() sees "-h" as the first argument, it assumes:
- this is the call for help usage
- the real git command will only print help usage then exit
So it skips all setup in this case. Unfortunately, some commands do other things before calling parse_options(), which is often where the help usage is printed. Some of those things may try to access the repository unnecessarily. If a repository is broken, the command may die() before it prints help usage, not really helpful.
Make real commands aware of this fast path so that they can handle it properly (i.e., print help usage then exit immediately) if they were going to do more initialization than git_config().
Demonstrating "git foo -h" fails depends on individual commands and is generally difficult to do. Instead GIT_TRACE is used to check if a command does set repo. If it does, it is supposed to fail if repo setup code chokes.
"git upload-archive" fails for another reason, but will be fixed too when "git upload-archive -h" is converted to use startup_info->help
--- cache.h | 1 + git.c | 18 ++++++++++++++---- t/t3905-help.sh | 24 ++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 4 deletions(-) create mode 100755 t/t3905-help.sh
cache.h +1 -0
@@ -1117,6 +1117,7 @@ const char *split_cmdline_strerror(int cmdline_errno);/* git.c */ struct startup_info {int have_repository; +int help;/* git foo -h */ };extern struct startup_info *startup_info;
git.c +14 -4
@@ -246,13 +246,23 @@ struct cmd_struct {static int run_builtin (struct cmd_struct *p,int argc,const char **argv) { -int status, help; +int status;struct stat st;const char *prefix; prefix =NULL ; - help = argc ==2 && !strcmp (argv[1 ],"-h" ); -if (!help) { + startup_info->help = argc ==2 && !strcmp (argv[1 ],"-h" ); +if (startup_info->help) { +/* +* Fast path for "git foo -h", no setup is done. +* Other functions might set .git up automatically +* and potentially die() along the way. It's best +* to check this flag from the beginning, print its +* help usage and exit, nothing more. +*/ + ; + } +else {if (p->option &RUN_SETUP ) prefix =setup_git_directory ();if (p->option &RUN_SETUP_GENTLY ) {
@@ -267,7 +277,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv) }commit_pager_choice (); -if (!help && p->option &NEED_WORK_TREE ) +if (!startup_info->help && p->option &NEED_WORK_TREE )setup_work_tree ();trace_argv_printf (argv,"trace: built-in: git" );
t/t3905-help.sh new file +24 -0
@@ -0,0 +1,24 @@ +#!/bin/sh + +test_description='tests that git foo -h should work even in potentially broken repos' + +. ./test-lib.sh + +test_help() { + test_expect_"$1" "$2 -h" " + GIT_TRACE=\"`pwd`\"/$2.log test_must_fail git $2 -h && + test \$exit_code = 129 && + ! grep 'defaults to' $2.log + " +} + +test_help failure branch +test_help failure checkout-index +test_help failure commit +test_help failure gc +test_help failure ls-files +test_help failure merge +test_help failure update-index +test_help failure upload-archive + +test_done
Signature · 1 line
1.7.0.2.445.gcbdb3
Re: [PATCH v2 2/4] run_builtin(): save "-h" detection result for later use
Quoted 10 lines · Nguyễn Thái Ngọc Duy
When
run_builtin()sees "-h" as the first argument, it assumes:- this is the call for help usage
- the real git command will only print help usage then exitSo it skips all setup in this case. Unfortunately, some commands do other things before calling
parse_options(), which is often where the help usage is printed. Some of those things may try to access the repository unnecessarily. If a repository is broken, the command maydie()before it prints help usage, not really helpful.
What does die() message say in that case? If it says "your repository is broken", that may be more useful than giving a help message. I dunno.
Quoted 4 lines
Demonstrating "git foo -h" fails depends on individual commands and is generally difficult to do. Instead
GIT_TRACEis used to check if a command does set repo. If it does, it is supposed to fail if repo setup code chokes.
Hmm, I am not sure I understand this one. If you are interested in changing the behaviour of these commands when run with "-h" in a corrupt repository, perhaps you can deliberately corrupt the test repository in the trash directory you start with, and run these commands there, no? For a good measure, you could use CEILING_DIRECTORIES to make sure the tests do not climb up to the project repository.
Re: [PATCH v2 2/4] run_builtin(): save "-h" detection result for later use
2010/10/22 Junio C Hamano <[email protected]>:
Quoted 7 lines
Quoted 5 lines · Nguyễn Thái Ngọc Duy
When
run_builtin()sees "-h" as the first argument, it assumes:- this is the call for help usage
- the real git command will only print help usage then exit
Quoted 8 lines
Quoted 5 lines
So it skips all setup in this case. Unfortunately, some commands do other things before calling
parse_options(), which is often where the help usage is printed. Some of those things may try to access the repository unnecessarily. If a repository is broken, the command maydie()before it prints help usage, not really helpful.What does
die()message say in that case? If it says "your repository is broken", that may be more useful than giving a help message. I dunno.
I should have written "if repository access is broken" (i.e. .git has not been found, then somewhere access to .git is requested and .git is set up automatically). But I'm chasing a ghost here. And the impact to "-h" is probably nothing (how can accessing a wrong .git impacts a static help string?). I'll take the series back. There are more important things to work on than this.
--
Duy
PATCH v2 3/4 builtins: utilize startup_info->help where possible
In reply to Nguyễn Thái Ngọc DuyIt helps reduce false alarms while I'm looking for "git foo -h" code path that accesses repository. Anyway it looks like a good thing to do. If one day people like to have "git foo --help" as an alternative to "git foo -h", it would be easy.
--- builtin/check-ref-format.c | 2 +- builtin/grep.c | 2 +- builtin/index-pack.c | 2 +- builtin/log.c | 6 +----- builtin/merge-ours.c | 2 +- builtin/pack-redundant.c | 2 +- builtin/show-ref.c | 2 +- 7 files changed, 7 insertions(+), 11 deletions(-)
builtin/check-ref-format.c +1 -1
@@ -58,7 +58,7 @@ static int check_ref_format_print(const char *arg)int cmd_check_ref_format (int argc,const char **argv,const char *prefix) { -if (argc ==2 && !strcmp (argv[1 ],"-h" )) +if (startup_info->help)usage (builtin_check_ref_format_usage);if (argc ==3 && !strcmp (argv[1 ],"--branch" ))
builtin/grep.c +1 -1
@@ -934,7 +934,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)* 'git grep -h', unlike 'git grep -h <pattern>', is a request * to show usage information and exit. */ -if (argc ==2 && !strcmp (argv[1 ],"-h" )) +if (startup_info->help)usage_with_options (grep_usage, options);memset (&opt,0 ,sizeof (opt));
builtin/index-pack.c +1 -1
@@ -881,7 +881,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)struct pack_idx_entry **idx_objects;unsigned char pack_sha1[20 ]; -if (argc ==2 && !strcmp (argv[1 ],"-h" )) +if (startup_info->help)usage (index_pack_usage); read_replace_refs =0 ;
builtin/log.c +1 -5
@@ -69,11 +69,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,if (default_date_mode) rev->date_mode =parse_date_format (default_date_mode); -/* -* Check for -h before setup_revisions(), or "git log -h" will -* fail when run without a git directory. -*/ -if (argc ==2 && !strcmp (argv[1 ],"-h" )) +if (startup_info->help)usage (builtin_log_usage); argc =setup_revisions (argc, argv, rev, opt);
builtin/merge-ours.c +1 -1
@@ -20,7 +20,7 @@ static const char *diff_index_args[] = {int cmd_merge_ours (int argc,const char **argv,const char *prefix) { -if (argc ==2 && !strcmp (argv[1 ],"-h" )) +if (startup_info->help)usage (builtin_merge_ours_usage);/*
builtin/pack-redundant.c +1 -1
@@ -601,7 +601,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)unsigned char *sha1;char buf[42 ];/* 40 byte sha1 + \n + \0 */ -if (argc ==2 && !strcmp (argv[1 ],"-h" )) +if (startup_info->help)usage (pack_redundant_usage);for (i =1 ; i < argc; i++) {
builtin/show-ref.c +1 -1
@@ -204,7 +204,7 @@ static const struct option show_ref_options[] = {int cmd_show_ref (int argc,const char **argv,const char *prefix) { -if (argc ==2 && !strcmp (argv[1 ],"-h" )) +if (startup_info->help)usage_with_options (show_ref_usage, show_ref_options); argc = parse_options(argc, argv, prefix, show_ref_options,
Signature · 1 line
1.7.0.2.445.gcbdb3
PATCH v2 4/4 builtins: check for startup_info->help, print and exit early
In reply to Nguyễn Thái Ngọc DuyBy ensuring no access to repo is done in "git foo -h" case, the commands have better chance of really printing out help usage. Access to repo is not necessary and may terminate program if it finds something wrong.
--- builtin/branch.c | 3 +++ builtin/checkout-index.c | 3 +++ builtin/commit.c | 6 ++++++ builtin/gc.c | 3 +++ builtin/ls-files.c | 3 +++ builtin/merge.c | 3 +++ builtin/update-index.c | 3 +++ builtin/upload-archive.c | 7 ++++--- t/t3905-help.sh | 16 ++++++++-------- 9 files changed, 36 insertions(+), 11 deletions(-)
builtin/branch.c +3 -0
@@ -667,6 +667,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix) OPT_END(), }; +if (startup_info->help) +usage_with_options (builtin_branch_usage, options); +git_config (git_branch_config,NULL );if (branch_use_color ==-1 )
builtin/checkout-index.c +3 -0
@@ -241,6 +241,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) OPT_END() }; +if (startup_info->help) +usage_with_options (builtin_checkout_index_usage, builtin_checkout_index_options); +git_config (git_default_config,NULL ); state.base_dir ="" ; prefix_length = prefix ?strlen (prefix) :0 ;
builtin/commit.c +6 -0
@@ -1070,6 +1070,9 @@ int cmd_status(int argc, const char **argv, const char *prefix) OPT_END(), }; +if (startup_info->help) +usage_with_options (builtin_status_usage, builtin_status_options); +if (null_termination && status_format ==STATUS_FORMAT_LONG ) status_format =STATUS_FORMAT_PORCELAIN ;
@@ -1255,6 +1258,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)int allow_fast_forward =1 ;struct wt_status s; +if (startup_info->help) +usage_with_options (builtin_commit_usage, builtin_commit_options); +wt_status_prepare (&s);git_config (git_commit_config, &s); in_merge =file_exists (git_path ("MERGE_HEAD" ));
builtin/gc.c +3 -0
@@ -189,6 +189,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix) OPT_END() }; +if (startup_info->help) +usage_with_options (builtin_gc_usage, builtin_gc_options); +git_config (gc_config,NULL );if (pack_refs <0 )
builtin/ls-files.c +3 -0
@@ -530,6 +530,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) OPT_END() }; +if (startup_info->help) +usage_with_options (ls_files_usage, builtin_ls_files_options); +memset (&dir,0 ,sizeof (dir)); prefix = cmd_prefix;if (prefix)
builtin/merge.c +3 -0
@@ -917,6 +917,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)const char *best_strategy =NULL , *wt_strategy =NULL ;struct commit_list **remotes = &remoteheads; +if (startup_info->help) +usage_with_options (builtin_merge_usage, builtin_merge_options); +if (read_cache_unmerged ()) {die_resolve_conflict ("merge" ); }
builtin/update-index.c +3 -0
@@ -589,6 +589,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)int lock_error =0 ;struct lock_file *lock_file; +if (startup_info->help) +usage (update_index_usage); +git_config (git_default_config,NULL );/* We can't free this memory, it becomes part of a linked list parsed atexit() */
builtin/upload-archive.c +4 -3
@@ -26,9 +26,6 @@ static int run_upload_archive(int argc, const char **argv, const char *prefix)int sent_argc;int len; -if (argc !=2 ) -usage (upload_archive_usage); -if (strlen (argv[1 ]) +1 >sizeof (buf))die ("insanely long repository name" );
@@ -98,6 +95,10 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix) {pid_t writer;int fd1[2 ], fd2[2 ]; + +if (startup_info->help || argc !=2 ) +usage (upload_archive_usage); +/* * Set up sideband subprocess. *
t/t3905-help.sh +8 -8
@@ -12,13 +12,13 @@ test_help() { " } -test_help failure branch -test_help failure checkout-index -test_help failure commit -test_help failure gc -test_help failure ls-files -test_help failure merge -test_help failure update-index -test_help failure upload-archive +test_help success branch +test_help success checkout-index +test_help success commit +test_help success gc +test_help success ls-files +test_help success merge +test_help success update-index +test_help success upload-archive test_done
Signature · 1 line
1.7.0.2.445.gcbdb3
cover [PATCH en/and-cascade-tests 0/7]
Quoted 8 lines · Nguyễn Thái Ngọc Duy
builtin/branch.c | 3 +++ builtin/checkout-index.c | 3 +++ builtin/commit.c | 6 ++++++ builtin/gc.c | 3 +++ builtin/ls-files.c | 3 +++ builtin/merge.c | 3 +++ builtin/update-index.c | 3 +++ builtin/upload-archive.c | 7 ++++---
Okay, all of them survive a reroll except upload-archive. As for upload-archive:
- it already doesn't support "-h"
- it is such low-level plumbing, I don't really mind that. Maybe someone is using a repository named "-h" (though I hope not, of course).
Maybe these patches should be squashed. I separated them because they were easier to write and it would be easier to cc the right people this way.
The primary motivation for the series is that the repository checker from the nd/setup series will not be happy without some change like this. In the git <foo> -h codepath, repository setup is not run so repository access is forbidden.
The secondary motivation is to create better behavior in situations of repository corruption. When I type "git checkout -h", I am asking for a usage message, not a repository self-check. Especially when trying to repair a repository, commands that do not do what they are asked to are generally frustrating to use.
Patches apply on top of "test-lib: make test_expect_code a test command" from the en/and-cascade-tests topic.
Enjoy,
Jonathan
Nguyễn Thái Ngọc Duy (7):
branch -h: show usage even in an invalid repository checkout-index -h: show usage even in an invalid repository commit/status -h: show usage even with broken configuration gc -h: show usage even with broken configuration ls-files -h: show usage even with corrupt index merge -h: show usage even with corrupt index update-index -h: show usage even with corrupt index
builtin/branch.c | 3 +++ builtin/checkout-index.c | 3 +++ builtin/commit.c | 6 ++++++ builtin/gc.c | 3 +++ builtin/ls-files.c | 3 +++ builtin/merge.c | 2 ++ builtin/update-index.c | 3 +++ t/t2006-checkout-index-basic.sh | 24 ++++++++++++++++++++++++ t/t2107-update-index-basic.sh | 32 ++++++++++++++++++++++++++++++++ t/t3004-ls-files-basic.sh | 39 +++++++++++++++++++++++++++++++++++++++ t/t3200-branch.sh | 11 +++++++++++ t/t6500-gc.sh | 28 ++++++++++++++++++++++++++++ t/t7508-status.sh | 24 ++++++++++++++++++++++++ t/t7600-merge.sh | 11 +++++++++++ 14 files changed, 192 insertions(+), 0 deletions(-) create mode 100755 t/t2006-checkout-index-basic.sh create mode 100755 t/t2107-update-index-basic.sh create mode 100755 t/t3004-ls-files-basic.sh create mode 100755 t/t6500-gc.sh
Signature · 1 line
1.7.2.3
PATCH 1/7 branch -h: show usage even in an invalid repository
From: Nguyễn Thái Ngọc Duy <[email protected]>
There is no need for "git branch -h" to try to access a repository.
In the spirit of v1.6.6-rc0~34^2~3 (Let 'git <command> -h' show usage without a git dir, 2009-11-09). This brings git one step closer to passing the following (automatically verifiable) test:
Before any repository access (aside from git_config()), a function from the setup_git_directory_* family has been run
and thus one step closer to being able to use an automatic repository access checker.
[jn: simplified; new commit message, test]
--- builtin/branch.c | 3 +++ t/t3200-branch.sh | 11 +++++++++++ 2 files changed, 14 insertions(+), 0 deletions(-)
builtin/branch.c +3 -0
@@ -667,6 +667,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix) OPT_END(), }; +if (argc ==2 && !strcmp (argv[1 ],"-h" )) +usage_with_options (builtin_branch_usage, options); +git_config (git_branch_config,NULL );if (branch_use_color ==-1 )
t/t3200-branch.sh +11 -0
@@ -26,6 +26,17 @@ test_expect_success \ ! test -f .git/refs/heads/--help ' +test_expect_success 'branch -h in broken repository' ' + mkdir broken && + ( + cd broken && + git init && + >.git/refs/heads/master && + test_expect_code 129 git branch -h >usage 2>&1 + ) && + grep "[Uu]sage" broken/usage +' + test_expect_success \ 'git branch abc should create a branch' \ 'git branch abc && test -f .git/refs/heads/abc'
Signature · 1 line
1.7.2.3
Re: [PATCH 1/7] branch -h: show usage even in an invalid repository
Quoted 1 line · Jonathan Nieder
[jn: simplified; new commit message, test]
Overall (not just this 1/7) the messages justify the changes a lot better. Thanks for a pleasant read ;-).
Quoted 8 lines
t/t3200-branch.sh +0
-0@@ -26,6 +26,17 @@ test_expect_success \ ! test -f .git/refs/heads/--help '
t/t3200-branch.sh test_expect_success \ · line 38
Quoted 10 lines
t/t3200-branch.sh +10
-0+test_expect_success 'branch -h in broken repository' ' + mkdir broken && + ( + cd broken && + git init && + >.git/refs/heads/master && + test_expect_code 129 git branch -h >usage 2>&1 + ) && + grep "[Uu]sage" broken/usage +'
A handful of points that struck me (not just test, but taking tests from other patches together) as slightly odd, all minor:
- Why '[Uu]sage"? Should we make the messages consistent further?
- The final "grep" check may want to make sure that the message is free of "fatal" (or "error", "warning", etc.) as well?
- Each test seems to anticipate a specific kind of breakage tailored for the command being tested (e.g. "branch" test does not corrupt config nor the index). Perhaps "lib-corrupt.sh" helper to run the same test in variety of corrupt repositories would help improve coverage? [*1*]
- Some tests redirect both the standard output and the standard error (like this patch) and check the combined result, while some others (e.g. 2/7) check only the standard error stream. Don't we want to be testing them more uniformly?
t/t3200-branch.sh test_expect_success \ · line 42
Quoted 3 lines
t/t3200-branch.sh +0
-0test_expect_success \ 'git branch abc should create a branch' \ 'git branch abc && test -f .git/refs/heads/abc'
Don't we want to rather use resolve-ref or "rev-parse --verify", just in case we may later change "git branch" to update packed-refs directly?
[Footnote]
*1* I am of two minds, as for example a corrupt "gc.pruneexpire" cannot possibly matter for correct operation of "git branch", but am just throwing out an idea to see if somebody else have clever ideas.
Re: [PATCH 1/7] branch -h: show usage even in an invalid repository
Quoted 4 lines · Junio C Hamano
- Some tests redirect both the standard output and the standard error (like this patch) and check the combined result, while some others (e.g. 2/7) check only the standard error stream. Don't we want to be testing them more uniformly?
Good point. The current state is:
- Messages from a mistake in usage are reported to stderr.
- Messages from git <command> -h are reported to stdout or stderr, with a slight preference for stdout.
A nicer behavior would be
- Messages from a mistake in usage are reported to stderr.
- Messages from git <command> -h are reported to stdout.
Here's a helper to make it easier for commands that use parse-options to adopt that nicer behavior. It writes its output to stdout, so it should only be used to be used to handle the -h option. Example:
if (argc == 2 && !strcmp(argv[1], "-h")) usage_for_help_opt(builtin_foo_usage, options); ... some scary commands that should not run with -h ... argc = parse_options(argc, ... if (argc != 1) usage_with_options(builtin_foo_usage, options);
---
parse-options.c +7 -0
@@ -567,6 +567,13 @@ void usage_with_options(const char * const *usagestr,exit (129 ); } +void usage_for_help_opt (const char *const *usagestr, +const struct option *opts) +{ +usage_with_options_internal (NULL , usagestr, opts,0 ,0 ); +exit (129 ); +} +void usage_msg_opt (const char *msg,const char *const *usagestr,const struct option *options)
parse-options.h +3 -0
@@ -152,6 +152,9 @@ extern int parse_options(int argc, const char **argv, const char *prefix, extern NORETURN void usage_with_options(const char * const *usagestr,const struct option *options); +extern NORETURN void usage_for_help_opt(const char * const *usagestr, +const struct option *options); + extern NORETURN void usage_msg_opt(const char *msg,const char *const *usagestr,const struct option *options);
cover update-index: migrate to parse-options API
Quoted 3 lines · Jonathan Nieder
Here's a helper to make it easier for commands that use parse-options to adopt that nicer behavior. It writes its output to stdout, so it should only be used to be used to handle the -h option.
Alas, "git update-index" does not use parse-options yet. But that is easily enough changed...
Patch 1 introduces an OPTION_LOWLEVEL_CALLBACK backdoor to parse-options, so new option types (like the three-argument type used by update-index --cacheinfo) can be supported without tempting inventors of other commands through mention in the public API.
Patch 2 is a toy patch I previously send to Bo. It makes parse_option_step() distinguish between stops due to end of options and stops due to appearance of a nonoption; update-index benefits from something like this because it needs to deal with filename parameters as they appear.
Patch 3 converts update-index to parse-options. The commit message mentions some potential simplifications.
All three patches ought to introduce new tests, but none do. I hope that does not hinder reading them too much. Thoughts?
Jonathan Nieder (3):
parse-options: allow git commands to invent new option types
parse-options: make resuming easier after
PARSE_OPT_STOP_AT_NON_OPTION
update-index: migrate to parse-options API
Nguyễn Thái Ngọc Duy (1):
setup: save prefix (original cwd relative to toplevel) in
startup_info
builtin/update-index.c | 389 +++++++++++++++++++++++++++++------------------ cache.h | 1 + parse-options.c | 9 +- parse-options.h | 10 ++ setup.c | 4 +- 5 files changed, 259 insertions(+), 154 deletions(-)
Signature · 1 line
1.7.2.3
PATCH 1/4 parse-options: allow git commands to invent new option types
parse-options provides a variety of option behaviors, including OPTION_CALLBACK, which should take care of just about any sane behavior. All supported behaviors obey the following constraint:
A --foo option can only accept (and base its behavior on) one argument, which would be the following command-line argument in the "unsticked" form.
Alas, some existing git commands have options that do not obey that constraint. For example, update-index --cacheinfo takes three arguments, and update-index --resolve takes all later parameters as arguments.
Introduce a new option type (OPTION_LOWLEVEL_CALLBACK) to support such unusual options. parse_options() callers can implement an arbitrary custom get_value() function to override the usual one and pass it through the callback field for options of interest.
--- parse-options.c | 6 +++--- parse-options.h | 9 +++++++++ 2 files changed, 12 insertions(+), 3 deletions(-)
parse-options.c +3 -3
@@ -8,9 +8,6 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,const char *const *usagestr, const struct option *opts, int err); -#define OPT_SHORT 1 -#define OPT_UNSET 2 -static int opterror (const struct option *opt,const char *reason,int flags) {if (flags &OPT_SHORT )
@@ -51,6 +48,9 @@ static int get_value(struct parse_opt_ctx_t *p,const int unset = flags &OPT_UNSET ;int err; +if (opt->type ==OPTION_LOWLEVEL_CALLBACK ) +return (*(parse_opt_ll_cb *)opt->callback)(p, opt, flags); +if (unset && p->opt)return opterror (opt,"takes no value" , flags);if (unset && (opt->flags &PARSE_OPT_NONEG ))
parse-options.h +9 -0
@@ -17,6 +17,7 @@ enum parse_opt_type {OPTION_STRING ,OPTION_INTEGER ,OPTION_CALLBACK , +OPTION_LOWLEVEL_CALLBACK , OPTION_FILENAME };
@@ -40,8 +41,16 @@ enum parse_opt_option_flags { PARSE_OPT_SHELL_EVAL = 256 }; +enum parse_opt_ll_flags { +OPT_SHORT =1 , +OPT_UNSET =2 +}; +struct option ; +struct parse_opt_ctx_t ;typedef int parse_opt_cb (const struct option *,const char *arg,int unset); +typedef int parse_opt_ll_cb (struct parse_opt_ctx_t *ctx, +const struct option *opt,int flags);/* * `type`::
Signature · 1 line
1.7.2.3
Re: [PATCH 1/4] parse-options: allow git commands to invent new option types
Quoted 15 lines · Jonathan Nieder
parse-options.c +0
-3@@ -8,9 +8,6 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,const char *const *usagestr, const struct option *opts, int err); -#define OPT_SHORT 1 -#define OPT_UNSET 2 -static int opterror (const struct option *opt,const char *reason,int flags) {if (flags &OPT_SHORT )
parse-options.c static int get_value(struct parse_opt_ctx_t *p, · line 53
Quoted 6 lines
parse-options.c +3
-0@@ -51,6 +48,9 @@ static int get_value(struct parse_opt_ctx_t *p,const int unset = flags &OPT_UNSET ;int err; +if (opt->type ==OPTION_LOWLEVEL_CALLBACK ) +return (*(parse_opt_ll_cb *)opt->callback)(p, opt, flags); +
(I read patch 4 before this one)
Being able to modify the context within a callback is nice. Having to know if the option is short or long and and checking for validity seems like something that should be handled within the parse options library itself.
Is there an actual use case where someone needs to completely override get_value()? If you move this into the case statement then we get the generic error checking of get_value() with the benefits of being able to modify the context within a callback. We could also probably use the return value of the low level callback to indicate whether or not to take some action after parsing the option. Perhaps something like quiting the option parsing loop when encountering such an option?
This reminds me, we can probably simplify that "takes no value" error path in get_value() (see below).
parse-options.h enum parse_opt_type { · line 22
Quoted 11 lines
parse-options.h +1
-0@@ -17,6 +17,7 @@ enum parse_opt_type {OPTION_STRING ,OPTION_INTEGER ,OPTION_CALLBACK , +OPTION_LOWLEVEL_CALLBACK , OPTION_FILENAME };
Don't forget to document what this option does in the comments of this file and in api-parse-options.txt
parse-options.h enum parse_opt_option_flags { · line 48
Quoted 10 lines
parse-options.h +5
-0@@ -40,8 +41,16 @@ enum parse_opt_option_flags { PARSE_OPT_SHELL_EVAL = 256 }; +enum parse_opt_ll_flags { +OPT_SHORT =1 , +OPT_UNSET =2 +}; +
I hope this isn't necessary.
Hope this pasted ok.
--->8----
parse-options.c +1 -11
@@ -56,22 +56,8 @@ static int get_value(struct parse_opt_ctx_t *p,if (unset && (opt->flags &PARSE_OPT_NONEG ))return opterror (opt,"isn't available" , flags); -if (!(flags &OPT_SHORT ) && p->opt) { -switch (opt->type) { -case OPTION_CALLBACK : -if (!(opt->flags &PARSE_OPT_NOARG )) -break ; -/* FALLTHROUGH */ -case OPTION_BOOLEAN : -case OPTION_BIT : -case OPTION_NEGBIT : -case OPTION_SET_INT : -case OPTION_SET_PTR : + if (!(flags & OPT_SHORT) && p->opt && (opt->flags &
PARSE_OPT_NOARG))
return opterror(opt, "takes no value", flags);
- default:
- break;
- }
- }
switch (opt->type) {
case OPTION_BIT:Re: [PATCH 1/4] parse-options: allow git commands to invent new option types
Quoted 4 lines · Stephen Boyd
Is there an actual use case where someone needs to completely override
get_value()? If you move this into the case statement then we get the generic error checking ofget_value()with the benefits of being able to modify the context within a callback.
Yes, I like this idea.
Quoted 4 lines
We could also probably use the
return value of the low level callback to indicate whether or not to take some action after parsing the option. Perhaps something like quiting the option parsing loop when encountering such an option?
I'd rather not, since that would involve anticipating needs in advance. This is meant to be a back door for ugly and unusual option types that the mainstream API does not take care of yet. It seems best to allow arbitrary effects.
[...]
Quoted 2 lines
Quoted 2 lines
(quoted diff) +0
-2-#define OPT_SHORT 1 -#define OPT_UNSET 2
[...]
Quoted 5 lines
Quoted 5 lines
+++
b/parse-options.h(quoted diff) +0
-0@@ -40,8 +41,16 @@ enum parse_opt_option_flags { PARSE_OPT_SHELL_EVAL = 256 };
Quoted 7 lines
Review enum parse_opt_option_flags { · line 48
Quoted 5 lines
(quoted diff) +5
-0+enum parse_opt_ll_flags { + OPT_SHORT = 1, + OPT_UNSET = 2 +}; +I hope this isn't necessary.
Because of namespace or something else?
Quoted 2 lines
This reminds me, we can probably simplify that "takes no value" error path in
get_value()(see below).
Nice.
Thanks,
Jonathan
PATCH 2/4 parse-options: make resuming easier after PARSE_OPT_STOP_AT_NON_OPTION
In reply to Jonathan NiederIntroduce a PARSE_OPT_NON_OPTION state, so parse_option_step() callers can easily distinguish between non-options and other reasons for option parsing termination (like "--").
--- parse-options.c | 3 ++- parse-options.h | 1 + 2 files changed, 3 insertions(+), 1 deletions(-)
parse-options.c +2 -1
@@ -374,7 +374,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,if (parse_nodash_opt (ctx, arg, options) ==0 )continue ;if (ctx->flags &PARSE_OPT_STOP_AT_NON_OPTION ) -break ; +return PARSE_OPT_NON_OPTION ; ctx->out[ctx->cpidx++] = ctx->argv[0 ];continue ; }
@@ -456,6 +456,7 @@ int parse_options(int argc, const char **argv, const char *prefix,switch (parse_options_step (&ctx, options, usagestr)) {case PARSE_OPT_HELP :exit (129 ); +case PARSE_OPT_NON_OPTION :case PARSE_OPT_DONE :break ;default :/* PARSE_OPT_UNKNOWN */
parse-options.h +1 -0
@@ -170,6 +170,7 @@ extern NORETURN void usage_msg_opt(const char *msg,enum {PARSE_OPT_HELP =-1 ,PARSE_OPT_DONE , +PARSE_OPT_NON_OPTION ,PARSE_OPT_UNKNOWN };
Signature · 1 line
1.7.2.3
PATCH 3/4 setup: save prefix (original cwd relative to toplevel) in startup_info
In reply to Jonathan NiederFrom: Nguyễn Thái Ngọc Duy <[email protected]>
Save the path from the original cwd to the cwd at the end of the setup procedure in the startup_info struct introduced in e37c1329 (2010-08-05). The value cannot vary from thread to thread anyway, since the cwd is global.
So now in your builtin command, instead of passing prefix around, when you want to convert a user-supplied path to a cwd-relative path, you can use startup_info->prefix directly.
Caveat: As with the return value from setup_git_directory_gently(), startup_info->prefix would be NULL when the original cwd is not a subdir of the toplevel.
Longer term, this woiuld allow the prefix to be reused when several noncooperating functions require access to the same repository (for example, when accessing configuration before running a builtin).
--- cache.h | 1 + setup.c | 4 +++- 2 files changed, 4 insertions(+), 1 deletions(-)
cache.h +1 -0
@@ -1117,6 +1117,7 @@ const char *split_cmdline_strerror(int cmdline_errno);/* git.c */ struct startup_info {int have_repository; +const char *prefix; };extern struct startup_info *startup_info;
setup.c +3 -1
@@ -512,8 +512,10 @@ const char *setup_git_directory_gently(int *nongit_ok)const char *prefix; prefix =setup_git_directory_gently_1 (nongit_ok); -if (startup_info) +if (startup_info) { startup_info->have_repository = !nongit_ok || !*nongit_ok; + startup_info->prefix = prefix; + }return prefix; }
Signature · 1 line
1.7.2.3
PATCH 4/4 update-index: migrate to parse-options API
In reply to Jonathan Nieder--refresh and --really-refresh accept flags (like -q) and modify an error indicator. It might make sense to make the error indicator global; but just pass the flags and a pointer to the error indicator in a struct instead.
--cacheinfo wants 3 arguments. Use the new OPTION_LOWLEVEL_CALLBACK extension to grab them.
--assume-unchanged and --no-assume-unchanged probably should use the OPT_UYN feature; but use a callback for now so the existing MARK_FLAG and UNMARK_FLAG values can be used.
--stdin and --index-info are still constrained to be the last argument (implemented using the OPTION_LOWLEVEL_CALLBACK extension).
--unresolve and --again consume all arguments that come after them (also using OPTION_LOWLEVEL_CALLBACK).
The order of options matters. Each path on the command line is affected only by the options that come before it; use a custom argument-parsing loop with parse_options_step() to bring that about.
In exchange for all the fuss, we get the usual perks: support for un-sticked options, better usage error messages, more useful -h output, and argument parsing code that should be easier to tweak in the future.
--- builtin/update-index.c | 389 +++++++++++++++++++++++++++++------------------ 1 files changed, 240 insertions(+), 149 deletions(-)
builtin/update-index.c +240 -149
@@ -10,6 +10,7 @@#include "builtin.h" #include "refs.h" #include "resolve-undo.h" +#include "parse-options.h" /* * Default to not allowing changes to the list of files. The
@@ -397,8 +398,10 @@ static void read_index_info(int line_termination)strbuf_release (&uq); } -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 +};static unsigned char head_sha1[20 ];static unsigned char merge_head_sha1[20 ];
@@ -578,16 +581,207 @@ static int do_reupdate(int ac, const char **av,return 0 ; } +struct refresh_params { +unsigned int flags; +int *has_errors; +}; + +static int refresh_cb_1 (const struct option *opt,const char *arg,int unset, +unsigned int flag) +{ +struct refresh_params *o = opt->value; +setup_work_tree (); + *o->has_errors |=refresh_cache (o->flags | flag); +return 0 ; +} + +static int refresh_cb (const struct option *opt,const char *arg,int unset) +{ +return refresh_cb_1 (opt, arg, unset,0 ); +} + +static int really_refresh_cb (const struct option *opt,const char *arg,int unset) +{ +return refresh_cb_1 (opt, arg, unset,REFRESH_REALLY ); +} + +static int chmod_arg_cb (const struct option *opt,const char *arg,int unset) +{ +char *set_executable_bit = opt->value; +if ((arg[0 ] !='-' && arg[0 ] !='+' ) || arg[1 ] !='x' || arg[2 ]) +return error ("argument to --chmod must be +x or -x" ); + *set_executable_bit = arg[0 ]; +return 0 ; +} + +static int resolve_undo_clear_cb (const struct option *opt,const char *arg,int unset) +{ +resolve_undo_clear (); +return 0 ; +} + +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" ); +if (ctx->opt) +return error ("--cacheinfo does not accept an attached argument" ); +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); + 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); +if (flags &OPT_UNSET ) +return error ("--%s cannot be negated" , opt->long_name); + *read_from_stdin =1 ; +return 0 ; +} + +static int unresolve_cb (struct parse_opt_ctx_t *ctx,const struct option *opt, +int flags) +{ +int *has_errors = opt->value; +const char *prefix = startup_info->prefix; + *has_errors =do_unresolve (ctx->argc, ctx->argv, + prefix, !prefix ?0 :strlen (prefix)); + ctx->argv += ctx->argc -1 ; + ctx->argc =1 ; +if (*has_errors) + active_cache_changed =0 ; +return 0 ; +} + +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; +setup_work_tree (); + *has_errors =do_reupdate (ctx->argc, ctx->argv, + prefix, !prefix ?0 :strlen (prefix)); + ctx->argv += ctx->argc -1 ; + ctx->argc =1 ; +if (*has_errors) + active_cache_changed =0 ; +return 0 ; +} +int cmd_update_index (int argc,const char **argv,const char *prefix) { -int i, newfd, entries, has_errors =0 , line_termination ='\n' ; -int allow_options =1 ; +int newfd, entries, has_errors =0 , line_termination ='\n' ;int read_from_stdin =0 ;int prefix_length = prefix ?strlen (prefix) :0 ;char set_executable_bit =0 ; -unsigned int refresh_flags =0 ; +struct refresh_params refresh_args = {0 , &has_errors};int lock_error =0 ;struct lock_file *lock_file; +struct parse_opt_ctx_t ctx; +int parseopt_state =PARSE_OPT_UNKNOWN ; +struct option options[] = { +OPT_BIT ('q' ,NULL , &refresh_args.flags, +"continue refresh even when index needs update" , +REFRESH_QUIET ), +OPT_BIT (0 ,"ignore-submodules" , &refresh_args.flags, +"refresh: ignore submodules" , +REFRESH_IGNORE_SUBMODULES ), +OPT_SET_INT (0 ,"add" , &allow_add, +"do not ignore new files" ,1 ), +OPT_SET_INT (0 ,"replace" , &allow_replace, +"let files replace directories and vice-versa" ,1 ), +OPT_SET_INT (0 ,"remove" , &allow_remove, +"notice files missing from worktree" ,1 ), +OPT_BIT (0 ,"unmerged" , &refresh_args.flags, +"refresh even if index contains unmerged entries" , +REFRESH_UNMERGED ), + {OPTION_CALLBACK ,0 ,"refresh" , &refresh_args,NULL , +"refresh stat information" , +PARSE_OPT_NOARG |PARSE_OPT_NONEG , refresh_cb}, + {OPTION_CALLBACK ,0 ,"really-refresh" , &refresh_args,NULL , +"like --refresh, but ignore assume-unchanged setting" , +PARSE_OPT_NOARG |PARSE_OPT_NONEG , really_refresh_cb}, + {OPTION_LOWLEVEL_CALLBACK ,0 ,"cacheinfo" ,NULL , +"<mode> <object> <path>" , +"add the specified entry to the index" , +PARSE_OPT_LITERAL_ARGHELP , (parse_opt_cb *)cacheinfo_cb}, + {OPTION_CALLBACK ,0 ,"chmod" , &set_executable_bit,"(+/-)x" , +"override the executable bit of the listed files" , +PARSE_OPT_NONEG |PARSE_OPT_LITERAL_ARGHELP , + chmod_arg_cb}, + {OPTION_SET_INT ,0 ,"assume-unchanged" , &mark_valid_only,NULL , +"mark files as \"assumed to be unmodified\"" , +PARSE_OPT_NOARG |PARSE_OPT_NONEG ,NULL , +MARK_FLAG }, + {OPTION_SET_INT ,0 ,"no-assume-unchanged" , &mark_valid_only,NULL , +"unset \"assumed unchanged\" bit" , +PARSE_OPT_NOARG |PARSE_OPT_NONEG ,NULL , +UNMARK_FLAG }, + {OPTION_SET_INT ,0 ,"skip-worktree" , &mark_skip_worktree_only,NULL , +"mark files as \"index-only\"" , +PARSE_OPT_NOARG |PARSE_OPT_NONEG ,NULL , +MARK_FLAG }, + {OPTION_SET_INT ,0 ,"no-skip-worktree" , &mark_skip_worktree_only,NULL , +"unset \"skip worktree\" big" , +PARSE_OPT_NOARG |PARSE_OPT_NONEG ,NULL , +UNMARK_FLAG }, +OPT_SET_INT (0 ,"info-only" , &info_only, +"add to index only; do not add content to object database" ,1 ), +OPT_SET_INT (0 ,"force-remove" , &force_remove, +"remove named paths even if present in worktree" ,1 ), +OPT_SET_INT ('z' ,NULL , &line_termination, +"with --stdin: input lines are terminated by null bytes" ,'\0' ), + {OPTION_LOWLEVEL_CALLBACK ,0 ,"stdin" , &read_from_stdin,NULL , +"read list of paths to be updated from standard input" , +PARSE_OPT_NONEG |PARSE_OPT_NOARG , + (parse_opt_cb *)last_arg_cb}, + {OPTION_LOWLEVEL_CALLBACK ,0 ,"index-info" , &line_termination,NULL , +"add entries from standard input to the index" , +PARSE_OPT_NONEG |PARSE_OPT_NOARG , + (parse_opt_cb *)stdin_cacheinfo_cb}, + {OPTION_LOWLEVEL_CALLBACK ,0 ,"unresolve" , &has_errors,NULL , +"repopulate stages #2 and #3 for the listed paths" , +PARSE_OPT_NONEG |PARSE_OPT_NOARG , + (parse_opt_cb *)unresolve_cb}, + {OPTION_LOWLEVEL_CALLBACK ,'g' ,"again" , &has_errors,NULL , +"only update entries that differ from HEAD" , +PARSE_OPT_NONEG |PARSE_OPT_NOARG , + (parse_opt_cb *)reupdate_cb}, +OPT_BIT (0 ,"ignore-missing" , &refresh_args.flags, +"ignore files missing from worktree" , +REFRESH_IGNORE_MISSING ), +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 () + };git_config (git_default_config,NULL );
@@ -602,151 +796,49 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)if (entries <0 )die ("cache corrupted" ); -for (i =1 ; i < argc; i++) { -const char *path = argv[i]; -const char *p; +/* +* Custom copy of parse_options() because we want to handle +* filename arguments as they come. +*/ +parse_options_start (&ctx, argc, argv, prefix, +PARSE_OPT_STOP_AT_NON_OPTION ); +while (ctx.argc) { +if (parseopt_state !=PARSE_OPT_DONE ) + parseopt_state =parse_options_step (&ctx, options, + update_index_usage); +if (!ctx.argc) +break ; +switch (parseopt_state) { +case PARSE_OPT_HELP : +exit (129 ); +case PARSE_OPT_NON_OPTION : +case PARSE_OPT_DONE : + { +const char *path = ctx.argv[0 ]; +const char *p; -if (allow_options && *path =='-' ) { -if (!strcmp (path,"--" )) { - allow_options =0 ; -continue ; - } -if (!strcmp (path,"-q" )) { - refresh_flags |=REFRESH_QUIET ; -continue ; - } -if (!strcmp (path,"--ignore-submodules" )) { - refresh_flags |=REFRESH_IGNORE_SUBMODULES ; -continue ; - } -if (!strcmp (path,"--add" )) { - allow_add =1 ; -continue ; - } -if (!strcmp (path,"--replace" )) { - allow_replace =1 ; -continue ; - } -if (!strcmp (path,"--remove" )) { - allow_remove =1 ; -continue ; - } -if (!strcmp (path,"--unmerged" )) { - refresh_flags |=REFRESH_UNMERGED ; -continue ; - } -if (!strcmp (path,"--refresh" )) { -setup_work_tree (); - has_errors |=refresh_cache (refresh_flags); -continue ; - } -if (!strcmp (path,"--really-refresh" )) { -setup_work_tree (); - has_errors |=refresh_cache (REFRESH_REALLY | refresh_flags); -continue ; - } -if (!strcmp (path,"--cacheinfo" )) { -unsigned char sha1[20 ]; -unsigned int mode; - -if (i+3 >= argc) -die ("git update-index: --cacheinfo <mode> <sha1> <path>" ); - -if (strtoul_ui (argv[i+1 ],8 , &mode) || -get_sha1_hex (argv[i+2 ], sha1) || -add_cacheinfo (mode, sha1, argv[i+3 ],0 )) -die ("git update-index: --cacheinfo" -" cannot add %s" , argv[i+3 ]); - i +=3 ; -continue ; - } -if (!strcmp (path,"--chmod=-x" ) || - !strcmp (path,"--chmod=+x" )) { -if (argc <= i+1 ) -die ("git update-index: %s <path>" , path); - set_executable_bit = path[8 ]; -continue ; - } -if (!strcmp (path,"--assume-unchanged" )) { - mark_valid_only =MARK_FLAG ; -continue ; - } -if (!strcmp (path,"--no-assume-unchanged" )) { - mark_valid_only =UNMARK_FLAG ; -continue ; - } -if (!strcmp (path,"--no-skip-worktree" )) { - mark_skip_worktree_only =UNMARK_FLAG ; -continue ; - } -if (!strcmp (path,"--skip-worktree" )) { - mark_skip_worktree_only =MARK_FLAG ; -continue ; - } -if (!strcmp (path,"--info-only" )) { - info_only =1 ; -continue ; - } -if (!strcmp (path,"--force-remove" )) { - force_remove =1 ; -continue ; - } -if (!strcmp (path,"-z" )) { - line_termination =0 ; -continue ; - } -if (!strcmp (path,"--stdin" )) { -if (i != argc -1 ) -die ("--stdin must be at the end" ); - read_from_stdin =1 ; -break ; - } -if (!strcmp (path,"--index-info" )) { -if (i != argc -1 ) -die ("--index-info must be at the end" ); - allow_add = allow_replace = allow_remove =1 ; -read_index_info (line_termination); -break ; - } -if (!strcmp (path,"--unresolve" )) { - has_errors =do_unresolve (argc - i, argv + i, - prefix, prefix_length); -if (has_errors) - active_cache_changed =0 ; - goto finish; - } -if (!strcmp (path,"--again" ) || !strcmp (path,"-g" )) { -setup_work_tree (); - has_errors =do_reupdate (argc - i, argv + i, - prefix, prefix_length); -if (has_errors) - active_cache_changed =0 ; - goto finish; - } -if (!strcmp (path,"--ignore-missing" )) { - refresh_flags |=REFRESH_IGNORE_MISSING ; -continue ; - } -if (!strcmp (path,"--verbose" )) { - verbose =1 ; -continue ; - } -if (!strcmp (path,"--clear-resolve-undo" )) { -resolve_undo_clear (); -continue ; - } -if (!strcmp (path,"-h" ) || !strcmp (path,"--help" )) -usage (update_index_usage); -die ("unknown option %s" , path); +trace_printf ("trace: update-index %s\n" , path); +setup_work_tree (); + p =prefix_path (prefix, prefix_length, path); +update_one (p,NULL ,0 ); +if (set_executable_bit) +chmod_path (set_executable_bit, p); +if (p < path || p > path +strlen (path)) +free ((char *)p); + ctx.argc--; + ctx.argv++; +break ; + } +case PARSE_OPT_UNKNOWN : +if (ctx.argv[0 ][1 ] =='-' ) +error ("unknown option '%s'" , ctx.argv[0 ] +2 ); +else +error ("unknown switch '%c'" , *ctx.opt); +usage_with_options (update_index_usage, options); } -setup_work_tree (); - p =prefix_path (prefix, prefix_length, path); -update_one (p,NULL ,0 ); -if (set_executable_bit) -chmod_path (set_executable_bit, p); -if (p < path || p > path +strlen (path)) -free ((char *)p); } + argc =parse_options_end (&ctx); +if (read_from_stdin) {struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ;
@@ -770,10 +862,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)strbuf_release (&buf); } - finish:if (active_cache_changed) {if (newfd <0 ) { -if (refresh_flags &REFRESH_QUIET ) +if (refresh_args.flags &REFRESH_QUIET )exit (128 );unable_to_lock_index_die (get_index_file (), lock_error); }
Signature · 1 line
1.7.2.3
Re: [PATCH 4/4] update-index: migrate to parse-options API
Quoted 4 lines · Jonathan Nieder
--- builtin/update-index.c | 389 +++++++++++++++++++++++++++++------------------ 1 files changed, 240 insertions(+), 149 deletions(-)
I would suspect that the code would get smaller. Too many callbacks?
builtin/update-index.c static void read_index_info(int line_termination) · line 404
Quoted 7 lines
builtin/update-index.c +4
-2-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?
builtin/update-index.c static int do_reupdate(int ac, const char **av, · line 629
Quoted 7 lines
builtin/update-index.c +7
-0+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.
builtin/update-index.c static int do_reupdate(int ac, const char **av, · line 631
Quoted 2 lines
builtin/update-index.c +2
-0+if (ctx->opt) +return error ("--cacheinfo does not accept an attached argument" );
And this too.
builtin/update-index.c static int do_reupdate(int ac, const char **av, · line 637
Quoted 6 lines
builtin/update-index.c +6
-0+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.
Quoted 13 lines
builtin/update-index.c +13
-0+ 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 ;
builtin/update-index.c static int do_reupdate(int ac, const char **av, · line 660
Quoted 10 lines
builtin/update-index.c +10
-0+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.
builtin/update-index.c static int do_reupdate(int ac, const char **av, · line 665
Quoted 5 lines
builtin/update-index.c +5
-0+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?
builtin/update-index.c static int do_reupdate(int ac, const char **av, · line 685
Quoted 5 lines
builtin/update-index.c +5
-0+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.
builtin/update-index.c static int do_reupdate(int ac, const char **av, · line 690
Quoted 5 lines
builtin/update-index.c +5
-0+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.
Quoted 4 lines
builtin/update-index.c +4
-0+struct option options[] = { +OPT_BIT ('q' ,NULL , &refresh_args.flags, +"continue refresh even when index needs update" , +REFRESH_QUIET ),
[snip]
builtin/update-index.c static int do_reupdate(int ac, const char **av, · line 784
Quoted 7 lines
builtin/update-index.c +7
-0+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.
builtin/update-index.c int cmd_update_index(int argc, const char **argv, const char *prefix) · line 820
Quoted 4 lines
builtin/update-index.c +1
-3-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?
Re: [PATCH 4/4] update-index: migrate to parse-options API
Hi,
Quoted 6 lines · Stephen Boyd
Quoted 2 lines · Jonathan Nieder
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.
Quoted 6 lines
builtin/update-index.c static void read_index_info(int line_termination) · line 404
Quoted 3 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>...]",
builtin/update-index.c +2
-0+ 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. :)
Quoted 2 lines
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.
[...]
Quoted 10 lines
builtin/update-index.c static int do_reupdate(int ac, const char **av, · line 660
Quoted 6 lines
builtin/update-index.c +6
-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.
Agreed but I'm not doing it now, since I don't want to encourage other commands. Will reconsider.
Quoted 9 lines
builtin/update-index.c static int do_reupdate(int ac, const char **av, · line 685
Quoted 5 lines
builtin/update-index.c +5
-0+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_infoover 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.
Quoted 7 lines
builtin/update-index.c static int do_reupdate(int ac, const char **av, · line 690
Quoted 5 lines
builtin/update-index.c +5
-0+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.
Quoted 5 lines
Quoted 4 lines
builtin/update-index.c +4
-0+struct option options[] = { +OPT_BIT ('q' ,NULL , &refresh_args.flags, +"continue refresh even when index needs update" , +REFRESH_QUIET ),[snip]
Quoted 9 lines
builtin/update-index.c static int do_reupdate(int ac, const char **av, · line 784
Quoted 7 lines
builtin/update-index.c +7
-0+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_INToverOPT_BOOLEAN? Just curious.
OPT_BOOLEAN means OPT_INCR and there is only one level of verbosity here.
Quoted 1 line
Quoted 1 line
builtin/update-index.c +1
-0+trace_printf ("trace: update-index %s\n" , path);
[...]
Quoted 1 line
Debugging stuff?
Yes, good catch.
Thanks again for all the comments, and sorry to take so long to get back to this.
Regards,
Jonathan
Re: [RFC/PATCH 0/4] update-index: migrate to parse-options API
In reply to Jonathan NiederQuoted 8 lines · Jonathan Nieder
Quoted 3 lines · Jonathan Nieder
Here's a helper to make it easier for commands that use parse-options to adopt that nicer behavior. It writes its output to stdout, so it should only be used to be used to handle the -h option.
Alas, "git update-index" does not use parse-options yet. But that is easily enough changed...
I don't know parse-options well enough to give comments. If I did, I would have converted update-index long ago (hit it a few times and always ran away). Thanks for the conversion.
Signature · 1 line
Duy
Re: [RFC/PATCH 0/4] update-index: migrate to parse-options API
In reply to Jonathan NiederQuoted 6 lines · Jonathan Nieder
Quoted 3 lines · Jonathan Nieder
Here's a helper to make it easier for commands that use parse-options to adopt that nicer behavior. It writes its output to stdout, so it should only be used to be used to handle the -h option.
Quoted 7 lines
Alas, "git update-index" does not use parse-options yet. But that is easily enough changed...
Patch 1 introduces an
OPTION_LOWLEVEL_CALLBACKbackdoor to parse-options, so new option types (like the three-argument type used by update-index --cacheinfo) can be supported without tempting inventors of other commands through mention in the public API.
I like the idea of this one. I imagine it can also be used to finally whip the command line parsing of "log -L" series Thomas mentored into a reasonable shape, which needs to parse the -L option that takes <range> and an optional <path> (not <pathspec>) as arguments.
cover update-index: migrate to parse-options API
In reply to Jonathan NiederHere's a reroll. Changes explained in the comments following each patch.
Thanks to Stephen for a great deal of helpful advice. The result turns out to be less surgery than I thought from the version in pu, mostly because I am trying to be conservative (meaning further simplifications are welcome as patches on top, natch).
Based against ca20906, as before.
Jonathan Nieder (4):
parse-options: sanity check PARSE_OPT_NOARG flag
parse-options: allow git commands to invent new option types
parse-options: make resuming easier after
PARSE_OPT_STOP_AT_NON_OPTION
update-index: migrate to parse-options API
Nguyễn Thái Ngọc Duy (1):
setup: save prefix (original cwd relative to toplevel) in
startup_info
Stephen Boyd (1):
parse-options: eliminate implicit PARSE_OPT_NOARG for built-in option
types
builtin/update-index.c | 391 ++++++++++++++++++++++++++++++------------------ cache.h | 1 + parse-options.c | 47 ++++--- parse-options.h | 9 +- setup.c | 4 +- 5 files changed, 283 insertions(+), 169 deletions(-)
PATCH 1/6 parse-options: sanity check PARSE_OPT_NOARG flag
Some option types cannot use an argument --- boolean options that would set a bit or flag or increment a counter, for example. If configured in the flag word to accept an argument anyway, the result is an argument that is advertised in "program -h" output only to be rejected by parse-options::get_value.
Luckily all current users of these option types use PARSE_OPT_NOARG and do not use PARSE_OPT_OPTARG. Add a check to ensure that that remains true.
---
New patch, preparing for patch 2. Maybe the check should be implemented elsewhere (ideally compile time) so all flags can be checked rather than just the flags that happen to be used in a given test run.
parse-options.c | 22 ++++++++++++++++++++++ 1 files changed, 22 insertions(+), 0 deletions(-)
parse-options.c +22 -0
@@ -44,6 +44,26 @@ static void fix_filename(const char *prefix, const char **file) *file =xstrdup (prefix_filename (prefix,strlen (prefix), *file)); } +static void check_flags (const struct option *opt) +{ +switch (opt->type) { +case OPTION_BOOLEAN : +case OPTION_BIT : +case OPTION_NEGBIT : +case OPTION_SET_INT : +case OPTION_SET_PTR : +case OPTION_NUMBER : +break ; +default :/* (usually accepts an argument) */ +return ; + } +if ((opt->flags & (PARSE_OPT_OPTARG |PARSE_OPT_NOARG )) ==PARSE_OPT_NOARG ) +return ; +die ("BUG: option '-%c%s' should not accept an argument" , + !opt->short_name ?'-' : opt->short_name, + !opt->short_name ? opt->long_name :"" ); +} +static int get_value (struct parse_opt_ctx_t *p,const struct option *opt,int flags) {
@@ -51,6 +71,8 @@ static int get_value(struct parse_opt_ctx_t *p,const int unset = flags &OPT_UNSET ;int err; +check_flags (opt); +if (unset && p->opt)return opterror (opt,"takes no value" , flags);if (unset && (opt->flags &PARSE_OPT_NONEG ))
Signature · 1 line
1.7.2.3
Re: [PATCH 1/6] parse-options: sanity check PARSE_OPT_NOARG flag
Quoted 10 lines · Jonathan Nieder
(quoted diff) +10
-0+static void check_flags(const struct option *opt) +{ + switch (opt->type) { + case OPTION_BOOLEAN: + case OPTION_BIT: + case OPTION_NEGBIT: + case OPTION_SET_INT: + case OPTION_SET_PTR: + case OPTION_NUMBER: + break;
Review
Quoted 10 lines
(quoted diff) +10
-0+ default: /* (usually accepts an argument) */ + return; + } + if ((opt->flags & (PARSE_OPT_OPTARG | PARSE_OPT_NOARG)) == PARSE_OPT_NOARG) + return; + die("BUG: option '-%c%s' should not accept an argument", + !opt->short_name ? '-' : opt->short_name, + !opt->short_name ? opt->long_name : ""); +} +
This check should probably go into parse_options_check() and be run once for each invocation of parse_options_start()... Oh that isn't good.
Looks like parse_options_check() is being called for each parse_options_step(). Here's a patch to fix that. Junio, this can probably be applied to maint.
---8<------>8-----
Subject: [PATCH] parse-options: Don't call parse_options_check() so much
parse_options_check() is being called for each invocation of parse_options_step() which can be quite a bit for some commands. The commit introducing this function cb9d398 (parse-options: add parse_options_check to validate option specs., 2009-06-09) had the correct motivation and explicitly states that parse_options_check() should be called from parse_options_start(). However, the implementation differs from the motivation. Fix it.
--- builtin/blame.c | 2 +- builtin/shortlog.c | 2 +- parse-options.c | 7 +++---- parse-options.h | 2 +- 4 files changed, 6 insertions(+), 7 deletions(-)
builtin/blame.c +0 -0
@@ -2310,7 +2310,7 @@ int cmd_blame(int argc, const char **argv, const
char *prefix)
dashdash_pos = 0;
parse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |
- PARSE_OPT_KEEP_ARGV0);
+ PARSE_OPT_KEEP_ARGV0, options);
for (;;) {
switch (parse_options_step(&ctx, options, blame_opt_usage)) { case PARSE_OPT_HELP:
builtin/shortlog.c +0 -0
@@ -269,7 +269,7 @@ int cmd_shortlog(int argc, const char **argv, const
char *prefix)
shortlog_init(&log);
init_revisions(&rev, prefix);
parse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |
- PARSE_OPT_KEEP_ARGV0);
+ PARSE_OPT_KEEP_ARGV0, options);
for (;;) {
switch (parse_options_step(&ctx, options, shortlog_usage)) {
parse-options.c +0 -0
@@ -338,7 +338,7 @@ static void parse_options_check(const struct option
*opts)
void parse_options_start(struct parse_opt_ctx_t *ctx,
int argc, const char **argv, const char *prefix,
- int flags)
+ int flags, const struct option *options)
{
memset(ctx, 0, sizeof(*ctx));
ctx->argc = argc - 1;
@@ -350,6 +350,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
(flags & PARSE_OPT_STOP_AT_NON_OPTION))
die("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
+ parse_options_check(options);
}
static int usage_with_options_internal(struct parse_opt_ctx_t *,
@@ -362,8 +363,6 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
{
int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);
- parse_options_check(options);
-
/* we must reset ->opt, unknown short option leave it dangling */ ctx->opt = NULL;
@@ -452,7 +451,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
{
struct parse_opt_ctx_t ctx;
- parse_options_start(&ctx, argc, argv, prefix, flags);
+ parse_options_start(&ctx, argc, argv, prefix, flags, options);
switch (parse_options_step(&ctx, options, usagestr)) { case PARSE_OPT_HELP:
exit(129);
parse-options.h +1 -1
@@ -180,7 +180,7 @@ struct parse_opt_ctx_t {extern void parse_options_start (struct parse_opt_ctx_t *ctx,int argc,const char **argv,const char *prefix, -int flags); +int flags,const struct option *options); extern int parse_options_step(struct parse_opt_ctx_t *ctx, const struct option *options,
Signature · 1 line
1.7.3.2.614.g03864
Re: [PATCH 1/6] parse-options: sanity check PARSE_OPT_NOARG flag
Quoted 15 lines · Stephen Boyd
Quoted 14 lines · Jonathan Nieder
(quoted diff) +14
-0+static void check_flags(const struct option *opt) +{ + switch (opt->type) { + case OPTION_BOOLEAN: + case OPTION_BIT: + case OPTION_NEGBIT: + case OPTION_SET_INT: + case OPTION_SET_PTR: + case OPTION_NUMBER: + break; + default: /* (usually accepts an argument) */ + return; + } + if ((opt->flags & (PARSE_OPT_OPTARG | PARSE_OPT_NOARG)) == PARSE_OPT_NOARG)
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ return; + die("BUG: option '-%c%s' should not accept an argument", + !opt->short_name ? '-' : opt->short_name, + !opt->short_name ? opt->long_name : ""); +} +This check should probably go into
parse_options_check()...
Very good suggestion---that way we can check and get diagnosis for all the errors, not just dying on the first one.
Re: [PATCH 1/6] parse-options: sanity check PARSE_OPT_NOARG flag
In reply to Stephen BoydQuoted 3 lines · Stephen Boyd
Looks like
parse_options_check()is being called for eachparse_options_step(). Here's a patch to fix that. Junio, this can probably be applied to maint.
Looks correct, but why is it a maint material?
Quoted 103 lines
---8<------>8-----
Subject: [PATCH] parse-options: Don't callparse_options_check()so much
parse_options_check()is being called for each invocation ofparse_options_step()which can be quite a bit for some commands. The commit introducing this function cb9d398 (parse-options: addparse_options_checkto validate option specs., 2009-06-09) had the correct motivation and explicitly states thatparse_options_check()should be called fromparse_options_start(). However, the implementation differs from the motivation. Fix it.Signed-off-by Stephen Boyd <[email protected]>--- builtin/blame.c | 2 +- builtin/shortlog.c | 2 +- parse-options.c | 7 +++---- parse-options.h | 2 +- 4 files changed, 6 insertions(+), 7 deletions(-)builtin/blame.c +1
-1@@ -2310,7 +2310,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix) dashdash_pos =0 ;parse_options_start (&ctx, argc, argv, prefix,PARSE_OPT_KEEP_DASHDASH | -PARSE_OPT_KEEP_ARGV0 ); +PARSE_OPT_KEEP_ARGV0 , options);for (;;) {switch (parse_options_step (&ctx, options, blame_opt_usage)) {builtin/blame.c +0
-0case PARSE_OPT_HELP :builtin/shortlog.c +1
-1@@ -269,7 +269,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix) shortlog_init(&log);init_revisions (&rev, prefix);parse_options_start (&ctx, argc, argv, prefix,PARSE_OPT_KEEP_DASHDASH | -PARSE_OPT_KEEP_ARGV0 ); +PARSE_OPT_KEEP_ARGV0 , options);for (;;) {builtin/shortlog.c +0
-0switch (parse_options_step (&ctx, options, shortlog_usage)) {parse-options.c +1
-1@@ -338,7 +338,7 @@ static void parse_options_check(const struct option *opts)void parse_options_start (struct parse_opt_ctx_t *ctx,int argc,const char **argv,const char *prefix, -int flags) +int flags,const struct option *options) {memset (ctx,0 ,sizeof (*ctx));parse-options.c +2
-3ctx->argc = argc -1 ;@@ -350,6 +350,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,if ((flags &PARSE_OPT_KEEP_UNKNOWN ) && (flags &PARSE_OPT_STOP_AT_NON_OPTION ))die ("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together" ); +parse_options_check (options); } static int usage_with_options_internal(struct parse_opt_ctx_t *,@@ -362,8 +363,6 @@ int parse_options_step(struct parse_opt_ctx_t *ctx, {int internal_help = !(ctx->flags &PARSE_OPT_NO_INTERNAL_HELP ); -parse_options_check (options); -/* we must reset ->opt, unknown short option leave it dangling */ ctx->opt =NULL ;@@ -452,7 +451,7 @@ int parse_options(int argc, const char **argv, const char *prefix, {struct parse_opt_ctx_t ctx; -parse_options_start (&ctx, argc, argv, prefix, flags); +parse_options_start (&ctx, argc, argv, prefix, flags, options);switch (parse_options_step (&ctx, options, usagestr)) {case PARSE_OPT_HELP :parse-options.c +0
-0exit (129 );parse-options.h +1
-1@@ -180,7 +180,7 @@ struct parse_opt_ctx_t {extern void parse_options_start (struct parse_opt_ctx_t *ctx,int argc,const char **argv,const char *prefix, -int flags); +int flags,const struct option *options); extern int parse_options_step(struct parse_opt_ctx_t *ctx, const struct option *options,Signature · 1 line
1.7.3.2.614.g03864
Re: [PATCH 1/6] parse-options: sanity check PARSE_OPT_NOARG flag
Quoted 8 lines · Junio C Hamano
Quoted 3 lines · Stephen Boyd
Looks like
parse_options_check()is being called for eachparse_options_step(). Here's a patch to fix that. Junio, this can probably be applied to maint.Looks correct, but why is it a maint material?
Possible performance regression? Which is why its qualified with probably.
PATCH 2/6 parse-options: do not infer PARSE_OPT_NOARG from option type
In reply to Jonathan NiederFrom: Stephen Boyd <[email protected]>
Simplify the "takes no value" error path by relying on PARSE_OPT_NOARG being set correctly. That is:
- if the PARSE_OPT_NOARG flag is set, reject --opt=value regardless of the option type;
- if the PARSE_OPT_NOARG flag is unset, accept --opt=value regardless of the option type.
This way, the accepted usage more closely matches the usage advertised with --help-all.
No functional change intended, since the NOARG flag is only used with "boolean-only" option types in existing parse_options callers.
--- parse-options.c | 19 ++----------------- 1 files changed, 2 insertions(+), 17 deletions(-)
parse-options.c +2 -17
@@ -77,23 +77,8 @@ static int get_value(struct parse_opt_ctx_t *p,return opterror (opt,"takes no value" , flags);if (unset && (opt->flags &PARSE_OPT_NONEG ))return opterror (opt,"isn't available" , flags); - -if (!(flags &OPT_SHORT ) && p->opt) { -switch (opt->type) { -case OPTION_CALLBACK : -if (!(opt->flags &PARSE_OPT_NOARG )) -break ; -/* FALLTHROUGH */ -case OPTION_BOOLEAN : -case OPTION_BIT : -case OPTION_NEGBIT : -case OPTION_SET_INT : -case OPTION_SET_PTR : -return opterror (opt,"takes no value" , flags); -default : -break ; - } - } +if (!(flags &OPT_SHORT ) && p->opt && (opt->flags &PARSE_OPT_NOARG )) +return opterror (opt,"takes no value" , flags);switch (opt->type) {case OPTION_BIT :
Signature · 1 line
1.7.2.3
Re: [PATCH 2/6] parse-options: do not infer PARSE_OPT_NOARG from option type
Quoted 10 lines · Jonathan Nieder
From: Stephen Boyd <[email protected]>
Simplify the "takes no value" error path by relying on
PARSE_OPT_NOARGbeing set correctly. That is:- if the
PARSE_OPT_NOARGflag is set, reject --opt=value regardless of the option type;
- if thePARSE_OPT_NOARGflag is unset, accept --opt=value regardless of the option type.
Quoted 6 lines
This way, the accepted usage more closely matches the usage advertised with --help-all.
No functional change intended, since the NOARG flag is only used with "boolean-only" option types in existing
parse_optionscallers.
Quoted 2 lines
Signed-off-by Jonathan Nieder <[email protected]>---
Thanks for the nice words.
PATCH 3/6 parse-options: allow git commands to invent new option types
In reply to Jonathan Niederparse-options provides a variety of option behaviors, including OPTION_CALLBACK, which should take care of just about any sane behavior. All supported behaviors obey the following constraint:
A --foo option can only accept (and base its behavior on) one argument, which would be the following command-line argument in the "unsticked" form.
Alas, some existing git commands have options that do not obey that constraint. For example, update-index --cacheinfo takes three arguments, and update-index --resolve takes all later parameters as arguments.
Introduces an OPTION_LOWLEVEL_CALLBACK backdoor to parse-options so such option types can be supported without tempting inventors of other commands through mention in the public API. Commands can set the callback field to a function accepting three arguments: the option parsing context, the option itself, and a flag indicating whether the the option was negated. When the option is encountered, that function is called to take over from get_value(). The return value should be zero for success, -1 for usage errors.
Thanks to Stephen Boyd for API guidance.
---
Changes from v1:
. lowlevel callbacks inherit standard get_value argument
checking
. lowlevel callbacks cannot tailor error messages based on
long vs short option
. OPT_UNSET and OPT_SHORT therefore do not need to be exposed . brief mention in parse-options.h comments.
We can always tweak the API again later. None of the examples in update-index even pay attention to the "unset" bit.
parse-options.c | 3 +++ parse-options.h | 8 +++++++- 2 files changed, 10 insertions(+), 1 deletions(-)
parse-options.c +3 -0
@@ -81,6 +81,9 @@ static int get_value(struct parse_opt_ctx_t *p,return opterror (opt,"takes no value" , flags);switch (opt->type) { +case OPTION_LOWLEVEL_CALLBACK : +return (*(parse_opt_ll_cb *)opt->callback)(p, opt, unset); +case OPTION_BIT :if (unset) *(int *)opt->value &= ~opt->defval;
parse-options.h +7 -1
@@ -17,6 +17,7 @@ enum parse_opt_type {OPTION_STRING ,OPTION_INTEGER ,OPTION_CALLBACK , +OPTION_LOWLEVEL_CALLBACK , OPTION_FILENAME };
@@ -43,6 +44,10 @@ enum parse_opt_option_flags {struct option ;typedef int parse_opt_cb (const struct option *,const char *arg,int unset); +struct parse_opt_ctx_t ; +typedef int parse_opt_ll_cb (struct parse_opt_ctx_t *ctx, +const struct option *opt,int unset); +/* * `type`:: * holds the type of the option, you must have an OPTION_END last in your
@@ -87,7 +92,8 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset);* useful for users of OPTION_NEGBIT. * * `callback`:: -* pointer to the callback to use for OPTION_CALLBACK. +* pointer to the callback to use for OPTION_CALLBACK or +* OPTION_LOWLEVEL_CALLBACK. * * `defval`:: * default value to fill (*->value) with for PARSE_OPT_OPTARG.
Signature · 1 line
1.7.2.3
PATCH 4/6 parse-options: make resuming easier after PARSE_OPT_STOP_AT_NON_OPTION
In reply to Jonathan NiederIntroduce a PARSE_OPT_NON_OPTION state, so parse_option_step() callers can easily distinguish between non-options and other reasons for option parsing termination (like "--").
---
As before.
parse-options.c | 3 ++- parse-options.h | 1 + 2 files changed, 3 insertions(+), 1 deletions(-)
parse-options.c +2 -1
@@ -384,7 +384,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,if (parse_nodash_opt (ctx, arg, options) ==0 )continue ;if (ctx->flags &PARSE_OPT_STOP_AT_NON_OPTION ) -break ; +return PARSE_OPT_NON_OPTION ; ctx->out[ctx->cpidx++] = ctx->argv[0 ];continue ; }
@@ -466,6 +466,7 @@ int parse_options(int argc, const char **argv, const char *prefix,switch (parse_options_step (&ctx, options, usagestr)) {case PARSE_OPT_HELP :exit (129 ); +case PARSE_OPT_NON_OPTION :case PARSE_OPT_DONE :break ;default :/* PARSE_OPT_UNKNOWN */
parse-options.h +1 -0
@@ -167,6 +167,7 @@ extern NORETURN void usage_msg_opt(const char *msg,enum {PARSE_OPT_HELP =-1 ,PARSE_OPT_DONE , +PARSE_OPT_NON_OPTION ,PARSE_OPT_UNKNOWN };
Signature · 1 line
1.7.2.3
PATCH 5/6 setup: save prefix (original cwd relative to toplevel) in startup_info
In reply to Jonathan NiederFrom: Nguyễn Thái Ngọc Duy <[email protected]>
Save the path from the original cwd to the cwd at the end of the setup procedure in the startup_info struct introduced in e37c1329 (2010-08-05). The value cannot vary from thread to thread anyway, since the cwd is global.
So now in your builtin command, instead of passing prefix around, when you want to convert a user-supplied path to a cwd-relative path, you can use startup_info->prefix directly.
Caveat: As with the return value from setup_git_directory_gently(), startup_info->prefix would be NULL when the original cwd is not a subdir of the toplevel.
Longer term, this would allow the prefix to be reused when several noncooperating functions require access to the same repository (for example, when accessing configuration before running a builtin).
---
Typo in v1 log message fixed (s/wiould/would/).
cache.h | 1 + setup.c | 4 +++- 2 files changed, 4 insertions(+), 1 deletions(-)
cache.h +1 -0
@@ -1117,6 +1117,7 @@ const char *split_cmdline_strerror(int cmdline_errno);/* git.c */ struct startup_info {int have_repository; +const char *prefix; };extern struct startup_info *startup_info;
setup.c +3 -1
@@ -512,8 +512,10 @@ const char *setup_git_directory_gently(int *nongit_ok)const char *prefix; prefix =setup_git_directory_gently_1 (nongit_ok); -if (startup_info) +if (startup_info) { startup_info->have_repository = !nongit_ok || !*nongit_ok; + startup_info->prefix = prefix; + }return prefix; }
Signature · 1 line
1.7.2.3
PATCH 6/6 update-index: migrate to parse-options API
In reply to Jonathan Nieder--refresh and --really-refresh accept flags (like -q) and modify an error indicator. It might make sense to make the error indicator global, but just pass the flags and a pointer to the error indicator in a struct instead.
--cacheinfo wants 3 arguments. Use the OPTION_LOWLEVEL_CALLBACK extension to grab them.
--assume-unchanged and --no-assume-unchanged probably should use the OPT_UYN feature; but use a callback for now so the existing MARK_FLAG and UNMARK_FLAG values can be used.
--stdin and --index-info are still constrained to be the last argument (implemented using the OPTION_LOWLEVEL_CALLBACK extension).
--unresolve and --again consume all arguments that come after them (also using OPTION_LOWLEVEL_CALLBACK).
The order of options matters. Each path on the command line is affected only by the options that come before it. A custom argument-parsing loop with parse_options_step() brings that about.
In exchange for all the fuss, we get the usual perks: support for un-sticked options, better usage error messages, more useful -h output, and argument parsing code that should be easier to tweak in the future.
---
Lots of clarity improvements:
- simpler usage message
- callback names end in _callback instead of _cb
- redundant argument checking is gone
- some strategic use of vertical whitespace
- error messages more consistent with opterror() output
- wording tweaks in -h output
- removed debug noise
That's it. Thanks to Stephen for many useful comments that made this possible.
Good night.
Jonathan
builtin/update-index.c | 391 ++++++++++++++++++++++++++++++------------------ 1 files changed, 242 insertions(+), 149 deletions(-)
builtin/update-index.c +242 -149
@@ -10,6 +10,7 @@#include "builtin.h" #include "refs.h" #include "resolve-undo.h" +#include "parse-options.h" /* * Default to not allowing changes to the list of files. The
@@ -397,8 +398,10 @@ static void read_index_info(int line_termination)strbuf_release (&uq); } -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 [options] [--] [<file>...]" , +NULL +};static unsigned char head_sha1[20 ];static unsigned char merge_head_sha1[20 ];
@@ -578,16 +581,210 @@ static int do_reupdate(int ac, const char **av,return 0 ; } +struct refresh_params { +unsigned int flags; +int *has_errors; +}; + +static int refresh (struct refresh_params *o,unsigned int flag) +{ +setup_work_tree (); + *o->has_errors |=refresh_cache (o->flags | flag); +return 0 ; +} + +static int refresh_callback (const struct option *opt, +const char *arg,int unset) +{ +return refresh (opt->value,0 ); +} + +static int really_refresh_callback (const struct option *opt, +const char *arg,int unset) +{ +return refresh (opt->value,REFRESH_REALLY ); +} + +static int chmod_callback (const struct option *opt, +const char *arg,int unset) +{ +char *flip = opt->value; +if ((arg[0 ] !='-' && arg[0 ] !='+' ) || arg[1 ] !='x' || arg[2 ]) +return error ("option 'chmod' expects \"+x\" or \"-x\"" ); + *flip = arg[0 ]; +return 0 ; +} + +static int resolve_undo_clear_callback (const struct option *opt, +const char *arg,int unset) +{ +resolve_undo_clear (); +return 0 ; +} + +static int cacheinfo_callback (struct parse_opt_ctx_t *ctx, +const struct option *opt,int unset) +{ +unsigned char sha1[20 ]; +unsigned int mode; + +if (ctx->argc <=3 ) +return error ("option 'cacheinfo' expects 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); + ctx->argc -=3 ; +return 0 ; +} + +static int stdin_cacheinfo_callback (struct parse_opt_ctx_t *ctx, +const struct option *opt,int unset) +{ +int *line_termination = opt->value; + +if (ctx->argc !=1 ) +return error ("option '%s' must be the last argument" , opt->long_name); + allow_add = allow_replace = allow_remove =1 ; +read_index_info (*line_termination); +return 0 ; +} + +static int stdin_callback (struct parse_opt_ctx_t *ctx, +const struct option *opt,int unset) +{ +int *read_from_stdin = opt->value; + +if (ctx->argc !=1 ) +return error ("option '%s' must be the last argument" , opt->long_name); + *read_from_stdin =1 ; +return 0 ; +} + +static int unresolve_callback (struct parse_opt_ctx_t *ctx, +const struct option *opt,int flags) +{ +int *has_errors = opt->value; +const char *prefix = startup_info->prefix; + +/* consume remaining arguments. */ + *has_errors =do_unresolve (ctx->argc, ctx->argv, + prefix, prefix ?strlen (prefix) :0 ); +if (*has_errors) + active_cache_changed =0 ; + + ctx->argv += ctx->argc -1 ; + ctx->argc =1 ; +return 0 ; +} + +static int reupdate_callback (struct parse_opt_ctx_t *ctx, +const struct option *opt,int flags) +{ +int *has_errors = opt->value; +const char *prefix = startup_info->prefix; + +/* consume remaining arguments. */ +setup_work_tree (); + *has_errors =do_reupdate (ctx->argc, ctx->argv, + prefix, prefix ?strlen (prefix) :0 ); +if (*has_errors) + active_cache_changed =0 ; + + ctx->argv += ctx->argc -1 ; + ctx->argc =1 ; +return 0 ; +} +int cmd_update_index (int argc,const char **argv,const char *prefix) { -int i, newfd, entries, has_errors =0 , line_termination ='\n' ; -int allow_options =1 ; +int newfd, entries, has_errors =0 , line_termination ='\n' ;int read_from_stdin =0 ;int prefix_length = prefix ?strlen (prefix) :0 ;char set_executable_bit =0 ; -unsigned int refresh_flags =0 ; +struct refresh_params refresh_args = {0 , &has_errors};int lock_error =0 ;struct lock_file *lock_file; +struct parse_opt_ctx_t ctx; +int parseopt_state =PARSE_OPT_UNKNOWN ; +struct option options[] = { +OPT_BIT ('q' ,NULL , &refresh_args.flags, +"continue refresh even when index needs update" , +REFRESH_QUIET ), +OPT_BIT (0 ,"ignore-submodules" , &refresh_args.flags, +"refresh: ignore submodules" , +REFRESH_IGNORE_SUBMODULES ), +OPT_SET_INT (0 ,"add" , &allow_add, +"do not ignore new files" ,1 ), +OPT_SET_INT (0 ,"replace" , &allow_replace, +"let files replace directories and vice-versa" ,1 ), +OPT_SET_INT (0 ,"remove" , &allow_remove, +"notice files missing from worktree" ,1 ), +OPT_BIT (0 ,"unmerged" , &refresh_args.flags, +"refresh even if index contains unmerged entries" , +REFRESH_UNMERGED ), + {OPTION_CALLBACK ,0 ,"refresh" , &refresh_args,NULL , +"refresh stat information" , +PARSE_OPT_NOARG |PARSE_OPT_NONEG , + refresh_callback}, + {OPTION_CALLBACK ,0 ,"really-refresh" , &refresh_args,NULL , +"like --refresh, but ignore assume-unchanged setting" , +PARSE_OPT_NOARG |PARSE_OPT_NONEG , + really_refresh_callback}, + {OPTION_LOWLEVEL_CALLBACK ,0 ,"cacheinfo" ,NULL , +"<mode> <object> <path>" , +"add the specified entry to the index" , +PARSE_OPT_NOARG |PARSE_OPT_NONEG |PARSE_OPT_LITERAL_ARGHELP , + (parse_opt_cb *) cacheinfo_callback}, + {OPTION_CALLBACK ,0 ,"chmod" , &set_executable_bit,"(+/-)x" , +"override the executable bit of the listed files" , +PARSE_OPT_NONEG |PARSE_OPT_LITERAL_ARGHELP , + chmod_callback}, + {OPTION_SET_INT ,0 ,"assume-unchanged" , &mark_valid_only,NULL , +"mark files as \"not changing\"" , +PARSE_OPT_NOARG |PARSE_OPT_NONEG ,NULL ,MARK_FLAG }, + {OPTION_SET_INT ,0 ,"no-assume-unchanged" , &mark_valid_only,NULL , +"clear assumed-unchanged bit" , +PARSE_OPT_NOARG |PARSE_OPT_NONEG ,NULL ,UNMARK_FLAG }, + {OPTION_SET_INT ,0 ,"skip-worktree" , &mark_skip_worktree_only,NULL , +"mark files as \"index-only\"" , +PARSE_OPT_NOARG |PARSE_OPT_NONEG ,NULL ,MARK_FLAG }, + {OPTION_SET_INT ,0 ,"no-skip-worktree" , &mark_skip_worktree_only,NULL , +"clear skip-worktree bit" , +PARSE_OPT_NOARG |PARSE_OPT_NONEG ,NULL ,UNMARK_FLAG }, +OPT_SET_INT (0 ,"info-only" , &info_only, +"add to index only; do not add content to object database" ,1 ), +OPT_SET_INT (0 ,"force-remove" , &force_remove, +"remove named paths even if present in worktree" ,1 ), +OPT_SET_INT ('z' ,NULL , &line_termination, +"with --stdin: input lines are terminated by null bytes" ,'\0' ), + {OPTION_LOWLEVEL_CALLBACK ,0 ,"stdin" , &read_from_stdin,NULL , +"read list of paths to be updated from standard input" , +PARSE_OPT_NONEG |PARSE_OPT_NOARG , + (parse_opt_cb *) stdin_callback}, + {OPTION_LOWLEVEL_CALLBACK ,0 ,"index-info" , &line_termination,NULL , +"add entries from standard input to the index" , +PARSE_OPT_NONEG |PARSE_OPT_NOARG , + (parse_opt_cb *) stdin_cacheinfo_callback}, + {OPTION_LOWLEVEL_CALLBACK ,0 ,"unresolve" , &has_errors,NULL , +"repopulate stages #2 and #3 for the listed paths" , +PARSE_OPT_NONEG |PARSE_OPT_NOARG , + (parse_opt_cb *) unresolve_callback}, + {OPTION_LOWLEVEL_CALLBACK ,'g' ,"again" , &has_errors,NULL , +"only update entries that differ from HEAD" , +PARSE_OPT_NONEG |PARSE_OPT_NOARG , + (parse_opt_cb *) reupdate_callback}, +OPT_BIT (0 ,"ignore-missing" , &refresh_args.flags, +"ignore files missing from worktree" , +REFRESH_IGNORE_MISSING ), +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_callback}, +OPT_END () + };git_config (git_default_config,NULL );
@@ -602,151 +799,48 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)if (entries <0 )die ("cache corrupted" ); -for (i =1 ; i < argc; i++) { -const char *path = argv[i]; -const char *p; +/* +* Custom copy of parse_options() because we want to handle +* filename arguments as they come. +*/ +parse_options_start (&ctx, argc, argv, prefix, +PARSE_OPT_STOP_AT_NON_OPTION ); +while (ctx.argc) { +if (parseopt_state !=PARSE_OPT_DONE ) + parseopt_state =parse_options_step (&ctx, options, + update_index_usage); +if (!ctx.argc) +break ; +switch (parseopt_state) { +case PARSE_OPT_HELP : +exit (129 ); +case PARSE_OPT_NON_OPTION : +case PARSE_OPT_DONE : + { +const char *path = ctx.argv[0 ]; +const char *p; -if (allow_options && *path =='-' ) { -if (!strcmp (path,"--" )) { - allow_options =0 ; -continue ; - } -if (!strcmp (path,"-q" )) { - refresh_flags |=REFRESH_QUIET ; -continue ; - } -if (!strcmp (path,"--ignore-submodules" )) { - refresh_flags |=REFRESH_IGNORE_SUBMODULES ; -continue ; - } -if (!strcmp (path,"--add" )) { - allow_add =1 ; -continue ; - } -if (!strcmp (path,"--replace" )) { - allow_replace =1 ; -continue ; - } -if (!strcmp (path,"--remove" )) { - allow_remove =1 ; -continue ; - } -if (!strcmp (path,"--unmerged" )) { - refresh_flags |=REFRESH_UNMERGED ; -continue ; - } -if (!strcmp (path,"--refresh" )) { -setup_work_tree (); - has_errors |=refresh_cache (refresh_flags); -continue ; - } -if (!strcmp (path,"--really-refresh" )) { -setup_work_tree (); - has_errors |=refresh_cache (REFRESH_REALLY | refresh_flags); -continue ; - } -if (!strcmp (path,"--cacheinfo" )) { -unsigned char sha1[20 ]; -unsigned int mode; - -if (i+3 >= argc) -die ("git update-index: --cacheinfo <mode> <sha1> <path>" ); - -if (strtoul_ui (argv[i+1 ],8 , &mode) || -get_sha1_hex (argv[i+2 ], sha1) || -add_cacheinfo (mode, sha1, argv[i+3 ],0 )) -die ("git update-index: --cacheinfo" -" cannot add %s" , argv[i+3 ]); - i +=3 ; -continue ; - } -if (!strcmp (path,"--chmod=-x" ) || - !strcmp (path,"--chmod=+x" )) { -if (argc <= i+1 ) -die ("git update-index: %s <path>" , path); - set_executable_bit = path[8 ]; -continue ; - } -if (!strcmp (path,"--assume-unchanged" )) { - mark_valid_only =MARK_FLAG ; -continue ; - } -if (!strcmp (path,"--no-assume-unchanged" )) { - mark_valid_only =UNMARK_FLAG ; -continue ; - } -if (!strcmp (path,"--no-skip-worktree" )) { - mark_skip_worktree_only =UNMARK_FLAG ; -continue ; - } -if (!strcmp (path,"--skip-worktree" )) { - mark_skip_worktree_only =MARK_FLAG ; -continue ; - } -if (!strcmp (path,"--info-only" )) { - info_only =1 ; -continue ; - } -if (!strcmp (path,"--force-remove" )) { - force_remove =1 ; -continue ; - } -if (!strcmp (path,"-z" )) { - line_termination =0 ; -continue ; - } -if (!strcmp (path,"--stdin" )) { -if (i != argc -1 ) -die ("--stdin must be at the end" ); - read_from_stdin =1 ; -break ; - } -if (!strcmp (path,"--index-info" )) { -if (i != argc -1 ) -die ("--index-info must be at the end" ); - allow_add = allow_replace = allow_remove =1 ; -read_index_info (line_termination); -break ; - } -if (!strcmp (path,"--unresolve" )) { - has_errors =do_unresolve (argc - i, argv + i, - prefix, prefix_length); -if (has_errors) - active_cache_changed =0 ; - goto finish; - } -if (!strcmp (path,"--again" ) || !strcmp (path,"-g" )) { -setup_work_tree (); - has_errors =do_reupdate (argc - i, argv + i, - prefix, prefix_length); -if (has_errors) - active_cache_changed =0 ; - goto finish; - } -if (!strcmp (path,"--ignore-missing" )) { - refresh_flags |=REFRESH_IGNORE_MISSING ; -continue ; - } -if (!strcmp (path,"--verbose" )) { - verbose =1 ; -continue ; - } -if (!strcmp (path,"--clear-resolve-undo" )) { -resolve_undo_clear (); -continue ; - } -if (!strcmp (path,"-h" ) || !strcmp (path,"--help" )) -usage (update_index_usage); -die ("unknown option %s" , path); +setup_work_tree (); + p =prefix_path (prefix, prefix_length, path); +update_one (p,NULL ,0 ); +if (set_executable_bit) +chmod_path (set_executable_bit, p); +if (p < path || p > path +strlen (path)) +free ((char *)p); + ctx.argc--; + ctx.argv++; +break ; + } +case PARSE_OPT_UNKNOWN : +if (ctx.argv[0 ][1 ] =='-' ) +error ("unknown option '%s'" , ctx.argv[0 ] +2 ); +else +error ("unknown switch '%c'" , *ctx.opt); +usage_with_options (update_index_usage, options); } -setup_work_tree (); - p =prefix_path (prefix, prefix_length, path); -update_one (p,NULL ,0 ); -if (set_executable_bit) -chmod_path (set_executable_bit, p); -if (p < path || p > path +strlen (path)) -free ((char *)p); } + argc =parse_options_end (&ctx); +if (read_from_stdin) {struct strbuf buf =STRBUF_INIT , nbuf =STRBUF_INIT ;
@@ -770,10 +864,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)strbuf_release (&buf); } - finish:if (active_cache_changed) {if (newfd <0 ) { -if (refresh_flags &REFRESH_QUIET ) +if (refresh_args.flags &REFRESH_QUIET )exit (128 );unable_to_lock_index_die (get_index_file (), lock_error); }
Signature · 1 line
1.7.2.3
Re: [PATCH 6/6] update-index: migrate to parse-options API
builtin/update-index.c static int do_reupdate(int ac, const char **av, · line 738
Quoted 5 lines · Jonathan Nieder
builtin/update-index.c +5
-0+ {OPTION_LOWLEVEL_CALLBACK ,0 ,"cacheinfo" ,NULL , +"<mode> <object> <path>" , +"add the specified entry to the index" , +PARSE_OPT_NOARG |PARSE_OPT_NONEG |PARSE_OPT_LITERAL_ARGHELP , + (parse_opt_cb *) cacheinfo_callback},
Doesn't this take arguments and thus shouldn't be marked PARSE_OPT_NOARG? Confused.
builtin/update-index.c int cmd_update_index(int argc, const char **argv, const char *prefix) · line 807
Quoted 13 lines
builtin/update-index.c +6
-3@@ -602,151 +799,48 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)if (entries <0 )die ("cache corrupted" ); -for (i =1 ; i < argc; i++) { -const char *path = argv[i]; -const char *p; +/* +* Custom copy of parse_options() because we want to handle +* filename arguments as they come. +*/ +parse_options_start (&ctx, argc, argv, prefix, +PARSE_OPT_STOP_AT_NON_OPTION );
This will need to take options too, sorry.
PATCH 1/1 parse-options: always show arghelp when LITERAL_ARGHELP is set
The PARSE_OPT_LITERAL_ARGHELP flag allows a program to override the usual "<argument> for mandatory, [argument] for optional" markup in its help message. Extend it by allowing the usual "no text for disallowed" to be overridden, too (for options with PARSE_OPT_NOARG | PARSE_OPT_LITERAL_ARGHELP, which was previously an unsupported combination).
So now a person can impose ugly usage messages like
--refresh [--] <pathspec>... don't add, only refresh the index
but more importantly, update-index can correctly advertise
--cacheinfo <mode> <object> <path>
add the specified entry to the index
without unsetting PARSE_OPT_NOARG and making that '--cacheinfo=<mode>' '<object>' '<path>'.
---
Quoted 9 lines · Stephen Boyd
Review
Quoted 5 lines · Jonathan Nieder
(quoted diff) +5
-0+ {OPTION_LOWLEVEL_CALLBACK, 0, "cacheinfo", NULL, + "<mode> <object> <path>", + "add the specified entry to the index", + PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP, + (parse_opt_cb *) cacheinfo_callback},Doesn't this take arguments and thus shouldn't be marked
PARSE_OPT_NOARG? Confused.
Yes, that deserves a comment.
PARSE_OPT_NOARG | /* disallow sticky --cacheinfo=<mode> form */ PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
Quoted 1 line
Quoted 1 line
(quoted diff) +0
-0@@ -602,151 +799,48 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
[...]
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ parse_options_start(&ctx, argc, argv, prefix, + PARSE_OPT_STOP_AT_NON_OPTION);This will need to take options too, sorry.
That would just be a merge artifact, no? :) Thanks for a pointer.
If all goes well, I'll reroll the series with your patch later today.
parse-options.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
parse-options.c +2 -1
@@ -552,7 +552,8 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,if (opts->type ==OPTION_NUMBER ) pos +=fprintf (outfile,"-NUM" ); -if (!(opts->flags &PARSE_OPT_NOARG )) +if ((opts->flags &PARSE_OPT_LITERAL_ARGHELP ) || + !(opts->flags &PARSE_OPT_NOARG )) pos +=usage_argh (opts, outfile);if (pos <=USAGE_OPTS_WIDTH )
Signature · 1 line
1.7.2.3
PATCH 2/7 checkout-index -h: show usage even in an invalid repository
In reply to Jonathan NiederFrom: Nguyễn Thái Ngọc Duy <[email protected]>
checkout-index loads the index before parsing options. Erroring out is counterproductive at that point if the operator is hunting for a command to recover useful data from the broken repository.
[jn: new commit message, tests]
--- builtin/checkout-index.c | 3 +++ t/t2006-checkout-index-basic.sh | 24 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 0 deletions(-) create mode 100755 t/t2006-checkout-index-basic.sh
builtin/checkout-index.c +3 -0
@@ -241,6 +241,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix) OPT_END() }; +if (argc ==2 && !strcmp (argv[1 ],"-h" )) +usage_with_options (builtin_checkout_index_usage, + builtin_checkout_index_options);git_config (git_default_config,NULL ); state.base_dir ="" ; prefix_length = prefix ?strlen (prefix) :0 ;
t/t2006-checkout-index-basic.sh new file +24 -0
@@ -0,0 +1,24 @@ +#!/bin/sh + +test_description='basic checkout-index tests +' + +. ./test-lib.sh + +test_expect_success 'checkout-index --gobbledegook' ' + test_expect_code 129 git checkout-index --gobbledegook 2>err && + grep "[Uu]sage" err +' + +test_expect_success 'checkout-index -h in broken repository' ' + mkdir broken && + ( + cd broken && + git init && + >.git/index && + test_expect_code 129 git checkout-index -h >usage 2>&1 + ) && + grep "[Uu]sage" broken/usage +' + +test_done
Signature · 1 line
1.7.2.3
PATCH 3/7 commit/status -h: show usage even with broken configuration
In reply to Jonathan NiederFrom: Nguyễn Thái Ngọc Duy <[email protected]>
"git status" and "git commit" read .git/config and .gitmodules before parsing options, but there is no reason to access a repository at all when the caller just wanted to know what arguments are accepted.
[jn: rewrote the log message and added test]
--- builtin/commit.c | 6 ++++++ t/t7508-status.sh | 24 ++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 0 deletions(-)
builtin/commit.c +6 -0
@@ -1070,6 +1070,9 @@ int cmd_status(int argc, const char **argv, const char *prefix) OPT_END(), }; +if (argc ==2 && !strcmp (argv[1 ],"-h" )) +usage_with_options (builtin_status_usage, builtin_status_options); +if (null_termination && status_format ==STATUS_FORMAT_LONG ) status_format =STATUS_FORMAT_PORCELAIN ;
@@ -1255,6 +1258,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)int allow_fast_forward =1 ;struct wt_status s; +if (argc ==2 && !strcmp (argv[1 ],"-h" )) +usage_with_options (builtin_commit_usage, builtin_commit_options); +wt_status_prepare (&s);git_config (git_commit_config, &s); in_merge =file_exists (git_path ("MERGE_HEAD" ));
t/t7508-status.sh +24 -0
@@ -7,6 +7,30 @@ test_description='git status' . ./test-lib.sh +test_expect_success 'status -h in broken repository' ' + mkdir broken && + test_when_finished "rm -fr broken" && + ( + cd broken && + git init && + echo "[status] showuntrackedfiles = CORRUPT" >>.git/config && + test_expect_code 129 git status -h >usage 2>&1 + ) && + grep "[Uu]sage" broken/usage +' + +test_expect_success 'commit -h in broken repository' ' + mkdir broken && + test_when_finished "rm -fr broken" && + ( + cd broken && + git init && + echo "[status] showuntrackedfiles = CORRUPT" >>.git/config && + test_expect_code 129 git commit -h >usage 2>&1 + ) && + grep "[Uu]sage" broken/usage +' + test_expect_success 'setup' ' : >tracked && : >modified &&
Signature · 1 line
1.7.2.3
PATCH 4/7 gc -h: show usage even with broken configuration
In reply to Jonathan NiederFrom: Nguyễn Thái Ngọc Duy <[email protected]>
Given a request for command-line usage information rather than some more substantial action, the only friendly thing to do is to report the usage information as soon as possible and exit.
Without this change, as "git gc" glances over the repository, it can be distracted by the desire to report a malformed configuration file.
Noticed while working through reports from Duy's repository access checker.
[jn: with rewritten log message and tests]
--- builtin/gc.c | 3 +++ t/t6500-gc.sh | 28 ++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 0 deletions(-) create mode 100755 t/t6500-gc.sh
builtin/gc.c +3 -0
@@ -189,6 +189,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix) OPT_END() }; +if (argc ==2 && !strcmp (argv[1 ],"-h" )) +usage_with_options (builtin_gc_usage, builtin_gc_options); +git_config (gc_config,NULL );if (pack_refs <0 )
t/t6500-gc.sh new file +28 -0
@@ -0,0 +1,28 @@ +#!/bin/sh + +test_description='basic git gc tests +' + +. ./test-lib.sh + +test_expect_success 'gc empty repository' ' + git gc +' + +test_expect_success 'gc --gobbledegook' ' + test_expect_code 129 git gc --nonsense 2>err && + grep "[Uu]sage: git gc" err +' + +test_expect_success 'gc -h with invalid configuration' ' + mkdir broken && + ( + cd broken && + git init && + echo "[gc] pruneexpire = CORRUPT" >>.git/config && + test_expect_code 129 git gc -h >usage 2>&1 + ) && + grep "[Uu]sage" broken/usage +' + +test_done
Signature · 1 line
1.7.2.3
PATCH 5/7 ls-files -h: show usage even with corrupt index
In reply to Jonathan NiederFrom: Nguyễn Thái Ngọc Duy <[email protected]>
Part of a campaign to avoid git <command> -h being distracted by access to the repository. A caller hoping to use "git ls-files" with an alternate index as part of a repair operation may well use "git ls-files -h" to show usage while planning it out.
[jn: with rewritten log message and tests]
--- builtin/ls-files.c | 3 +++ t/t3004-ls-files-basic.sh | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 0 deletions(-) create mode 100755 t/t3004-ls-files-basic.sh
builtin/ls-files.c +3 -0
@@ -530,6 +530,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) OPT_END() }; +if (argc ==2 && !strcmp (argv[1 ],"-h" )) +usage_with_options (ls_files_usage, builtin_ls_files_options); +memset (&dir,0 ,sizeof (dir)); prefix = cmd_prefix;if (prefix)
t/t3004-ls-files-basic.sh new file +39 -0
@@ -0,0 +1,39 @@ +#!/bin/sh + +test_description='basic ls-files tests + +This test runs git ls-files with various unusual or malformed +command-line arguments. +' + +. ./test-lib.sh + +>empty + +test_expect_success 'ls-files in empty repository' ' + git ls-files >actual && + test_cmp empty actual +' + +test_expect_success 'ls-files with nonexistent path' ' + git ls-files doesnotexist >actual && + test_cmp empty actual +' + +test_expect_success 'ls-files with nonsense option' ' + test_expect_code 129 git ls-files --nonsense 2>actual && + grep "[Uu]sage: git ls-files" actual +' + +test_expect_success 'ls-files -h in corrupt repository' ' + mkdir broken && + ( + cd broken && + git init && + >.git/index && + test_expect_code 129 git ls-files -h >usage 2>&1 + ) && + grep "[Uu]sage: git ls-files " broken/usage +' + +test_done
Signature · 1 line
1.7.2.3
Re: [PATCH 5/7] ls-files -h: show usage even with corrupt index
t/t3004-ls-files-basic.sh line 21
Quoted 4 lines · Jonathan Nieder
t/t3004-ls-files-basic.sh new file +4
-0+test_expect_success 'ls-files with nonexistent path' ' + git ls-files doesnotexist >actual && + test_cmp empty actual +'
This technically is not "nonexistent path" but is "a pathspec that matches nothing".
PATCH 6/7 merge -h: show usage even with corrupt index
In reply to Jonathan NiederFrom: Nguyễn Thái Ngọc Duy <[email protected]>
Part of a campaign to make sure "git <command> -h" works correctly when run from distractingly bad repositories.
[jn: with rewritten log message and tests]
--- builtin/merge.c | 2 ++ t/t7600-merge.sh | 11 +++++++++++ 2 files changed, 13 insertions(+), 0 deletions(-)
builtin/merge.c +2 -0
@@ -909,6 +909,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)const char *best_strategy =NULL , *wt_strategy =NULL ;struct commit_list **remotes = &remoteheads; +if (argc ==2 && !strcmp (argv[1 ],"-h" )) +usage_with_options (builtin_merge_usage, builtin_merge_options);if (read_cache_unmerged ()) {die_resolve_conflict ("merge" ); }
t/t7600-merge.sh +11 -0
@@ -144,6 +144,17 @@ test_expect_success 'test option parsing' ' test_must_fail git merge ' +test_expect_success 'merge -h with invalid index' ' + mkdir broken && + ( + cd broken && + git init && + >.git/index && + test_expect_code 129 git merge -h 2>usage + ) && + grep "[Uu]sage: git merge" broken/usage +' + test_expect_success 'reject non-strategy with a git-merge-foo name' ' test_must_fail git merge -s index c1 '
Signature · 1 line
1.7.2.3
Re: [PATCH 6/7] merge -h: show usage even with corrupt index
Quoted 1 line · Jonathan Nieder
+test_expect_success 'merge -h with invalid index' '
You say "in broken repository" in 1-3, "with invalid configuration" in 4, "in corrupt repository" in 5, "with invalid index" here, and "with corrupt index" in 7.
t/t7600-merge.sh test_expect_success 'test option parsing' ' · line 152
Quoted 5 lines
t/t7600-merge.sh +5
-0+ mkdir broken && + ( + cd broken && + git init && + >.git/index &&
I think describing which aspect of brokenness the test is interested in on the title line is better; let's restate "in broken/corrupt repository" (1-3, 5) to be more specific, and match others in terminology. E.g.
1. corrupt ref
2. corrupt index
3. corrupt status.showuntrackedfiles config
4. corrupt gc.pruneexpire config
5. corrupt index
6. corrupt index
7. corrupt index
PATCH 7/7 update-index -h: show usage even with corrupt index
In reply to Jonathan NiederFrom: Nguyễn Thái Ngọc Duy <[email protected]>
When trying to fix up a corrupt repository, one might prefer that "update-index -h" print an accurate usage message and exit rather than reading the repository and complaining about the corruption.
[jn: with rewritten log message and tests]
--- builtin/update-index.c | 3 +++ t/t2107-update-index-basic.sh | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 0 deletions(-) create mode 100755 t/t2107-update-index-basic.sh
builtin/update-index.c +3 -0
@@ -589,6 +589,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)int lock_error =0 ;struct lock_file *lock_file; +if (argc ==2 && !strcmp (argv[1 ],"-h" )) +usage (update_index_usage); +git_config (git_default_config,NULL );/* We can't free this memory, it becomes part of a linked list parsed atexit() */
t/t2107-update-index-basic.sh new file +32 -0
@@ -0,0 +1,32 @@ +#!/bin/sh + +test_description='basic update-index tests + +Tests for command-line parsing and basic operation. +' + +. ./test-lib.sh + +test_expect_success 'update-index --nonsense fails' ' + test_must_fail git update-index --nonsense 2>msg && + cat msg && + test -s msg +' + +test_expect_failure 'update-index --nonsense dumps usage' ' + test_expect_code 129 git update-index --nonsense 2>err && + grep "[Uu]sage: git update-index" err +' + +test_expect_success 'update-index -h with corrupt index' ' + mkdir broken && + ( + cd broken && + git init && + >.git/index && + test_expect_code 129 git update-index -h >usage 2>&1 + ) && + grep "[Uu]sage: git update-index" broken/usage +' + +test_done
Signature · 1 line
1.7.2.3
cover avoid repository access during "git <foo> -h"
In reply to Jonathan NiederQuoted 1 line · Jonathan Nieder
[Subject: [PATCH en/and-cascade-tests 0/7] ]
Hmph, teaches me to send a series at night time. The topic is printing usage information early to avoid repository access during "git <foo> -h".
Sorry, I should have prepared and sent that earlier. Hope it helps still.
Re: [PATCH en/and-cascade-tests 0/7]
In reply to Jonathan Nieder2010/10/22 Jonathan Nieder <[email protected]>:
Quoted 8 lines
Nguyễn Thái Ngọc Duy (7):
branch -h: show usage even in an invalid repository checkout-index -h: show usage even in an invalid repository commit/status -h: show usage even with broken configuration gc -h: show usage even with broken configuration ls-files -h: show usage even with corrupt index merge -h: show usage even with corrupt index update-index -h: show usage even with corrupt index
You should take the credit. I bet my code change does not take as much time as your writing tests. Nice tests by the way. Inspiring.
Signature · 1 line
Duy