{"thread":{"id":"25483","subject":"[PATCH v2 1/4] setup_git_env: print the true $GIT_DIR for debugging","startedAt":"2010-10-20T03:11:57Z","lastAt":"2010-12-03T07:35:35Z","messageCount":45,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Nguyen Thai Ngoc Duy","Jonathan Nieder","Stephen Boyd"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"153844","messageId":"1287544320-8499-1-git-send-email-pclouds@gmail.com","threadId":"25483","inReplyTo":null,"subject":"[PATCH v2 1/4] setup_git_env: print the true $GIT_DIR for debugging","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-10-20T03:11:57Z","receivedAt":"2010-10-20T03:11:57Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Ideally, GIT_DIR_ENVIRONMENT should be set before setup_git_env() is\ncalled and \"setup: GIT_DIR defaults to .git\" should never be printed\nout. If it is printed, $GIT_DIR is set up automatically, unexpectedly,\nelsewhere.\n\nChecking for that line is a good way to know if setup code works\nproperly.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Second try. Better tests.\n\n environment.c | 6 +++++-\n 1 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/environment.c b/environment.c\nindex de5581f..e34adc3 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -91,8 +91,12 @@ static void setup_git_env(void)\n \t\tgit_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);\n \t\tgit_dir = git_dir ? xstrdup(git_dir) : NULL;\n \t}\n-\tif (!git_dir)\n+\tif (!git_dir) {\n+\t\ttrace_printf(\"setup: GIT_DIR defaults to .git\\n\");\n \t\tgit_dir = DEFAULT_GIT_DIR_ENVIRONMENT;\n+\t}\n+\telse\n+\t\ttrace_printf(\"setup: GIT_DIR set to %s\\n\", git_dir);\n \tgit_object_dir = getenv(DB_ENVIRONMENT);\n \tif (!git_object_dir) {\n \t\tgit_object_dir = xmalloc(strlen(git_dir) + 9);\n-- \n1.7.0.2.445.gcbdb3\n"},{"id":"153845","messageId":"1287544320-8499-2-git-send-email-pclouds@gmail.com","threadId":"25483","inReplyTo":"1287544320-8499-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 2/4] run_builtin(): save \"-h\" detection result for later use","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-10-20T03:11:58Z","receivedAt":"2010-10-20T03:11:58Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When run_builtin() sees \"-h\" as the first argument, it assumes:\n\n - this is the call for help usage\n - the real git command will only print help usage then exit\n\nSo it skips all setup in this case. Unfortunately, some commands do\nother things before calling parse_options(), which is often where the\nhelp usage is printed. Some of those things may try to access the\nrepository unnecessarily. If a repository is broken, the command may\ndie() before it prints help usage, not really helpful.\n\nMake real commands aware of this fast path so that they can handle it\nproperly (i.e., print help usage then exit immediately) if they were\ngoing to do more initialization than git_config().\n\nDemonstrating \"git foo -h\" fails depends on individual commands and\nis generally difficult to do. Instead GIT_TRACE is used to check\nif a command does set repo. If it does, it is supposed to fail if\nrepo setup code chokes.\n\n\"git upload-archive\" fails for another reason, but will be fixed too\nwhen \"git upload-archive -h\" is converted to use startup_info->help\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n cache.h | 1 +\n git.c | 18 ++++++++++++++----\n t/t3905-help.sh | 24 ++++++++++++++++++++++++\n 3 files changed, 39 insertions(+), 4 deletions(-)\n create mode 100755 t/t3905-help.sh\n\ndiff --git a/cache.h b/cache.h\nindex 33decd9..bb57e34 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1117,6 +1117,7 @@ const char *split_cmdline_strerror(int cmdline_errno);\n /* git.c */\n struct startup_info {\n \tint have_repository;\n+\tint help; /* git foo -h */\n };\n extern struct startup_info *startup_info;\n \ndiff --git a/git.c b/git.c\nindex 50a1401..bb67540 100644\n--- a/git.c\n+++ b/git.c\n@@ -246,13 +246,23 @@ struct cmd_struct {\n \n static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n {\n-\tint status, help;\n+\tint status;\n \tstruct stat st;\n \tconst char *prefix;\n \n \tprefix = NULL;\n-\thelp = argc == 2 && !strcmp(argv[1], \"-h\");\n-\tif (!help) {\n+\tstartup_info->help = argc == 2 && !strcmp(argv[1], \"-h\");\n+\tif (startup_info->help) {\n+\t\t/*\n+\t\t * Fast path for \"git foo -h\", no setup is done.\n+\t\t * Other functions might set .git up automatically\n+\t\t * and potentially die() along the way. It's best\n+\t\t * to check this flag from the beginning, print its\n+\t\t * help usage and exit, nothing more.\n+\t\t */\n+\t\t;\n+\t}\n+\telse {\n \t\tif (p->option & RUN_SETUP)\n \t\t\tprefix = setup_git_directory();\n \t\tif (p->option & RUN_SETUP_GENTLY) {\n@@ -267,7 +277,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \t}\n \tcommit_pager_choice();\n \n-\tif (!help && p->option & NEED_WORK_TREE)\n+\tif (!startup_info->help && p->option & NEED_WORK_TREE)\n \t\tsetup_work_tree();\n \n \ttrace_argv_printf(argv, \"trace: built-in: git\");\ndiff --git a/t/t3905-help.sh b/t/t3905-help.sh\nnew file mode 100755\nindex 0000000..0dcbedf\n--- /dev/null\n+++ b/t/t3905-help.sh\n@@ -0,0 +1,24 @@\n+#!/bin/sh\n+\n+test_description='tests that git foo -h should work even in potentially broken repos'\n+\n+. ./test-lib.sh\n+\n+test_help() {\n+\ttest_expect_\"$1\" \"$2 -h\" \"\n+\t\tGIT_TRACE=\\\"`pwd`\\\"/$2.log test_must_fail git $2 -h &&\n+\t\ttest \\$exit_code = 129 &&\n+\t\t! grep 'defaults to' $2.log\n+\t\"\n+}\n+\n+test_help failure branch\n+test_help failure checkout-index\n+test_help failure commit\n+test_help failure gc\n+test_help failure ls-files\n+test_help failure merge\n+test_help failure update-index\n+test_help failure upload-archive\n+\n+test_done\n-- \n1.7.0.2.445.gcbdb3\n"},{"id":"153846","messageId":"1287544320-8499-3-git-send-email-pclouds@gmail.com","threadId":"25483","inReplyTo":"1287544320-8499-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 3/4] builtins: utilize startup_info->help where possible","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-10-20T03:11:59Z","receivedAt":"2010-10-20T03:11:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"It helps reduce false alarms while I'm looking for \"git foo -h\" code\npath that accesses repository. Anyway it looks like a good thing to\ndo. If one day people like to have \"git foo --help\" as an alternative\nto \"git foo -h\", it would be easy.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/check-ref-format.c | 2 +-\n builtin/grep.c | 2 +-\n builtin/index-pack.c | 2 +-\n builtin/log.c | 6 +-----\n builtin/merge-ours.c | 2 +-\n builtin/pack-redundant.c | 2 +-\n builtin/show-ref.c | 2 +-\n 7 files changed, 7 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex ae3f281..c6511e3 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -58,7 +58,7 @@ static int check_ref_format_print(const char *arg)\n \n int cmd_check_ref_format(int argc, const char **argv, const char *prefix)\n {\n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n+\tif (startup_info->help)\n \t\tusage(builtin_check_ref_format_usage);\n \n \tif (argc == 3 && !strcmp(argv[1], \"--branch\"))\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex da32f3d..ec39909 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -934,7 +934,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)\n \t * 'git grep -h', unlike 'git grep -h ', is a request\n \t * to show usage information and exit.\n \t */\n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n+\tif (startup_info->help)\n \t\tusage_with_options(grep_usage, options);\n \n \tmemset(&opt, 0, sizeof(opt));\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex e243d9d..649ad18 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -881,7 +881,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tstruct pack_idx_entry **idx_objects;\n \tunsigned char pack_sha1[20];\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n+\tif (startup_info->help)\n \t\tusage(index_pack_usage);\n \n \tread_replace_refs = 0;\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 22d1290..a7ba9ed 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -69,11 +69,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \tif (default_date_mode)\n \t\trev->date_mode = parse_date_format(default_date_mode);\n \n-\t/*\n-\t * Check for -h before setup_revisions(), or \"git log -h\" will\n-\t * fail when run without a git directory.\n-\t */\n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n+\tif (startup_info->help)\n \t\tusage(builtin_log_usage);\n \targc = setup_revisions(argc, argv, rev, opt);\n \ndiff --git a/builtin/merge-ours.c b/builtin/merge-ours.c\nindex 6844116..8e0777b 100644\n--- a/builtin/merge-ours.c\n+++ b/builtin/merge-ours.c\n@@ -20,7 +20,7 @@ static const char *diff_index_args[] = {\n \n int cmd_merge_ours(int argc, const char **argv, const char *prefix)\n {\n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n+\tif (startup_info->help)\n \t\tusage(builtin_merge_ours_usage);\n \n \t/*\ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex 41e1615..3f090b2 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -601,7 +601,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)\n \tunsigned char *sha1;\n \tchar buf[42]; /* 40 byte sha1 + \\n + \\0 */\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n+\tif (startup_info->help)\n \t\tusage(pack_redundant_usage);\n \n \tfor (i = 1; i < argc; i++) {\ndiff --git a/builtin/show-ref.c b/builtin/show-ref.c\nindex be9b512..7083fa9 100644\n--- a/builtin/show-ref.c\n+++ b/builtin/show-ref.c\n@@ -204,7 +204,7 @@ static const struct option show_ref_options[] = {\n \n int cmd_show_ref(int argc, const char **argv, const char *prefix)\n {\n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n+\tif (startup_info->help)\n \t\tusage_with_options(show_ref_usage, show_ref_options);\n \n \targc = parse_options(argc, argv, prefix, show_ref_options,\n-- \n1.7.0.2.445.gcbdb3\n"},{"id":"153847","messageId":"1287544320-8499-4-git-send-email-pclouds@gmail.com","threadId":"25483","inReplyTo":"1287544320-8499-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 4/4] builtins: check for startup_info->help, print and exit early","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-10-20T03:12:00Z","receivedAt":"2010-10-20T03:12:00Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"By ensuring no access to repo is done in \"git foo -h\" case, the commands\nhave better chance of really printing out help usage. Access to repo is not\nnecessary and may terminate program if it finds something wrong.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/branch.c | 3 +++\n builtin/checkout-index.c | 3 +++\n builtin/commit.c | 6 ++++++\n builtin/gc.c | 3 +++\n builtin/ls-files.c | 3 +++\n builtin/merge.c | 3 +++\n builtin/update-index.c | 3 +++\n builtin/upload-archive.c | 7 ++++---\n t/t3905-help.sh | 16 ++++++++--------\n 9 files changed, 36 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 87976f0..9f152ed 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -667,6 +667,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tOPT_END(),\n \t};\n \n+\tif (startup_info->help)\n+\t\tusage_with_options(builtin_branch_usage, options);\n+\n \tgit_config(git_branch_config, NULL);\n \n \tif (branch_use_color == -1)\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex a7a5ee1..7f25cd7 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -241,6 +241,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n+\tif (startup_info->help)\n+\t\tusage_with_options(builtin_checkout_index_usage, builtin_checkout_index_options);\n+\n \tgit_config(git_default_config, NULL);\n \tstate.base_dir = \"\";\n \tprefix_length = prefix ? strlen(prefix) : 0;\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 66fdd22..8b086f2 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1070,6 +1070,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \t\tOPT_END(),\n \t};\n \n+\tif (startup_info->help)\n+\t\tusage_with_options(builtin_status_usage, builtin_status_options);\n+\n \tif (null_termination && status_format == STATUS_FORMAT_LONG)\n \t\tstatus_format = STATUS_FORMAT_PORCELAIN;\n \n@@ -1255,6 +1258,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tint allow_fast_forward = 1;\n \tstruct wt_status s;\n \n+\tif (startup_info->help)\n+\t\tusage_with_options(builtin_commit_usage, builtin_commit_options);\n+\n \twt_status_prepare(&s);\n \tgit_config(git_commit_config, &s);\n \tin_merge = file_exists(git_path(\"MERGE_HEAD\"));\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c304638..5a9d0da 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -189,6 +189,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n+\tif (startup_info->help)\n+\t\tusage_with_options(builtin_gc_usage, builtin_gc_options);\n+\n \tgit_config(gc_config, NULL);\n \n \tif (pack_refs < 0)\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex bb4f612..814da51 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -530,6 +530,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\tOPT_END()\n \t};\n \n+\tif (startup_info->help)\n+\t\tusage_with_options(ls_files_usage, builtin_ls_files_options);\n+\n \tmemset(&dir, 0, sizeof(dir));\n \tprefix = cmd_prefix;\n \tif (prefix)\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 2dba3b9..0169694 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -917,6 +917,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n \tstruct commit_list **remotes = &remoteheads;\n \n+\tif (startup_info->help)\n+\t\tusage_with_options(builtin_merge_usage, builtin_merge_options);\n+\n \tif (read_cache_unmerged()) {\n \t\tdie_resolve_conflict(\"merge\");\n \t}\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 3ab214d..46a53f5 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -589,6 +589,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \tint lock_error = 0;\n \tstruct lock_file *lock_file;\n \n+\tif (startup_info->help)\n+\t\tusage(update_index_usage);\n+\n \tgit_config(git_default_config, NULL);\n \n \t/* We can't free this memory, it becomes part of a linked list parsed atexit() */\ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 73f788e..d4f4741 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -26,9 +26,6 @@ static int run_upload_archive(int argc, const char **argv, const char *prefix)\n \tint sent_argc;\n \tint len;\n \n-\tif (argc != 2)\n-\t\tusage(upload_archive_usage);\n-\n \tif (strlen(argv[1]) + 1 > sizeof(buf))\n \t\tdie(\"insanely long repository name\");\n \n@@ -98,6 +95,10 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)\n {\n \tpid_t writer;\n \tint fd1[2], fd2[2];\n+\n+\tif (startup_info->help || argc != 2)\n+\t\tusage(upload_archive_usage);\n+\n \t/*\n \t * Set up sideband subprocess.\n \t *\ndiff --git a/t/t3905-help.sh b/t/t3905-help.sh\nindex 0dcbedf..6cab6b9 100755\n--- a/t/t3905-help.sh\n+++ b/t/t3905-help.sh\n@@ -12,13 +12,13 @@ test_help() {\n \t\"\n }\n \n-test_help failure branch\n-test_help failure checkout-index\n-test_help failure commit\n-test_help failure gc\n-test_help failure ls-files\n-test_help failure merge\n-test_help failure update-index\n-test_help failure upload-archive\n+test_help success branch\n+test_help success checkout-index\n+test_help success commit\n+test_help success gc\n+test_help success ls-files\n+test_help success merge\n+test_help success update-index\n+test_help success upload-archive\n \n test_done\n-- \n1.7.0.2.445.gcbdb3\n"},{"id":"154042","messageId":"7vsjzzrw6d.fsf@alter.siamese.dyndns.org","threadId":"25483","inReplyTo":"1287544320-8499-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 2/4] run_builtin(): save \"-h\" detection result for later use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-21T22:57:30Z","receivedAt":"2010-10-21T22:57:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> When run_builtin() sees \"-h\" as the first argument, it assumes:\n>\n> - this is the call for help usage\n> - the real git command will only print help usage then exit\n>\n> So it skips all setup in this case. Unfortunately, some commands do\n> other things before calling parse_options(), which is often where the\n> help usage is printed. Some of those things may try to access the\n> repository unnecessarily. If a repository is broken, the command may\n> die() before it prints help usage, not really helpful.\n\nWhat does die() message say in that case? If it says \"your repository is\nbroken\", that may be more useful than giving a help message. I dunno.\n\n> Demonstrating \"git foo -h\" fails depends on individual commands and\n> is generally difficult to do. Instead GIT_TRACE is used to check\n> if a command does set repo. If it does, it is supposed to fail if\n> repo setup code chokes.\n\nHmm, I am not sure I understand this one. If you are interested in\nchanging the behaviour of these commands when run with \"-h\" in a corrupt\nrepository, perhaps you can deliberately corrupt the test repository in\nthe trash directory you start with, and run these commands there, no?\nFor a good measure, you could use CEILING_DIRECTORIES to make sure the\ntests do not climb up to the project repository.\n"},{"id":"154087","messageId":"AANLkTikngVJX-GYuu3xv=QnjQEXDcTL15T1n5h6EsPsE@mail.gmail.com","threadId":"25483","inReplyTo":"7vsjzzrw6d.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/4] run_builtin(): save \"-h\" detection result for later use","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-10-22T01:47:31Z","receivedAt":"2010-10-22T01:47:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2010/10/22 Junio C Hamano :\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> When run_builtin() sees \"-h\" as the first argument, it assumes:\n>>\n>>  - this is the call for help usage\n>>  - the real git command will only print help usage then exit\n>>\n>> So it skips all setup in this case.  Unfortunately, some commands do\n>> other things before calling parse_options(), which is often where the\n>> help usage is printed.  Some of those things may try to access the\n>> repository unnecessarily. If a repository is broken, the command may\n>> die() before it prints help usage, not really helpful.\n>\n> What does die() message say in that case?  If it says \"your repository is\n> broken\", that may be more useful than giving a help message.  I dunno.\n\nI should have written \"if repository access is broken\" (i.e. .git has\nnot been found, then somewhere access to .git is requested and .git is\nset up automatically). But I'm chasing a ghost here. And the impact to\n\"-h\" is probably nothing (how can accessing a wrong .git impacts a\nstatic help string?). I'll take the series back. There are more\nimportant things to work on than this.\n--\nDuy\n"},{"id":"154107","messageId":"20101022063837.GA6081@burratino","threadId":"25483","inReplyTo":"1287544320-8499-4-git-send-email-pclouds@gmail.com","subject":"[PATCH en/and-cascade-tests 0/7]","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-22T06:38:37Z","receivedAt":"2010-10-22T06:38:37Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyễn Thái Ngọc Duy wrote:\n\n> builtin/branch.c | 3 +++\n> builtin/checkout-index.c | 3 +++\n> builtin/commit.c | 6 ++++++\n> builtin/gc.c | 3 +++\n> builtin/ls-files.c | 3 +++\n> builtin/merge.c | 3 +++\n> builtin/update-index.c | 3 +++\n> builtin/upload-archive.c | 7 ++++---\n\nOkay, all of them survive a reroll except upload-archive. As for\nupload-archive:\n\n - it already doesn't support \"-h\"\n - it is such low-level plumbing, I don't really mind that. Maybe\n someone is using a repository named \"-h\" (though I hope not, of\n course).\n\nMaybe these patches should be squashed. I separated them because they\nwere easier to write and it would be easier to cc the right people\nthis way.\n\nThe primary motivation for the series is that the repository checker\nfrom the nd/setup series will not be happy without some change like\nthis. In the git -h codepath, repository setup is not run so\nrepository access is forbidden.\n\nThe secondary motivation is to create better behavior in situations of\nrepository corruption. When I type \"git checkout -h\", I am asking for\na usage message, not a repository self-check. Especially when trying\nto repair a repository, commands that do not do what they are asked to\nare generally frustrating to use.\n\nPatches apply on top of \"test-lib: make test_expect_code a test\ncommand\" from the en/and-cascade-tests topic.\n\nEnjoy,\nJonathan\n\nNguyễn Thái Ngọc Duy (7):\n branch -h: show usage even in an invalid repository\n checkout-index -h: show usage even in an invalid repository\n commit/status -h: show usage even with broken configuration\n gc -h: show usage even with broken configuration\n ls-files -h: show usage even with corrupt index\n merge -h: show usage even with corrupt index\n update-index -h: show usage even with corrupt index\n\n builtin/branch.c | 3 +++\n builtin/checkout-index.c | 3 +++\n builtin/commit.c | 6 ++++++\n builtin/gc.c | 3 +++\n builtin/ls-files.c | 3 +++\n builtin/merge.c | 2 ++\n builtin/update-index.c | 3 +++\n t/t2006-checkout-index-basic.sh | 24 ++++++++++++++++++++++++\n t/t2107-update-index-basic.sh | 32 ++++++++++++++++++++++++++++++++\n t/t3004-ls-files-basic.sh | 39 +++++++++++++++++++++++++++++++++++++++\n t/t3200-branch.sh | 11 +++++++++++\n t/t6500-gc.sh | 28 ++++++++++++++++++++++++++++\n t/t7508-status.sh | 24 ++++++++++++++++++++++++\n t/t7600-merge.sh | 11 +++++++++++\n 14 files changed, 192 insertions(+), 0 deletions(-)\n create mode 100755 t/t2006-checkout-index-basic.sh\n create mode 100755 t/t2107-update-index-basic.sh\n create mode 100755 t/t3004-ls-files-basic.sh\n create mode 100755 t/t6500-gc.sh\n\n-- \n1.7.2.3\n"},{"id":"154108","messageId":"20101022064258.GB6081@burratino","threadId":"25483","inReplyTo":"20101022063837.GA6081@burratino","subject":"[PATCH 1/7] branch -h: show usage even in an invalid repository","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-22T06:42:58Z","receivedAt":"2010-10-22T06:42:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nThere is no need for \"git branch -h\" to try to access a repository.\n\nIn the spirit of v1.6.6-rc0~34^2~3 (Let 'git -h' show usage\nwithout a git dir, 2009-11-09). This brings git one step closer to\npassing the following (automatically verifiable) test:\n\n Before any repository access (aside from git_config()), a\n function from the setup_git_directory_* family has been run\n\nand thus one step closer to being able to use an automatic repository\naccess checker.\n\n[jn: simplified; new commit message, test]\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Jonathan Nieder \n---\n builtin/branch.c | 3 +++\n t/t3200-branch.sh | 11 +++++++++++\n 2 files changed, 14 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 87976f0..0e50556 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -667,6 +667,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tOPT_END(),\n \t};\n \n+\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n+\t\tusage_with_options(builtin_branch_usage, options);\n+\n \tgit_config(git_branch_config, NULL);\n \n \tif (branch_use_color == -1)\ndiff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\nindex f54a533..f308235 100755\n--- a/t/t3200-branch.sh\n+++ b/t/t3200-branch.sh\n@@ -26,6 +26,17 @@ test_expect_success \\\n ! test -f .git/refs/heads/--help\n '\n \n+test_expect_success 'branch -h in broken repository' '\n+\tmkdir broken &&\n+\t(\n+\t\tcd broken &&\n+\t\tgit init &&\n+\t\t>.git/refs/heads/master &&\n+\t\ttest_expect_code 129 git branch -h >usage 2>&1\n+\t) &&\n+\tgrep \"[Uu]sage\" broken/usage\n+'\n+\n test_expect_success \\\n 'git branch abc should create a branch' \\\n 'git branch abc && test -f .git/refs/heads/abc'\n-- \n1.7.2.3\n"},{"id":"154109","messageId":"20101022064400.GC6081@burratino","threadId":"25483","inReplyTo":"20101022063837.GA6081@burratino","subject":"[PATCH 2/7] checkout-index -h: show usage even in an invalid repository","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-22T06:44:01Z","receivedAt":"2010-10-22T06:44:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\ncheckout-index loads the index before parsing options. Erroring out\nis counterproductive at that point if the operator is hunting for a\ncommand to recover useful data from the broken repository.\n\n[jn: new commit message, tests]\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Jonathan Nieder \n---\n builtin/checkout-index.c | 3 +++\n t/t2006-checkout-index-basic.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 27 insertions(+), 0 deletions(-)\n create mode 100755 t/t2006-checkout-index-basic.sh\n\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex a7a5ee1..3bf3422 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -241,6 +241,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n+\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n+\t\tusage_with_options(builtin_checkout_index_usage,\n+\t\t\t\t builtin_checkout_index_options);\n \tgit_config(git_default_config, NULL);\n \tstate.base_dir = \"\";\n \tprefix_length = prefix ? strlen(prefix) : 0;\ndiff --git a/t/t2006-checkout-index-basic.sh b/t/t2006-checkout-index-basic.sh\nnew file mode 100755\nindex 0000000..b855983\n--- /dev/null\n+++ b/t/t2006-checkout-index-basic.sh\n@@ -0,0 +1,24 @@\n+#!/bin/sh\n+\n+test_description='basic checkout-index tests\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'checkout-index --gobbledegook' '\n+\ttest_expect_code 129 git checkout-index --gobbledegook 2>err &&\n+\tgrep \"[Uu]sage\" err\n+'\n+\n+test_expect_success 'checkout-index -h in broken repository' '\n+\tmkdir broken &&\n+\t(\n+\t\tcd broken &&\n+\t\tgit init &&\n+\t\t>.git/index &&\n+\t\ttest_expect_code 129 git checkout-index -h >usage 2>&1\n+\t) &&\n+\tgrep \"[Uu]sage\" broken/usage\n+'\n+\n+test_done\n-- \n1.7.2.3\n"},{"id":"154110","messageId":"20101022064547.GD6081@burratino","threadId":"25483","inReplyTo":"20101022063837.GA6081@burratino","subject":"[PATCH 3/7] commit/status -h: show usage even with broken configuration","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-22T06:45:47Z","receivedAt":"2010-10-22T06:45:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\n\"git status\" and \"git commit\" read .git/config and .gitmodules before\nparsing options, but there is no reason to access a repository at all\nwhen the caller just wanted to know what arguments are accepted.\n\n[jn: rewrote the log message and added test]\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Jonathan Nieder \n---\n builtin/commit.c | 6 ++++++\n t/t7508-status.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 66fdd22..0abb430 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1070,6 +1070,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \t\tOPT_END(),\n \t};\n \n+\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n+\t\tusage_with_options(builtin_status_usage, builtin_status_options);\n+\n \tif (null_termination && status_format == STATUS_FORMAT_LONG)\n \t\tstatus_format = STATUS_FORMAT_PORCELAIN;\n \n@@ -1255,6 +1258,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tint allow_fast_forward = 1;\n \tstruct wt_status s;\n \n+\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n+\t\tusage_with_options(builtin_commit_usage, builtin_commit_options);\n+\n \twt_status_prepare(&s);\n \tgit_config(git_commit_config, &s);\n \tin_merge = file_exists(git_path(\"MERGE_HEAD\"));\ndiff --git a/t/t7508-status.sh b/t/t7508-status.sh\nindex c9300f3..beaae94 100755\n--- a/t/t7508-status.sh\n+++ b/t/t7508-status.sh\n@@ -7,6 +7,30 @@ test_description='git status'\n \n . ./test-lib.sh\n \n+test_expect_success 'status -h in broken repository' '\n+\tmkdir broken &&\n+\ttest_when_finished \"rm -fr broken\" &&\n+\t(\n+\t\tcd broken &&\n+\t\tgit init &&\n+\t\techo \"[status] showuntrackedfiles = CORRUPT\" >>.git/config &&\n+\t\ttest_expect_code 129 git status -h >usage 2>&1\n+\t) &&\n+\tgrep \"[Uu]sage\" broken/usage\n+'\n+\n+test_expect_success 'commit -h in broken repository' '\n+\tmkdir broken &&\n+\ttest_when_finished \"rm -fr broken\" &&\n+\t(\n+\t\tcd broken &&\n+\t\tgit init &&\n+\t\techo \"[status] showuntrackedfiles = CORRUPT\" >>.git/config &&\n+\t\ttest_expect_code 129 git commit -h >usage 2>&1\n+\t) &&\n+\tgrep \"[Uu]sage\" broken/usage\n+'\n+\n test_expect_success 'setup' '\n \t: >tracked &&\n \t: >modified &&\n-- \n1.7.2.3\n"},{"id":"154111","messageId":"20101022064719.GE6081@burratino","threadId":"25483","inReplyTo":"20101022063837.GA6081@burratino","subject":"[PATCH 4/7] gc -h: show usage even with broken configuration","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-22T06:47:19Z","receivedAt":"2010-10-22T06:47:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nGiven a request for command-line usage information rather than some\nmore substantial action, the only friendly thing to do is to report\nthe usage information as soon as possible and exit.\n\nWithout this change, as \"git gc\" glances over the repository, it can\nbe distracted by the desire to report a malformed configuration file.\n\nNoticed while working through reports from Duy's repository access\nchecker.\n\n[jn: with rewritten log message and tests]\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Jonathan Nieder \n---\n builtin/gc.c | 3 +++\n t/t6500-gc.sh | 28 ++++++++++++++++++++++++++++\n 2 files changed, 31 insertions(+), 0 deletions(-)\n create mode 100755 t/t6500-gc.sh\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c304638..93deed5 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -189,6 +189,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n+\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n+\t\tusage_with_options(builtin_gc_usage, builtin_gc_options);\n+\n \tgit_config(gc_config, NULL);\n \n \tif (pack_refs < 0)\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nnew file mode 100755\nindex 0000000..82f3639\n--- /dev/null\n+++ b/t/t6500-gc.sh\n@@ -0,0 +1,28 @@\n+#!/bin/sh\n+\n+test_description='basic git gc tests\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'gc empty repository' '\n+\tgit gc\n+'\n+\n+test_expect_success 'gc --gobbledegook' '\n+\ttest_expect_code 129 git gc --nonsense 2>err &&\n+\tgrep \"[Uu]sage: git gc\" err\n+'\n+\n+test_expect_success 'gc -h with invalid configuration' '\n+\tmkdir broken &&\n+\t(\n+\t\tcd broken &&\n+\t\tgit init &&\n+\t\techo \"[gc] pruneexpire = CORRUPT\" >>.git/config &&\n+\t\ttest_expect_code 129 git gc -h >usage 2>&1\n+\t) &&\n+\tgrep \"[Uu]sage\" broken/usage\n+'\n+\n+test_done\n-- \n1.7.2.3\n"},{"id":"154112","messageId":"20101022064814.GF6081@burratino","threadId":"25483","inReplyTo":"20101022063837.GA6081@burratino","subject":"[PATCH 5/7] ls-files -h: show usage even with corrupt index","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-22T06:48:14Z","receivedAt":"2010-10-22T06:48:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nPart of a campaign to avoid git -h being distracted by\naccess to the repository. A caller hoping to use \"git ls-files\"\nwith an alternate index as part of a repair operation may well use\n\"git ls-files -h\" to show usage while planning it out.\n\n[jn: with rewritten log message and tests]\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Jonathan Nieder \n---\n builtin/ls-files.c | 3 +++\n t/t3004-ls-files-basic.sh | 39 +++++++++++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 0 deletions(-)\n create mode 100755 t/t3004-ls-files-basic.sh\n\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex bb4f612..87f0b8a 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -530,6 +530,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)\n \t\tOPT_END()\n \t};\n \n+\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n+\t\tusage_with_options(ls_files_usage, builtin_ls_files_options);\n+\n \tmemset(&dir, 0, sizeof(dir));\n \tprefix = cmd_prefix;\n \tif (prefix)\ndiff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh\nnew file mode 100755\nindex 0000000..490e052\n--- /dev/null\n+++ b/t/t3004-ls-files-basic.sh\n@@ -0,0 +1,39 @@\n+#!/bin/sh\n+\n+test_description='basic ls-files tests\n+\n+This test runs git ls-files with various unusual or malformed\n+command-line arguments.\n+'\n+\n+. ./test-lib.sh\n+\n+>empty\n+\n+test_expect_success 'ls-files in empty repository' '\n+\tgit ls-files >actual &&\n+\ttest_cmp empty actual\n+'\n+\n+test_expect_success 'ls-files with nonexistent path' '\n+\tgit ls-files doesnotexist >actual &&\n+\ttest_cmp empty actual\n+'\n+\n+test_expect_success 'ls-files with nonsense option' '\n+\ttest_expect_code 129 git ls-files --nonsense 2>actual &&\n+\tgrep \"[Uu]sage: git ls-files\" actual\n+'\n+\n+test_expect_success 'ls-files -h in corrupt repository' '\n+\tmkdir broken &&\n+\t(\n+\t\tcd broken &&\n+\t\tgit init &&\n+\t\t>.git/index &&\n+\t\ttest_expect_code 129 git ls-files -h >usage 2>&1\n+\t) &&\n+\tgrep \"[Uu]sage: git ls-files \" broken/usage\n+'\n+\n+test_done\n-- \n1.7.2.3\n"},{"id":"154113","messageId":"20101022064945.GG6081@burratino","threadId":"25483","inReplyTo":"20101022063837.GA6081@burratino","subject":"[PATCH 6/7] merge -h: show usage even with corrupt index","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-22T06:49:45Z","receivedAt":"2010-10-22T06:49:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nPart of a campaign to make sure \"git -h\" works correctly\nwhen run from distractingly bad repositories.\n\n[jn: with rewritten log message and tests]\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Jonathan Nieder \n---\n builtin/merge.c | 2 ++\n t/t7600-merge.sh | 11 +++++++++++\n 2 files changed, 13 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 5f65c0c..584c94f 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -909,6 +909,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n \tstruct commit_list **remotes = &remoteheads;\n \n+\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n+\t\tusage_with_options(builtin_merge_usage, builtin_merge_options);\n \tif (read_cache_unmerged()) {\n \t\tdie_resolve_conflict(\"merge\");\n \t}\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex b4f40e4..b147a1b 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -144,6 +144,17 @@ test_expect_success 'test option parsing' '\n \ttest_must_fail git merge\n '\n \n+test_expect_success 'merge -h with invalid index' '\n+\tmkdir broken &&\n+\t(\n+\t\tcd broken &&\n+\t\tgit init &&\n+\t\t>.git/index &&\n+\t\ttest_expect_code 129 git merge -h 2>usage\n+\t) &&\n+\tgrep \"[Uu]sage: git merge\" broken/usage\n+'\n+\n test_expect_success 'reject non-strategy with a git-merge-foo name' '\n \ttest_must_fail git merge -s index c1\n '\n-- \n1.7.2.3\n"},{"id":"154114","messageId":"20101022065100.GH6081@burratino","threadId":"25483","inReplyTo":"20101022063837.GA6081@burratino","subject":"[PATCH 7/7] update-index -h: show usage even with corrupt index","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-22T06:51:00Z","receivedAt":"2010-10-22T06:51:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nWhen trying to fix up a corrupt repository, one might prefer that\n\"update-index -h\" print an accurate usage message and exit rather\nthan reading the repository and complaining about the corruption.\n\n[jn: with rewritten log message and tests]\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Jonathan Nieder \n---\n builtin/update-index.c | 3 +++\n t/t2107-update-index-basic.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 35 insertions(+), 0 deletions(-)\n create mode 100755 t/t2107-update-index-basic.sh\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 3ab214d..a41d6d7 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -589,6 +589,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \tint lock_error = 0;\n \tstruct lock_file *lock_file;\n \n+\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n+\t\tusage(update_index_usage);\n+\n \tgit_config(git_default_config, NULL);\n \n \t/* We can't free this memory, it becomes part of a linked list parsed atexit() */\ndiff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh\nnew file mode 100755\nindex 0000000..33f8be0\n--- /dev/null\n+++ b/t/t2107-update-index-basic.sh\n@@ -0,0 +1,32 @@\n+#!/bin/sh\n+\n+test_description='basic update-index tests\n+\n+Tests for command-line parsing and basic operation.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'update-index --nonsense fails' '\n+\ttest_must_fail git update-index --nonsense 2>msg &&\n+\tcat msg &&\n+\ttest -s msg\n+'\n+\n+test_expect_failure 'update-index --nonsense dumps usage' '\n+\ttest_expect_code 129 git update-index --nonsense 2>err &&\n+\tgrep \"[Uu]sage: git update-index\" err\n+'\n+\n+test_expect_success 'update-index -h with corrupt index' '\n+\tmkdir broken &&\n+\t(\n+\t\tcd broken &&\n+\t\tgit init &&\n+\t\t>.git/index &&\n+\t\ttest_expect_code 129 git update-index -h >usage 2>&1\n+\t) &&\n+\tgrep \"[Uu]sage: git update-index\" broken/usage\n+'\n+\n+test_done\n-- \n1.7.2.3\n"},{"id":"154115","messageId":"20101022065522.GA3402@burratino","threadId":"25483","inReplyTo":"20101022063837.GA6081@burratino","subject":"[PATCH en/and-cascade-tests 0/7] avoid repository access during \"git -h\"","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-22T06:55:22Z","receivedAt":"2010-10-22T06:55:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> [Subject: [PATCH en/and-cascade-tests 0/7] ]\n\nHmph, teaches me to send a series at night time. The topic is\nprinting usage information early to avoid repository access during\n\"git -h\".\n\nSorry, I should have prepared and sent that earlier. Hope it\nhelps still.\n"},{"id":"154137","messageId":"AANLkTimm0_L9EwhcXu7DbJVdG8kO+qL7RuMa-x4RfM_e@mail.gmail.com","threadId":"25483","inReplyTo":"20101022063837.GA6081@burratino","subject":"Re: [PATCH en/and-cascade-tests 0/7]","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-10-22T11:23:48Z","receivedAt":"2010-10-22T11:23:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2010/10/22 Jonathan Nieder :\n> Nguyễn Thái Ngọc Duy (7):\n>  branch -h: show usage even in an invalid repository\n>  checkout-index -h: show usage even in an invalid repository\n>  commit/status -h: show usage even with broken configuration\n>  gc -h: show usage even with broken configuration\n>  ls-files -h: show usage even with corrupt index\n>  merge -h: show usage even with corrupt index\n>  update-index -h: show usage even with corrupt index\n\nYou should take the credit. I bet my code change does not take as much\ntime as your writing tests. Nice tests by the way. Inspiring.\n-- \nDuy\n"},{"id":"154181","messageId":"7v8w1qnkr1.fsf@alter.siamese.dyndns.org","threadId":"25483","inReplyTo":"20101022064258.GB6081@burratino","subject":"Re: [PATCH 1/7] branch -h: show usage even in an invalid repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-22T18:30:10Z","receivedAt":"2010-10-22T18:30:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> [jn: simplified; new commit message, test]\n\nOverall (not just this 1/7) the messages justify the changes a lot\nbetter. Thanks for a pleasant read ;-).\n\n> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh\n> index f54a533..f308235 100755\n> --- a/t/t3200-branch.sh\n> +++ b/t/t3200-branch.sh\n> @@ -26,6 +26,17 @@ test_expect_success \\\n> ! test -f .git/refs/heads/--help\n> '\n> \n> +test_expect_success 'branch -h in broken repository' '\n> +\tmkdir broken &&\n> +\t(\n> +\t\tcd broken &&\n> +\t\tgit init &&\n> +\t\t>.git/refs/heads/master &&\n> +\t\ttest_expect_code 129 git branch -h >usage 2>&1\n> +\t) &&\n> +\tgrep \"[Uu]sage\" broken/usage\n> +'\n\nA handful of points that struck me (not just test, but taking tests from\nother patches together) as slightly odd, all minor:\n\n - Why '[Uu]sage\"? Should we make the messages consistent further?\n\n - The final \"grep\" check may want to make sure that the message is free of\n \"fatal\" (or \"error\", \"warning\", etc.) as well?\n\n - Each test seems to anticipate a specific kind of breakage tailored for\n the command being tested (e.g. \"branch\" test does not corrupt config\n nor the index). Perhaps \"lib-corrupt.sh\" helper to run the same test\n in variety of corrupt repositories would help improve coverage? [*1*]\n\n - Some tests redirect both the standard output and the standard error\n (like this patch) and check the combined result, while some others\n (e.g. 2/7) check only the standard error stream. Don't we want to be\n testing them more uniformly?\n\n> test_expect_success \\\n> 'git branch abc should create a branch' \\\n> 'git branch abc && test -f .git/refs/heads/abc'\n\nDon't we want to rather use resolve-ref or \"rev-parse --verify\", just in\ncase we may later change \"git branch\" to update packed-refs directly?\n\n\n[Footnote]\n\n*1* I am of two minds, as for example a corrupt \"gc.pruneexpire\" cannot\npossibly matter for correct operation of \"git branch\", but am just\nthrowing out an idea to see if somebody else have clever ideas.\n"},{"id":"154180","messageId":"7v1v7inkqr.fsf@alter.siamese.dyndns.org","threadId":"25483","inReplyTo":"20101022064814.GF6081@burratino","subject":"Re: [PATCH 5/7] ls-files -h: show usage even with corrupt index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-22T18:30:20Z","receivedAt":"2010-10-22T18:30:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> +test_expect_success 'ls-files with nonexistent path' '\n> +\tgit ls-files doesnotexist >actual &&\n> +\ttest_cmp empty actual\n> +'\n\nThis technically is not \"nonexistent path\" but is \"a pathspec that matches\nnothing\".\n"},{"id":"154182","messageId":"7vtykem64h.fsf@alter.siamese.dyndns.org","threadId":"25483","inReplyTo":"20101022064945.GG6081@burratino","subject":"Re: [PATCH 6/7] merge -h: show usage even with corrupt index","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-22T18:31:26Z","receivedAt":"2010-10-22T18:31:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> +test_expect_success 'merge -h with invalid index' '\n\nYou say \"in broken repository\" in 1-3, \"with invalid configuration\" in 4,\n\"in corrupt repository\" in 5, \"with invalid index\" here, and \"with corrupt\nindex\" in 7.\n\n> +\tmkdir broken &&\n> +\t(\n> +\t\tcd broken &&\n> +\t\tgit init &&\n> +\t\t>.git/index &&\n\nI think describing which aspect of brokenness the test is interested in on\nthe title line is better; let's restate \"in broken/corrupt repository\"\n(1-3, 5) to be more specific, and match others in terminology. E.g.\n\n 1. corrupt ref\n 2. corrupt index\n 3. corrupt status.showuntrackedfiles config\n 4. corrupt gc.pruneexpire config\n 5. corrupt index\n 6. corrupt index\n 7. corrupt index\n"},{"id":"154294","messageId":"20101024072032.GA23455@burratino","threadId":"25483","inReplyTo":"7v8w1qnkr1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/7] branch -h: show usage even in an invalid repository","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-24T07:20:32Z","receivedAt":"2010-10-24T07:20:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> - Some tests redirect both the standard output and the standard error\n> (like this patch) and check the combined result, while some others\n> (e.g. 2/7) check only the standard error stream. Don't we want to be\n> testing them more uniformly?\n\nGood point. The current state is:\n\n - Messages from a mistake in usage are reported to stderr.\n - Messages from git -h are reported to stdout or\n stderr, with a slight preference for stdout.\n\nA nicer behavior would be\n\n - Messages from a mistake in usage are reported to stderr.\n - Messages from git -h are reported to stdout.\n\nHere's a helper to make it easier for commands that use parse-options\nto adopt that nicer behavior. It writes its output to stdout, so it\nshould only be used to be used to handle the -h option. Example:\n\n\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n\t\tusage_for_help_opt(builtin_foo_usage, options);\n\n\t... some scary commands that should not run with -h ...\n\n\targc = parse_options(argc, ...\n\tif (argc != 1)\n\t\tusage_with_options(builtin_foo_usage, options);\n\nSigned-off-by: Jonathan Nieder \n---\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 0fa79bc..e92fcfe 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -567,6 +567,13 @@ void usage_with_options(const char * const *usagestr,\n \texit(129);\n }\n \n+void usage_for_help_opt(const char * const *usagestr,\n+\t\t\tconst struct option *opts)\n+{\n+\tusage_with_options_internal(NULL, usagestr, opts, 0, 0);\n+\texit(129);\n+}\n+\n void usage_msg_opt(const char *msg,\n \t\t const char * const *usagestr,\n \t\t const struct option *options)\ndiff --git a/parse-options.h b/parse-options.h\nindex d982f0f..de69826 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -152,6 +152,9 @@ extern int parse_options(int argc, const char **argv, const char *prefix,\n extern NORETURN void usage_with_options(const char * const *usagestr,\n const struct option *options);\n \n+extern NORETURN void usage_for_help_opt(const char * const *usagestr,\n+ const struct option *options);\n+\n extern NORETURN void usage_msg_opt(const char *msg,\n \t\t\t\t const char * const *usagestr,\n \t\t\t\t const struct option *options);\n"},{"id":"154295","messageId":"20101024081316.GA29630@burratino","threadId":"25483","inReplyTo":"20101024072032.GA23455@burratino","subject":"[RFC/PATCH 0/4] update-index: migrate to parse-options API","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-24T08:13:16Z","receivedAt":"2010-10-24T08:13:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> Here's a helper to make it easier for commands that use parse-options\n> to adopt that nicer behavior. It writes its output to stdout, so it\n> should only be used to be used to handle the -h option.\n\nAlas, \"git update-index\" does not use parse-options yet. But that\nis easily enough changed...\n\nPatch 1 introduces an OPTION_LOWLEVEL_CALLBACK backdoor to\nparse-options, so new option types (like the three-argument type\nused by update-index --cacheinfo) can be supported without tempting\ninventors of other commands through mention in the public API.\n\nPatch 2 is a toy patch I previously send to Bo. It makes\nparse_option_step() distinguish between stops due to end of options\nand stops due to appearance of a nonoption; update-index benefits\nfrom something like this because it needs to deal with filename\nparameters as they appear.\n\nPatch 3 converts update-index to parse-options. The commit message\nmentions some potential simplifications.\n\nAll three patches ought to introduce new tests, but none do. I\nhope that does not hinder reading them too much. Thoughts?\n\nJonathan Nieder (3):\n parse-options: allow git commands to invent new option types\n parse-options: make resuming easier after\n PARSE_OPT_STOP_AT_NON_OPTION\n update-index: migrate to parse-options API\n\nNguyễn Thái Ngọc Duy (1):\n setup: save prefix (original cwd relative to toplevel) in\n startup_info\n\n builtin/update-index.c | 389 +++++++++++++++++++++++++++++------------------\n cache.h | 1 +\n parse-options.c | 9 +-\n parse-options.h | 10 ++\n setup.c | 4 +-\n 5 files changed, 259 insertions(+), 154 deletions(-)\n\n-- \n1.7.2.3\n"},{"id":"154296","messageId":"20101024081507.GB29630@burratino","threadId":"25483","inReplyTo":"20101024081316.GA29630@burratino","subject":"[PATCH 1/4] parse-options: allow git commands to invent new option types","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-24T08:15:07Z","receivedAt":"2010-10-24T08:15:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"parse-options provides a variety of option behaviors, including\nOPTION_CALLBACK, which should take care of just about any sane\nbehavior. All supported behaviors obey the following constraint:\n\n A --foo option can only accept (and base its behavior on)\n one argument, which would be the following command-line\n argument in the \"unsticked\" form.\n\nAlas, some existing git commands have options that do not obey that\nconstraint. For example, update-index --cacheinfo takes three\narguments, and update-index --resolve takes all later parameters as\narguments.\n\nIntroduce a new option type (OPTION_LOWLEVEL_CALLBACK) to support\nsuch unusual options. parse_options() callers can implement an\narbitrary custom get_value() function to override the usual one and\npass it through the callback field for options of interest.\n\nSigned-off-by: Jonathan Nieder \n---\n parse-options.c | 6 +++---\n parse-options.h | 9 +++++++++\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 0fa79bc..7907306 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -8,9 +8,6 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n \t\t\t const char * const *usagestr,\n \t\t\t const struct option *opts, int err);\n \n-#define OPT_SHORT 1\n-#define OPT_UNSET 2\n-\n static int opterror(const struct option *opt, const char *reason, int flags)\n {\n \tif (flags & OPT_SHORT)\n@@ -51,6 +48,9 @@ static int get_value(struct parse_opt_ctx_t *p,\n \tconst int unset = flags & OPT_UNSET;\n \tint err;\n \n+\tif (opt->type == OPTION_LOWLEVEL_CALLBACK)\n+\t\treturn (*(parse_opt_ll_cb *)opt->callback)(p, opt, flags);\n+\n \tif (unset && p->opt)\n \t\treturn opterror(opt, \"takes no value\", flags);\n \tif (unset && (opt->flags & PARSE_OPT_NONEG))\ndiff --git a/parse-options.h b/parse-options.h\nindex d982f0f..fa400da 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -17,6 +17,7 @@ enum parse_opt_type {\n \tOPTION_STRING,\n \tOPTION_INTEGER,\n \tOPTION_CALLBACK,\n+\tOPTION_LOWLEVEL_CALLBACK,\n \tOPTION_FILENAME\n };\n \n@@ -40,8 +41,16 @@ enum parse_opt_option_flags {\n \tPARSE_OPT_SHELL_EVAL = 256\n };\n \n+enum parse_opt_ll_flags {\n+\tOPT_SHORT = 1,\n+\tOPT_UNSET = 2\n+};\n+\n struct option;\n+struct parse_opt_ctx_t;\n typedef int parse_opt_cb(const struct option *, const char *arg, int unset);\n+typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,\n+\t\t\t const struct option *opt, int flags);\n \n /*\n * `type`::\n-- \n1.7.2.3\n"},{"id":"154297","messageId":"20101024081532.GC29630@burratino","threadId":"25483","inReplyTo":"20101024081316.GA29630@burratino","subject":"[PATCH 2/4] parse-options: make resuming easier after PARSE_OPT_STOP_AT_NON_OPTION","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-24T08:15:32Z","receivedAt":"2010-10-24T08:15:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Introduce a PARSE_OPT_NON_OPTION state, so parse_option_step()\ncallers can easily distinguish between non-options and other\nreasons for option parsing termination (like \"--\").\n\nSigned-off-by: Jonathan Nieder \n---\n parse-options.c | 3 ++-\n parse-options.h | 1 +\n 2 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 7907306..cba982a 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -374,7 +374,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\tif (parse_nodash_opt(ctx, arg, options) == 0)\n \t\t\t\tcontinue;\n \t\t\tif (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)\n-\t\t\t\tbreak;\n+\t\t\t\treturn PARSE_OPT_NON_OPTION;\n \t\t\tctx->out[ctx->cpidx++] = ctx->argv[0];\n \t\t\tcontinue;\n \t\t}\n@@ -456,6 +456,7 @@ int parse_options(int argc, const char **argv, const char *prefix,\n \tswitch (parse_options_step(&ctx, options, usagestr)) {\n \tcase PARSE_OPT_HELP:\n \t\texit(129);\n+\tcase PARSE_OPT_NON_OPTION:\n \tcase PARSE_OPT_DONE:\n \t\tbreak;\n \tdefault: /* PARSE_OPT_UNKNOWN */\ndiff --git a/parse-options.h b/parse-options.h\nindex fa400da..07a0543 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -170,6 +170,7 @@ extern NORETURN void usage_msg_opt(const char *msg,\n enum {\n \tPARSE_OPT_HELP = -1,\n \tPARSE_OPT_DONE,\n+\tPARSE_OPT_NON_OPTION,\n \tPARSE_OPT_UNKNOWN\n };\n \n-- \n1.7.2.3\n"},{"id":"154298","messageId":"20101024081655.GD29630@burratino","threadId":"25483","inReplyTo":"20101024081316.GA29630@burratino","subject":"[PATCH 3/4] setup: save prefix (original cwd relative to toplevel) in startup_info","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-24T08:16:55Z","receivedAt":"2010-10-24T08:16:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nSave the path from the original cwd to the cwd at the end of the\nsetup procedure in the startup_info struct introduced in e37c1329\n(2010-08-05). The value cannot vary from thread to thread anyway,\nsince the cwd is global.\n\nSo now in your builtin command, instead of passing prefix around,\nwhen you want to convert a user-supplied path to a cwd-relative\npath, you can use startup_info->prefix directly.\n\nCaveat: As with the return value from setup_git_directory_gently(),\nstartup_info->prefix would be NULL when the original cwd is not a\nsubdir of the toplevel.\n\nLonger term, this woiuld allow the prefix to be reused when several\nnoncooperating functions require access to the same repository (for\nexample, when accessing configuration before running a builtin).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Jonathan Nieder \n---\n cache.h | 1 +\n setup.c | 4 +++-\n 2 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 33decd9..222d9cf 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1117,6 +1117,7 @@ const char *split_cmdline_strerror(int cmdline_errno);\n /* git.c */\n struct startup_info {\n \tint have_repository;\n+\tconst char *prefix;\n };\n extern struct startup_info *startup_info;\n \ndiff --git a/setup.c b/setup.c\nindex a3b76de..833db12 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -512,8 +512,10 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \tconst char *prefix;\n \n \tprefix = setup_git_directory_gently_1(nongit_ok);\n-\tif (startup_info)\n+\tif (startup_info) {\n \t\tstartup_info->have_repository = !nongit_ok || !*nongit_ok;\n+\t\tstartup_info->prefix = prefix;\n+\t}\n \treturn prefix;\n }\n \n-- \n1.7.2.3\n"},{"id":"154299","messageId":"20101024081844.GE29630@burratino","threadId":"25483","inReplyTo":"20101024081316.GA29630@burratino","subject":"[PATCH 4/4] update-index: migrate to parse-options API","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-24T08:18:44Z","receivedAt":"2010-10-24T08:18:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"--refresh and --really-refresh accept flags (like -q) and modify\nan error indicator. It might make sense to make the error\nindicator global; but just pass the flags and a pointer to the error\nindicator in a struct instead.\n\n--cacheinfo wants 3 arguments. Use the new OPTION_LOWLEVEL_CALLBACK\nextension to grab them.\n\n--assume-unchanged and --no-assume-unchanged probably should use the\nOPT_UYN feature; but use a callback for now so the existing MARK_FLAG\nand UNMARK_FLAG values can be used.\n\n--stdin and --index-info are still constrained to be the last argument\n(implemented using the OPTION_LOWLEVEL_CALLBACK extension).\n\n--unresolve and --again consume all arguments that come after them\n(also using OPTION_LOWLEVEL_CALLBACK).\n\nThe order of options matters. Each path on the command line is\naffected only by the options that come before it; use a custom\nargument-parsing loop with parse_options_step() to bring that about.\n\nIn exchange for all the fuss, we get the usual perks: support for\nun-sticked options, better usage error messages, more useful -h\noutput, and argument parsing code that should be easier to tweak\nin the future.\n\nSigned-off-by: Jonathan Nieder \n---\n builtin/update-index.c | 389 +++++++++++++++++++++++++++++------------------\n 1 files changed, 240 insertions(+), 149 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 62d9f3f..021d054 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -10,6 +10,7 @@\n #include \"builtin.h\"\n #include \"refs.h\"\n #include \"resolve-undo.h\"\n+#include \"parse-options.h\"\n \n /*\n * Default to not allowing changes to the list of files. The\n@@ -397,8 +398,10 @@ static void read_index_info(int line_termination)\n \tstrbuf_release(&uq);\n }\n \n-static const char update_index_usage[] =\n-\"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] [--] [...]\";\n+static const char * const update_index_usage[] = {\n+\"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] [--] [...]\",\n+\tNULL\n+};\n \n static unsigned char head_sha1[20];\n static unsigned char merge_head_sha1[20];\n@@ -578,16 +581,207 @@ static int do_reupdate(int ac, const char **av,\n \treturn 0;\n }\n \n+struct refresh_params {\n+\tunsigned int flags;\n+\tint *has_errors;\n+};\n+\n+static int refresh_cb_1(const struct option *opt, const char *arg, int unset,\n+\t\t\tunsigned int flag)\n+{\n+\tstruct refresh_params *o = opt->value;\n+\tsetup_work_tree();\n+\t*o->has_errors |= refresh_cache(o->flags | flag);\n+\treturn 0;\n+}\n+\n+static int refresh_cb(const struct option *opt, const char *arg, int unset)\n+{\n+\treturn refresh_cb_1(opt, arg, unset, 0);\n+}\n+\n+static int really_refresh_cb(const struct option *opt, const char *arg, int unset)\n+{\n+\treturn refresh_cb_1(opt, arg, unset, REFRESH_REALLY);\n+}\n+\n+static int chmod_arg_cb(const struct option *opt, const char *arg, int unset)\n+{\n+\tchar *set_executable_bit = opt->value;\n+\tif ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2])\n+\t\treturn error(\"argument to --chmod must be +x or -x\");\n+\t*set_executable_bit = arg[0];\n+\treturn 0;\n+}\n+\n+static int resolve_undo_clear_cb(const struct option *opt, const char *arg, int unset)\n+{\n+\tresolve_undo_clear();\n+\treturn 0;\n+}\n+\n+static int cacheinfo_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,\n+\t\t\tint flags)\n+{\n+\tunsigned char sha1[20];\n+\tunsigned int mode;\n+\tif (flags & OPT_UNSET)\n+\t\treturn error(\"--cacheinfo cannot be negated\");\n+\tif (ctx->opt)\n+\t\treturn error(\"--cacheinfo does not accept an attached argument\");\n+\tif (ctx->argc <= 3)\n+\t\treturn error(\"--cacheinfo requires three arguments\");\n+\tif (strtoul_ui(*++ctx->argv, 8, &mode) ||\n+\t get_sha1_hex(*++ctx->argv, sha1) ||\n+\t add_cacheinfo(mode, sha1, *++ctx->argv, 0))\n+\t\tdie(\"git update-index: --cacheinfo cannot add %s\", *ctx->argv);\n+\tctx->argc -= 3;\n+\treturn 0;\n+}\n+\n+static int stdin_cacheinfo_cb(struct parse_opt_ctx_t *ctx,\n+\t\t\t const struct option *opt, int flags)\n+{\n+\tint *line_termination = opt->value;\n+\tif (ctx->argc != 1 || ctx->opt)\n+\t\treturn error(\"--%s must be at the end\", opt->long_name);\n+\tif (flags & OPT_UNSET)\n+\t\treturn error(\"--%s cannot be negated\", opt->long_name);\n+\tallow_add = allow_replace = allow_remove = 1;\n+\tread_index_info(*line_termination);\n+\treturn 0;\n+}\n+\n+static int last_arg_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,\n+\t\t int flags)\n+{\n+\tint *read_from_stdin = opt->value;\n+\tif (ctx->argc != 1)\n+\t\treturn error(\"--%s must be at the end\", opt->long_name);\n+\tif (flags & OPT_UNSET)\n+\t\treturn error(\"--%s cannot be negated\", opt->long_name);\n+\t*read_from_stdin = 1;\n+\treturn 0;\n+}\n+\n+static int unresolve_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,\n+\t\t\tint flags)\n+{\n+\tint *has_errors = opt->value;\n+\tconst char *prefix = startup_info->prefix;\n+\t*has_errors = do_unresolve(ctx->argc, ctx->argv,\n+\t\t\t\t prefix, !prefix ? 0 : strlen(prefix));\n+\tctx->argv += ctx->argc - 1;\n+\tctx->argc = 1;\n+\tif (*has_errors)\n+\t\tactive_cache_changed = 0;\n+\treturn 0;\n+}\n+\n+static int reupdate_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,\n+\t\t int flags)\n+{\n+\tint *has_errors = opt->value;\n+\tconst char *prefix = startup_info->prefix;\n+\tsetup_work_tree();\n+\t*has_errors = do_reupdate(ctx->argc, ctx->argv,\n+\t\t\t\t prefix, !prefix ? 0 : strlen(prefix));\n+\tctx->argv += ctx->argc - 1;\n+\tctx->argc = 1;\n+\tif (*has_errors)\n+\t\tactive_cache_changed = 0;\n+\treturn 0;\n+}\n+\n int cmd_update_index(int argc, const char **argv, const char *prefix)\n {\n-\tint i, newfd, entries, has_errors = 0, line_termination = '\\n';\n-\tint allow_options = 1;\n+\tint newfd, entries, has_errors = 0, line_termination = '\\n';\n \tint read_from_stdin = 0;\n \tint prefix_length = prefix ? strlen(prefix) : 0;\n \tchar set_executable_bit = 0;\n-\tunsigned int refresh_flags = 0;\n+\tstruct refresh_params refresh_args = {0, &has_errors};\n \tint lock_error = 0;\n \tstruct lock_file *lock_file;\n+\tstruct parse_opt_ctx_t ctx;\n+\tint parseopt_state = PARSE_OPT_UNKNOWN;\n+\tstruct option options[] = {\n+\t\tOPT_BIT('q', NULL, &refresh_args.flags,\n+\t\t\t\"continue refresh even when index needs update\",\n+\t\t\tREFRESH_QUIET),\n+\t\tOPT_BIT(0, \"ignore-submodules\", &refresh_args.flags,\n+\t\t\t\"refresh: ignore submodules\",\n+\t\t\tREFRESH_IGNORE_SUBMODULES),\n+\t\tOPT_SET_INT(0, \"add\", &allow_add,\n+\t\t\t\"do not ignore new files\", 1),\n+\t\tOPT_SET_INT(0, \"replace\", &allow_replace,\n+\t\t\t\"let files replace directories and vice-versa\", 1),\n+\t\tOPT_SET_INT(0, \"remove\", &allow_remove,\n+\t\t\t\"notice files missing from worktree\", 1),\n+\t\tOPT_BIT(0, \"unmerged\", &refresh_args.flags,\n+\t\t\t\"refresh even if index contains unmerged entries\",\n+\t\t\tREFRESH_UNMERGED),\n+\t\t{OPTION_CALLBACK, 0, \"refresh\", &refresh_args, NULL,\n+\t\t\t\"refresh stat information\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, refresh_cb},\n+\t\t{OPTION_CALLBACK, 0, \"really-refresh\", &refresh_args, NULL,\n+\t\t\t\"like --refresh, but ignore assume-unchanged setting\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, really_refresh_cb},\n+\t\t{OPTION_LOWLEVEL_CALLBACK, 0, \"cacheinfo\", NULL,\n+\t\t\t\" \",\n+\t\t\t\"add the specified entry to the index\",\n+\t\t\tPARSE_OPT_LITERAL_ARGHELP, (parse_opt_cb *)cacheinfo_cb},\n+\t\t{OPTION_CALLBACK, 0, \"chmod\", &set_executable_bit, \"(+/-)x\",\n+\t\t\t\"override the executable bit of the listed files\",\n+\t\t\tPARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n+\t\t\tchmod_arg_cb},\n+\t\t{OPTION_SET_INT, 0, \"assume-unchanged\", &mark_valid_only, NULL,\n+\t\t\t\"mark files as \\\"assumed to be unmodified\\\"\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL,\n+\t\t\tMARK_FLAG},\n+\t\t{OPTION_SET_INT, 0, \"no-assume-unchanged\", &mark_valid_only, NULL,\n+\t\t\t\"unset \\\"assumed unchanged\\\" bit\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL,\n+\t\t\tUNMARK_FLAG},\n+\t\t{OPTION_SET_INT, 0, \"skip-worktree\", &mark_skip_worktree_only, NULL,\n+\t\t\t\"mark files as \\\"index-only\\\"\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL,\n+\t\t\tMARK_FLAG},\n+\t\t{OPTION_SET_INT, 0, \"no-skip-worktree\", &mark_skip_worktree_only, NULL,\n+\t\t\t\"unset \\\"skip worktree\\\" big\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL,\n+\t\t\tUNMARK_FLAG},\n+\t\tOPT_SET_INT(0, \"info-only\", &info_only,\n+\t\t\t\"add to index only; do not add content to object database\", 1),\n+\t\tOPT_SET_INT(0, \"force-remove\", &force_remove,\n+\t\t\t\"remove named paths even if present in worktree\", 1),\n+\t\tOPT_SET_INT('z', NULL, &line_termination,\n+\t\t\t\"with --stdin: input lines are terminated by null bytes\", '\\0'),\n+\t\t{OPTION_LOWLEVEL_CALLBACK, 0, \"stdin\", &read_from_stdin, NULL,\n+\t\t\t\"read list of paths to be updated from standard input\",\n+\t\t\tPARSE_OPT_NONEG | PARSE_OPT_NOARG,\n+\t\t\t(parse_opt_cb *)last_arg_cb},\n+\t\t{OPTION_LOWLEVEL_CALLBACK, 0, \"index-info\", &line_termination, NULL,\n+\t\t\t\"add entries from standard input to the index\",\n+\t\t\tPARSE_OPT_NONEG | PARSE_OPT_NOARG,\n+\t\t\t(parse_opt_cb *)stdin_cacheinfo_cb},\n+\t\t{OPTION_LOWLEVEL_CALLBACK, 0, \"unresolve\", &has_errors, NULL,\n+\t\t\t\"repopulate stages #2 and #3 for the listed paths\",\n+\t\t\tPARSE_OPT_NONEG | PARSE_OPT_NOARG,\n+\t\t\t(parse_opt_cb *)unresolve_cb},\n+\t\t{OPTION_LOWLEVEL_CALLBACK, 'g', \"again\", &has_errors, NULL,\n+\t\t\t\"only update entries that differ from HEAD\",\n+\t\t\tPARSE_OPT_NONEG | PARSE_OPT_NOARG,\n+\t\t\t(parse_opt_cb *)reupdate_cb},\n+\t\tOPT_BIT(0, \"ignore-missing\", &refresh_args.flags,\n+\t\t\t\"ignore files missing from worktree\",\n+\t\t\tREFRESH_IGNORE_MISSING),\n+\t\tOPT_SET_INT(0, \"verbose\", &verbose,\n+\t\t\t\"report actions to standard output\", 1),\n+\t\t{OPTION_CALLBACK, 0, \"clear-resolve-undo\", NULL, NULL,\n+\t\t\t\"(for porcelains) forget saved unresolved conflicts\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, resolve_undo_clear_cb},\n+\t\tOPT_END()\n+\t};\n \n \tgit_config(git_default_config, NULL);\n \n@@ -602,151 +796,49 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \tif (entries < 0)\n \t\tdie(\"cache corrupted\");\n \n-\tfor (i = 1 ; i < argc; i++) {\n-\t\tconst char *path = argv[i];\n-\t\tconst char *p;\n+\t/*\n+\t * Custom copy of parse_options() because we want to handle\n+\t * filename arguments as they come.\n+\t */\n+\tparse_options_start(&ctx, argc, argv, prefix,\n+\t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n+\twhile (ctx.argc) {\n+\t\tif (parseopt_state != PARSE_OPT_DONE)\n+\t\t\tparseopt_state = parse_options_step(&ctx, options,\n+\t\t\t\t\t\t\t update_index_usage);\n+\t\tif (!ctx.argc)\n+\t\t\tbreak;\n+\t\tswitch (parseopt_state) {\n+\t\tcase PARSE_OPT_HELP:\n+\t\t\texit(129);\n+\t\tcase PARSE_OPT_NON_OPTION:\n+\t\tcase PARSE_OPT_DONE:\n+\t\t{\n+\t\t\tconst char *path = ctx.argv[0];\n+\t\t\tconst char *p;\n \n-\t\tif (allow_options && *path == '-') {\n-\t\t\tif (!strcmp(path, \"--\")) {\n-\t\t\t\tallow_options = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"-q\")) {\n-\t\t\t\trefresh_flags |= REFRESH_QUIET;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--ignore-submodules\")) {\n-\t\t\t\trefresh_flags |= REFRESH_IGNORE_SUBMODULES;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--add\")) {\n-\t\t\t\tallow_add = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--replace\")) {\n-\t\t\t\tallow_replace = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--remove\")) {\n-\t\t\t\tallow_remove = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--unmerged\")) {\n-\t\t\t\trefresh_flags |= REFRESH_UNMERGED;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--refresh\")) {\n-\t\t\t\tsetup_work_tree();\n-\t\t\t\thas_errors |= refresh_cache(refresh_flags);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--really-refresh\")) {\n-\t\t\t\tsetup_work_tree();\n-\t\t\t\thas_errors |= refresh_cache(REFRESH_REALLY | refresh_flags);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--cacheinfo\")) {\n-\t\t\t\tunsigned char sha1[20];\n-\t\t\t\tunsigned int mode;\n-\n-\t\t\t\tif (i+3 >= argc)\n-\t\t\t\t\tdie(\"git update-index: --cacheinfo \");\n-\n-\t\t\t\tif (strtoul_ui(argv[i+1], 8, &mode) ||\n-\t\t\t\t get_sha1_hex(argv[i+2], sha1) ||\n-\t\t\t\t add_cacheinfo(mode, sha1, argv[i+3], 0))\n-\t\t\t\t\tdie(\"git update-index: --cacheinfo\"\n-\t\t\t\t\t \" cannot add %s\", argv[i+3]);\n-\t\t\t\ti += 3;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--chmod=-x\") ||\n-\t\t\t !strcmp(path, \"--chmod=+x\")) {\n-\t\t\t\tif (argc <= i+1)\n-\t\t\t\t\tdie(\"git update-index: %s \", path);\n-\t\t\t\tset_executable_bit = path[8];\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--assume-unchanged\")) {\n-\t\t\t\tmark_valid_only = MARK_FLAG;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--no-assume-unchanged\")) {\n-\t\t\t\tmark_valid_only = UNMARK_FLAG;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--no-skip-worktree\")) {\n-\t\t\t\tmark_skip_worktree_only = UNMARK_FLAG;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--skip-worktree\")) {\n-\t\t\t\tmark_skip_worktree_only = MARK_FLAG;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--info-only\")) {\n-\t\t\t\tinfo_only = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--force-remove\")) {\n-\t\t\t\tforce_remove = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"-z\")) {\n-\t\t\t\tline_termination = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--stdin\")) {\n-\t\t\t\tif (i != argc - 1)\n-\t\t\t\t\tdie(\"--stdin must be at the end\");\n-\t\t\t\tread_from_stdin = 1;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--index-info\")) {\n-\t\t\t\tif (i != argc - 1)\n-\t\t\t\t\tdie(\"--index-info must be at the end\");\n-\t\t\t\tallow_add = allow_replace = allow_remove = 1;\n-\t\t\t\tread_index_info(line_termination);\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--unresolve\")) {\n-\t\t\t\thas_errors = do_unresolve(argc - i, argv + i,\n-\t\t\t\t\t\t\t prefix, prefix_length);\n-\t\t\t\tif (has_errors)\n-\t\t\t\t\tactive_cache_changed = 0;\n-\t\t\t\tgoto finish;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--again\") || !strcmp(path, \"-g\")) {\n-\t\t\t\tsetup_work_tree();\n-\t\t\t\thas_errors = do_reupdate(argc - i, argv + i,\n-\t\t\t\t\t\t\t prefix, prefix_length);\n-\t\t\t\tif (has_errors)\n-\t\t\t\t\tactive_cache_changed = 0;\n-\t\t\t\tgoto finish;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--ignore-missing\")) {\n-\t\t\t\trefresh_flags |= REFRESH_IGNORE_MISSING;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--verbose\")) {\n-\t\t\t\tverbose = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--clear-resolve-undo\")) {\n-\t\t\t\tresolve_undo_clear();\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"-h\") || !strcmp(path, \"--help\"))\n-\t\t\t\tusage(update_index_usage);\n-\t\t\tdie(\"unknown option %s\", path);\n+\t\t\ttrace_printf(\"trace: update-index %s\\n\", path);\n+\t\t\tsetup_work_tree();\n+\t\t\tp = prefix_path(prefix, prefix_length, path);\n+\t\t\tupdate_one(p, NULL, 0);\n+\t\t\tif (set_executable_bit)\n+\t\t\t\tchmod_path(set_executable_bit, p);\n+\t\t\tif (p < path || p > path + strlen(path))\n+\t\t\t\tfree((char *)p);\n+\t\t\tctx.argc--;\n+\t\t\tctx.argv++;\n+\t\t\tbreak;\n+\t\t}\n+\t\tcase PARSE_OPT_UNKNOWN:\n+\t\t\tif (ctx.argv[0][1] == '-')\n+\t\t\t\terror(\"unknown option '%s'\", ctx.argv[0] + 2);\n+\t\t\telse\n+\t\t\t\terror(\"unknown switch '%c'\", *ctx.opt);\n+\t\t\tusage_with_options(update_index_usage, options);\n \t\t}\n-\t\tsetup_work_tree();\n-\t\tp = prefix_path(prefix, prefix_length, path);\n-\t\tupdate_one(p, NULL, 0);\n-\t\tif (set_executable_bit)\n-\t\t\tchmod_path(set_executable_bit, p);\n-\t\tif (p < path || p > path + strlen(path))\n-\t\t\tfree((char *)p);\n \t}\n+\targc = parse_options_end(&ctx);\n+\n \tif (read_from_stdin) {\n \t\tstruct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;\n \n@@ -770,10 +862,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_release(&buf);\n \t}\n \n- finish:\n \tif (active_cache_changed) {\n \t\tif (newfd < 0) {\n-\t\t\tif (refresh_flags & REFRESH_QUIET)\n+\t\t\tif (refresh_args.flags & REFRESH_QUIET)\n \t\t\t\texit(128);\n \t\t\tunable_to_lock_index_die(get_index_file(), lock_error);\n \t\t}\n-- \n1.7.2.3\n"},{"id":"154332","messageId":"AANLkTik9OtRvB2P7OFFsYoUsPjdM8FofKVtGRt+VFQpm@mail.gmail.com","threadId":"25483","inReplyTo":"20101024081316.GA29630@burratino","subject":"Re: [RFC/PATCH 0/4] update-index: migrate to parse-options API","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-10-24T12:50:44Z","receivedAt":"2010-10-24T12:50:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Oct 24, 2010 at 3:13 PM, Jonathan Nieder wrote:\n> Jonathan Nieder wrote:\n>\n>> Here's a helper to make it easier for commands that use parse-options\n>> to adopt that nicer behavior.  It writes its output to stdout, so it\n>> should only be used to be used to handle the -h option.\n>\n> Alas, \"git update-index\" does not use parse-options yet.  But that\n> is easily enough changed...\n\nI don't know parse-options well enough to give comments. If I did, I\nwould have converted update-index long ago (hit it a few times and\nalways ran away). Thanks for the conversion.\n-- \nDuy\n"},{"id":"154397","messageId":"4CC55C4F.5020004@gmail.com","threadId":"25483","inReplyTo":"20101024081844.GE29630@burratino","subject":"Re: [PATCH 4/4] update-index: migrate to parse-options API","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2010-10-25T10:30:39Z","receivedAt":"2010-10-25T10:30:39Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"On 10/24/10 01:18, Jonathan Nieder wrote:\n> ---\n> builtin/update-index.c | 389 +++++++++++++++++++++++++++++------------------\n> 1 files changed, 240 insertions(+), 149 deletions(-)\n> \n\nI would suspect that the code would get smaller. Too many callbacks?\n\n> \n> -static const char update_index_usage[] =\n> -\"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] [--] [...]\";\n> +static const char * const update_index_usage[] = {\n> +\"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] [--] [...]\",\n> +\tNULL\n> +};\n\nPlease drop all this extraneous option stuff since it's already shown in\nthe -h output. The usage should list the different command modes. I know\nthat I've failed to do this in the past (and I should probably fix those).\n\nWould\n\n\tgit update-index [] -- \n\nbe enough?\n\n\n> +static int cacheinfo_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,\n> +\t\t\tint flags)\n> +{\n> +\tunsigned char sha1[20];\n> +\tunsigned int mode;\n> +\tif (flags & OPT_UNSET)\n> +\t\treturn error(\"--cacheinfo cannot be negated\");\n\nThis shouldn't be possible right? I thought parse options made sure\nNONEG options couldn't be negated... . Oh. It\nseems like there will be a lot of duplicated code that way. Maybe we can\nfixup patch 1 a bit so this isn't necessary.\n\n> +\tif (ctx->opt)\n> +\t\treturn error(\"--cacheinfo does not accept an attached argument\");\n\nAnd this too.\n\n> +\tif (ctx->argc <= 3)\n> +\t\treturn error(\"--cacheinfo requires three arguments\");\n> +\tif (strtoul_ui(*++ctx->argv, 8, &mode) ||\n> +\t get_sha1_hex(*++ctx->argv, sha1) ||\n> +\t add_cacheinfo(mode, sha1, *++ctx->argv, 0))\n> +\t\tdie(\"git update-index: --cacheinfo cannot add %s\", *ctx->argv);\n\nHmm, it would be neat if die() always prefixed the death message with\nthe command in which it died.\n\n> +\tctx->argc -= 3;\n> +\treturn 0;\n> +}\n> +\n> +static int stdin_cacheinfo_cb(struct parse_opt_ctx_t *ctx,\n> +\t\t\t const struct option *opt, int flags)\n> +{\n> +\tint *line_termination = opt->value;\n> +\tif (ctx->argc != 1 || ctx->opt)\n> +\t\treturn error(\"--%s must be at the end\", opt->long_name);\n> +\tif (flags & OPT_UNSET)\n> +\t\treturn error(\"--%s cannot be negated\", opt->long_name);\n> +\tallow_add = allow_replace = allow_remove = 1;\n> +\tread_index_info(*line_termination);\n> +\treturn 0;\n> +}\n> +\n> +static int last_arg_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,\n> +\t\t int flags)\n> +{\n> +\tint *read_from_stdin = opt->value;\n> +\tif (ctx->argc != 1)\n> +\t\treturn error(\"--%s must be at the end\", opt->long_name);\n\nThinking out loud, this might be better served as an option flag\n(PARSE_OPT_LAST_ARG?) to make it a bit more generic. Especially since\nyou use it twice.\n\n> +\tif (flags & OPT_UNSET)\n> +\t\treturn error(\"--%s cannot be negated\", opt->long_name);\n> +\t*read_from_stdin = 1;\n> +\treturn 0;\n> +}\n\nAnd then this callback would go away and you could use a custom\nOPTION_SET_PTR (or probably OPTION_SET_INT) right?\n\n\n> +static int reupdate_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,\n> +\t\t int flags)\n> +{\n> +\tint *has_errors = opt->value;\n> +\tconst char *prefix = startup_info->prefix;\n\nDoesn't the context also contain this? I know this is why you included\npatch 3, but it doesn't seem strictly necessary to use startup_info over\nctx.\n\n> +\tsetup_work_tree();\n> +\t*has_errors = do_reupdate(ctx->argc, ctx->argv,\n> +\t\t\t\t prefix, !prefix ? 0 : strlen(prefix));\n> +\tctx->argv += ctx->argc - 1;\n> +\tctx->argc = 1;\n\nAt first I thought you forgot to make this a -= here. Then I realized\nyou're doing this to make parse options skip to the end of processing.\nPerhaps you can just return PARSE_OPT_FINISH (equal to 1?) or something\nto indicate to parse options that you're done parsing options entirely?\nOr put a comment there so I don't get confused again.\n\n\n> +\tstruct option options[] = {\n> +\t\tOPT_BIT('q', NULL, &refresh_args.flags,\n> +\t\t\t\"continue refresh even when index needs update\",\n> +\t\t\tREFRESH_QUIET),\n[snip]\n> +\t\tOPT_SET_INT(0, \"verbose\", &verbose,\n> +\t\t\t\"report actions to standard output\", 1),\n> +\t\t{OPTION_CALLBACK, 0, \"clear-resolve-undo\", NULL, NULL,\n> +\t\t\t\"(for porcelains) forget saved unresolved conflicts\",\n> +\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, resolve_undo_clear_cb},\n> +\t\tOPT_END()\n> +\t};\n\nAny reason for OPT_SET_INT over OPT_BOOLEAN? Just curious.\n\n> -\t\t\tif (!strcmp(path, \"-h\") || !strcmp(path, \"--help\"))\n> -\t\t\t\tusage(update_index_usage);\n> -\t\t\tdie(\"unknown option %s\", path);\n> +\t\t\ttrace_printf(\"trace: update-index %s\\n\", path);\n\nMaybe useful; but doubtful considering we already get the whole command\nline already. Debugging stuff?\n"},{"id":"154398","messageId":"4CC55C67.8080908@gmail.com","threadId":"25483","inReplyTo":"20101024081507.GB29630@burratino","subject":"Re: [PATCH 1/4] parse-options: allow git commands to invent new option types","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2010-10-25T10:31:03Z","receivedAt":"2010-10-25T10:31:03Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"On 10/24/10 01:15, Jonathan Nieder wrote:\n> diff --git a/parse-options.c b/parse-options.c\n> index 0fa79bc..7907306 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -8,9 +8,6 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,\n> \t\t\t const char * const *usagestr,\n> \t\t\t const struct option *opts, int err);\n> \n> -#define OPT_SHORT 1\n> -#define OPT_UNSET 2\n> -\n> static int opterror(const struct option *opt, const char *reason, int flags)\n> {\n> \tif (flags & OPT_SHORT)\n> @@ -51,6 +48,9 @@ static int get_value(struct parse_opt_ctx_t *p,\n> \tconst int unset = flags & OPT_UNSET;\n> \tint err;\n> \n> +\tif (opt->type == OPTION_LOWLEVEL_CALLBACK)\n> +\t\treturn (*(parse_opt_ll_cb *)opt->callback)(p, opt, flags);\n> +\n\n(I read patch 4 before this one)\n\nBeing able to modify the context within a callback is nice. Having to\nknow if the option is short or long and and checking for validity seems\nlike something that should be handled within the parse options library\nitself.\n\nIs there an actual use case where someone needs to completely override\nget_value()? If you move this into the case statement then we get the\ngeneric error checking of get_value() with the benefits of being able to\nmodify the context within a callback. We could also probably use the\nreturn value of the low level callback to indicate whether or not to\ntake some action after parsing the option. Perhaps something like\nquiting the option parsing loop when encountering such an option?\n\nThis reminds me, we can probably simplify that \"takes no value\" error\npath in get_value() (see below).\n\n> diff --git a/parse-options.h b/parse-options.h\n> index d982f0f..fa400da 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -17,6 +17,7 @@ enum parse_opt_type {\n> \tOPTION_STRING,\n> \tOPTION_INTEGER,\n> \tOPTION_CALLBACK,\n> +\tOPTION_LOWLEVEL_CALLBACK,\n> \tOPTION_FILENAME\n> };\n\nDon't forget to document what this option does in the comments of this\nfile and in api-parse-options.txt\n\n> \n> @@ -40,8 +41,16 @@ enum parse_opt_option_flags {\n> \tPARSE_OPT_SHELL_EVAL = 256\n> };\n> \n> +enum parse_opt_ll_flags {\n> +\tOPT_SHORT = 1,\n> +\tOPT_UNSET = 2\n> +};\n> +\n\nI hope this isn't necessary.\n\nHope this pasted ok.\n--->8----\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 0fa79bc..7234c11 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -56,22 +56,8 @@ static int get_value(struct parse_opt_ctx_t *p,\n if (unset && (opt->flags & PARSE_OPT_NONEG))\n return opterror(opt, \"isn't available\", flags);\n\n- if (!(flags & OPT_SHORT) && p->opt) {\n- switch (opt->type) {\n- case OPTION_CALLBACK:\n- if (!(opt->flags & PARSE_OPT_NOARG))\n- break;\n- /* FALLTHROUGH */\n- case OPTION_BOOLEAN:\n- case OPTION_BIT:\n- case OPTION_NEGBIT:\n- case OPTION_SET_INT:\n- case OPTION_SET_PTR:\n+ if (!(flags & OPT_SHORT) && p->opt && (opt->flags &\nPARSE_OPT_NOARG))\n return opterror(opt, \"takes no value\", flags);\n- default:\n- break;\n- }\n- }\n\n switch (opt->type) {\n case OPTION_BIT:\n"},{"id":"154473","messageId":"7vfwvse08z.fsf@alter.siamese.dyndns.org","threadId":"25483","inReplyTo":"20101024081316.GA29630@burratino","subject":"Re: [RFC/PATCH 0/4] update-index: migrate to parse-options API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-27T04:19:08Z","receivedAt":"2010-10-27T04:19:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Jonathan Nieder wrote:\n>\n>> Here's a helper to make it easier for commands that use parse-options\n>> to adopt that nicer behavior. It writes its output to stdout, so it\n>> should only be used to be used to handle the -h option.\n>\n> Alas, \"git update-index\" does not use parse-options yet. But that\n> is easily enough changed...\n>\n> Patch 1 introduces an OPTION_LOWLEVEL_CALLBACK backdoor to\n> parse-options, so new option types (like the three-argument type\n> used by update-index --cacheinfo) can be supported without tempting\n> inventors of other commands through mention in the public API.\n\nI like the idea of this one. I imagine it can also be used to finally\nwhip the command line parsing of \"log -L\" series Thomas mentored into a\nreasonable shape, which needs to parse the -L option that takes \nand an optional (not ) as arguments.\n"},{"id":"156788","messageId":"20101129040354.GA30187@burratino","threadId":"25483","inReplyTo":"4CC55C67.8080908@gmail.com","subject":"Re: [PATCH 1/4] parse-options: allow git commands to invent new option types","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-29T04:03:54Z","receivedAt":"2010-11-29T04:03:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stephen Boyd wrote:\n\n> Is there an actual use case where someone needs to completely override\n> get_value()? If you move this into the case statement then we get the\n> generic error checking of get_value() with the benefits of being able to\n> modify the context within a callback.\n\nYes, I like this idea.\n\n> We could also probably use the\n> return value of the low level callback to indicate whether or not to\n> take some action after parsing the option. Perhaps something like\n> quiting the option parsing loop when encountering such an option?\n\nI'd rather not, since that would involve anticipating needs in advance.\nThis is meant to be a back door for ugly and unusual option types that\nthe mainstream API does not take care of yet. It seems best to allow\narbitrary effects.\n\n[...]\n>> -#define OPT_SHORT 1\n>> -#define OPT_UNSET 2\n[...]\n>> +++ b/parse-options.h\n>> @@ -40,8 +41,16 @@ enum parse_opt_option_flags {\n>> \tPARSE_OPT_SHELL_EVAL = 256\n>> };\n>> \n>> +enum parse_opt_ll_flags {\n>> +\tOPT_SHORT = 1,\n>> +\tOPT_UNSET = 2\n>> +};\n>> +\n>\n> I hope this isn't necessary.\n\nBecause of namespace or something else?\n\n> This reminds me, we can probably simplify that \"takes no value\" error\n> path in get_value() (see below).\n\nNice.\n\nThanks,\nJonathan\n"},{"id":"156873","messageId":"20101130023416.GB19191@burratino","threadId":"25483","inReplyTo":"4CC55C4F.5020004@gmail.com","subject":"Re: [PATCH 4/4] update-index: migrate to parse-options API","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-30T02:34:16Z","receivedAt":"2010-11-30T02:34:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStephen Boyd wrote:\n> On 10/24/10 01:18, Jonathan Nieder wrote:\n\n>> builtin/update-index.c | 389 +++++++++++++++++++++++++++++------------------\n>> 1 files changed, 240 insertions(+), 149 deletions(-)\n>\n> I would suspect that the code would get smaller. Too many callbacks?\n\nThat and added option descriptions.\n\n>> +\"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] [--] [...]\",\n>> +\tNULL\n>> +};\n>\n> Please drop all this extraneous option stuff since it's already shown in\n> the -h output.\n\nGood idea. I put\n\n\tgit update-index [options] [--] [...]\n\neven though that doesn't explain the actual syntax (especially\noddities like --stdin and --unresolve) at all. :)\n\n> This shouldn't be possible right? I thought parse options made sure\n> NONEG options couldn't be negated... . Oh.\n\nFixed.\n\n[...]\n>> +static int last_arg_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,\n>> +\t\t int flags)\n>> +{\n>> +\tint *read_from_stdin = opt->value;\n>> +\tif (ctx->argc != 1)\n>> +\t\treturn error(\"--%s must be at the end\", opt->long_name);\n>\n> Thinking out loud, this might be better served as an option flag\n> (PARSE_OPT_LAST_ARG?) to make it a bit more generic. Especially since\n> you use it twice.\n\nAgreed but I'm not doing it now, since I don't want to encourage other\ncommands. Will reconsider.\n\n>> +static int reupdate_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,\n>> +\t\t int flags)\n>> +{\n>> +\tint *has_errors = opt->value;\n>> +\tconst char *prefix = startup_info->prefix;\n>\n> Doesn't the context also contain this? I know this is why you included\n> patch 3, but it doesn't seem strictly necessary to use startup_info over\n> ctx.\n\nI figure patch 3 is inevitable anyway. It didn't seem right to\npeek at the context, which only includes it as an implementation\ndetail of OPTION_FILENAME support.\n\nMaybe eliminating ctx->prefix would be a good follow-up patch.\n\n>> +\tsetup_work_tree();\n>> +\t*has_errors = do_reupdate(ctx->argc, ctx->argv,\n>> +\t\t\t\t prefix, !prefix ? 0 : strlen(prefix));\n>> +\tctx->argv += ctx->argc - 1;\n>> +\tctx->argc = 1;\n>\n> At first I thought you forgot to make this a -= here.\n\nYep, needs a comment.\n\n>> +\tstruct option options[] = {\n>> +\t\tOPT_BIT('q', NULL, &refresh_args.flags,\n>> +\t\t\t\"continue refresh even when index needs update\",\n>> +\t\t\tREFRESH_QUIET),\n> [snip]\n>> +\t\tOPT_SET_INT(0, \"verbose\", &verbose,\n>> +\t\t\t\"report actions to standard output\", 1),\n>> +\t\t{OPTION_CALLBACK, 0, \"clear-resolve-undo\", NULL, NULL,\n>> +\t\t\t\"(for porcelains) forget saved unresolved conflicts\",\n>> +\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, resolve_undo_clear_cb},\n>> +\t\tOPT_END()\n>> +\t};\n>\n> Any reason for OPT_SET_INT over OPT_BOOLEAN? Just curious.\n\nOPT_BOOLEAN means OPT_INCR and there is only one level of verbosity\nhere.\n\n>> +\t\t\ttrace_printf(\"trace: update-index %s\\n\", path);\n[...]\n> Debugging stuff?\n\nYes, good catch.\n\nThanks again for all the comments, and sorry to take so long to get\nback to this.\n\nRegards,\nJonathan\n"},{"id":"156874","messageId":"20101130025223.GA5326@burratino","threadId":"25483","inReplyTo":"20101024081316.GA29630@burratino","subject":"[PATCH/RFCv2 0/6] update-index: migrate to parse-options API","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-30T02:52:23Z","receivedAt":"2010-11-30T02:52:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Here's a reroll. Changes explained in the comments following each\npatch.\n\nThanks to Stephen for a great deal of helpful advice. The result\nturns out to be less surgery than I thought from the version in pu,\nmostly because I am trying to be conservative (meaning further\nsimplifications are welcome as patches on top, natch).\n\nBased against ca20906, as before.\n\nJonathan Nieder (4):\n parse-options: sanity check PARSE_OPT_NOARG flag\n parse-options: allow git commands to invent new option types\n parse-options: make resuming easier after\n PARSE_OPT_STOP_AT_NON_OPTION\n update-index: migrate to parse-options API\n\nNguyễn Thái Ngọc Duy (1):\n setup: save prefix (original cwd relative to toplevel) in\n startup_info\n\nStephen Boyd (1):\n parse-options: eliminate implicit PARSE_OPT_NOARG for built-in option\n types\n\n builtin/update-index.c | 391 ++++++++++++++++++++++++++++++------------------\n cache.h | 1 +\n parse-options.c | 47 ++++---\n parse-options.h | 9 +-\n setup.c | 4 +-\n 5 files changed, 283 insertions(+), 169 deletions(-)\n"},{"id":"156876","messageId":"20101130025551.GB5326@burratino","threadId":"25483","inReplyTo":"20101130025223.GA5326@burratino","subject":"[PATCH 1/6] parse-options: sanity check PARSE_OPT_NOARG flag","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-30T02:55:51Z","receivedAt":"2010-11-30T02:55:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Some option types cannot use an argument --- boolean options that\nwould set a bit or flag or increment a counter, for example. If\nconfigured in the flag word to accept an argument anyway, the result\nis an argument that is advertised in \"program -h\" output only to be\nrejected by parse-options::get_value.\n\nLuckily all current users of these option types use PARSE_OPT_NOARG\nand do not use PARSE_OPT_OPTARG. Add a check to ensure that that\nremains true.\n\nSigned-off-by: Jonathan Nieder \n---\nNew patch, preparing for patch 2. Maybe the check should be\nimplemented elsewhere (ideally compile time) so all flags can be\nchecked rather than just the flags that happen to be used in a given\ntest run.\n\n parse-options.c | 22 ++++++++++++++++++++++\n 1 files changed, 22 insertions(+), 0 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 0fa79bc..1806bb3 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -44,6 +44,26 @@ static void fix_filename(const char *prefix, const char **file)\n \t*file = xstrdup(prefix_filename(prefix, strlen(prefix), *file));\n }\n \n+static void check_flags(const struct option *opt)\n+{\n+\tswitch (opt->type) {\n+\tcase OPTION_BOOLEAN:\n+\tcase OPTION_BIT:\n+\tcase OPTION_NEGBIT:\n+\tcase OPTION_SET_INT:\n+\tcase OPTION_SET_PTR:\n+\tcase OPTION_NUMBER:\n+\t\tbreak;\n+\tdefault:\t/* (usually accepts an argument) */\n+\t\treturn;\n+\t}\n+\tif ((opt->flags & (PARSE_OPT_OPTARG | PARSE_OPT_NOARG)) == PARSE_OPT_NOARG)\n+\t\treturn;\n+\tdie(\"BUG: option '-%c%s' should not accept an argument\",\n+\t\t\t\t!opt->short_name ? '-' : opt->short_name,\n+\t\t\t\t!opt->short_name ? opt->long_name : \"\");\n+}\n+\n static int get_value(struct parse_opt_ctx_t *p,\n \t\t const struct option *opt, int flags)\n {\n@@ -51,6 +71,8 @@ static int get_value(struct parse_opt_ctx_t *p,\n \tconst int unset = flags & OPT_UNSET;\n \tint err;\n \n+\tcheck_flags(opt);\n+\n \tif (unset && p->opt)\n \t\treturn opterror(opt, \"takes no value\", flags);\n \tif (unset && (opt->flags & PARSE_OPT_NONEG))\n-- \n1.7.2.3\n"},{"id":"156875","messageId":"20101130030441.GC5326@burratino","threadId":"25483","inReplyTo":"20101130025223.GA5326@burratino","subject":"[PATCH 2/6] parse-options: do not infer PARSE_OPT_NOARG from option type","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-30T03:04:41Z","receivedAt":"2010-11-30T03:04:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Stephen Boyd \n\nSimplify the \"takes no value\" error path by relying on PARSE_OPT_NOARG\nbeing set correctly. That is:\n\n - if the PARSE_OPT_NOARG flag is set, reject --opt=value\n regardless of the option type;\n - if the PARSE_OPT_NOARG flag is unset, accept --opt=value\n regardless of the option type.\n\nThis way, the accepted usage more closely matches the usage advertised\nwith --help-all.\n\nNo functional change intended, since the NOARG flag is only used\nwith \"boolean-only\" option types in existing parse_options callers.\n\nSigned-off-by: Jonathan Nieder \n---\n parse-options.c | 19 ++-----------------\n 1 files changed, 2 insertions(+), 17 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 1806bb3..bc92d69 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -77,23 +77,8 @@ static int get_value(struct parse_opt_ctx_t *p,\n \t\treturn opterror(opt, \"takes no value\", flags);\n \tif (unset && (opt->flags & PARSE_OPT_NONEG))\n \t\treturn opterror(opt, \"isn't available\", flags);\n-\n-\tif (!(flags & OPT_SHORT) && p->opt) {\n-\t\tswitch (opt->type) {\n-\t\tcase OPTION_CALLBACK:\n-\t\t\tif (!(opt->flags & PARSE_OPT_NOARG))\n-\t\t\t\tbreak;\n-\t\t\t/* FALLTHROUGH */\n-\t\tcase OPTION_BOOLEAN:\n-\t\tcase OPTION_BIT:\n-\t\tcase OPTION_NEGBIT:\n-\t\tcase OPTION_SET_INT:\n-\t\tcase OPTION_SET_PTR:\n-\t\t\treturn opterror(opt, \"takes no value\", flags);\n-\t\tdefault:\n-\t\t\tbreak;\n-\t\t}\n-\t}\n+\tif (!(flags & OPT_SHORT) && p->opt && (opt->flags & PARSE_OPT_NOARG))\n+\t\treturn opterror(opt, \"takes no value\", flags);\n \n \tswitch (opt->type) {\n \tcase OPTION_BIT:\n-- \n1.7.2.3\n"},{"id":"156877","messageId":"20101130030853.GD5326@burratino","threadId":"25483","inReplyTo":"20101130025223.GA5326@burratino","subject":"[PATCH 3/6] parse-options: allow git commands to invent new option types","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-30T03:08:53Z","receivedAt":"2010-11-30T03:08:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"parse-options provides a variety of option behaviors, including\nOPTION_CALLBACK, which should take care of just about any sane\nbehavior. All supported behaviors obey the following constraint:\n\n A --foo option can only accept (and base its behavior on)\n one argument, which would be the following command-line\n argument in the \"unsticked\" form.\n\nAlas, some existing git commands have options that do not obey that\nconstraint. For example, update-index --cacheinfo takes three\narguments, and update-index --resolve takes all later parameters as\narguments.\n\nIntroduces an OPTION_LOWLEVEL_CALLBACK backdoor to parse-options so\nsuch option types can be supported without tempting inventors of other\ncommands through mention in the public API. Commands can set the\ncallback field to a function accepting three arguments: the option\nparsing context, the option itself, and a flag indicating whether the\nthe option was negated. When the option is encountered, that function\nis called to take over from get_value(). The return value should be\nzero for success, -1 for usage errors.\n\nThanks to Stephen Boyd for API guidance.\n\nImproved-by: Stephen Boyd \nSigned-off-by: Jonathan Nieder \n---\nChanges from v1:\n\n. lowlevel callbacks inherit standard get_value argument\n checking\n. lowlevel callbacks cannot tailor error messages based on\n long vs short option\n. OPT_UNSET and OPT_SHORT therefore do not need to be exposed\n. brief mention in parse-options.h comments.\n\nWe can always tweak the API again later. None of the examples in\nupdate-index even pay attention to the \"unset\" bit.\n\n parse-options.c | 3 +++\n parse-options.h | 8 +++++++-\n 2 files changed, 10 insertions(+), 1 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex bc92d69..38c8fd3 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -81,6 +81,9 @@ static int get_value(struct parse_opt_ctx_t *p,\n \t\treturn opterror(opt, \"takes no value\", flags);\n \n \tswitch (opt->type) {\n+\tcase OPTION_LOWLEVEL_CALLBACK:\n+\t\treturn (*(parse_opt_ll_cb *)opt->callback)(p, opt, unset);\n+\n \tcase OPTION_BIT:\n \t\tif (unset)\n \t\t\t*(int *)opt->value &= ~opt->defval;\ndiff --git a/parse-options.h b/parse-options.h\nindex d982f0f..bd0fe16 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -17,6 +17,7 @@ enum parse_opt_type {\n \tOPTION_STRING,\n \tOPTION_INTEGER,\n \tOPTION_CALLBACK,\n+\tOPTION_LOWLEVEL_CALLBACK,\n \tOPTION_FILENAME\n };\n \n@@ -43,6 +44,10 @@ enum parse_opt_option_flags {\n struct option;\n typedef int parse_opt_cb(const struct option *, const char *arg, int unset);\n \n+struct parse_opt_ctx_t;\n+typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,\n+\t\t\t\tconst struct option *opt, int unset);\n+\n /*\n * `type`::\n * holds the type of the option, you must have an OPTION_END last in your\n@@ -87,7 +92,8 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset);\n *\t\t\t\tuseful for users of OPTION_NEGBIT.\n *\n * `callback`::\n- * pointer to the callback to use for OPTION_CALLBACK.\n+ * pointer to the callback to use for OPTION_CALLBACK or\n+ * OPTION_LOWLEVEL_CALLBACK.\n *\n * `defval`::\n * default value to fill (*->value) with for PARSE_OPT_OPTARG.\n-- \n1.7.2.3\n"},{"id":"156878","messageId":"20101130030933.GE5326@burratino","threadId":"25483","inReplyTo":"20101130025223.GA5326@burratino","subject":"[PATCH 4/6] parse-options: make resuming easier after PARSE_OPT_STOP_AT_NON_OPTION","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-30T03:09:33Z","receivedAt":"2010-11-30T03:09:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Introduce a PARSE_OPT_NON_OPTION state, so parse_option_step()\ncallers can easily distinguish between non-options and other\nreasons for option parsing termination (like \"--\").\n\nSigned-off-by: Jonathan Nieder \n---\nAs before.\n\n parse-options.c | 3 ++-\n parse-options.h | 1 +\n 2 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 38c8fd3..df8299c 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -384,7 +384,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\tif (parse_nodash_opt(ctx, arg, options) == 0)\n \t\t\t\tcontinue;\n \t\t\tif (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)\n-\t\t\t\tbreak;\n+\t\t\t\treturn PARSE_OPT_NON_OPTION;\n \t\t\tctx->out[ctx->cpidx++] = ctx->argv[0];\n \t\t\tcontinue;\n \t\t}\n@@ -466,6 +466,7 @@ int parse_options(int argc, const char **argv, const char *prefix,\n \tswitch (parse_options_step(&ctx, options, usagestr)) {\n \tcase PARSE_OPT_HELP:\n \t\texit(129);\n+\tcase PARSE_OPT_NON_OPTION:\n \tcase PARSE_OPT_DONE:\n \t\tbreak;\n \tdefault: /* PARSE_OPT_UNKNOWN */\ndiff --git a/parse-options.h b/parse-options.h\nindex bd0fe16..2cd23af 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -167,6 +167,7 @@ extern NORETURN void usage_msg_opt(const char *msg,\n enum {\n \tPARSE_OPT_HELP = -1,\n \tPARSE_OPT_DONE,\n+\tPARSE_OPT_NON_OPTION,\n \tPARSE_OPT_UNKNOWN\n };\n \n-- \n1.7.2.3\n"},{"id":"156879","messageId":"20101130031048.GF5326@burratino","threadId":"25483","inReplyTo":"20101130025223.GA5326@burratino","subject":"[PATCH 5/6] setup: save prefix (original cwd relative to toplevel) in startup_info","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-30T03:10:48Z","receivedAt":"2010-11-30T03:10:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nSave the path from the original cwd to the cwd at the end of the\nsetup procedure in the startup_info struct introduced in e37c1329\n(2010-08-05). The value cannot vary from thread to thread anyway,\nsince the cwd is global.\n\nSo now in your builtin command, instead of passing prefix around,\nwhen you want to convert a user-supplied path to a cwd-relative\npath, you can use startup_info->prefix directly.\n\nCaveat: As with the return value from setup_git_directory_gently(),\nstartup_info->prefix would be NULL when the original cwd is not a\nsubdir of the toplevel.\n\nLonger term, this would allow the prefix to be reused when several\nnoncooperating functions require access to the same repository (for\nexample, when accessing configuration before running a builtin).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Jonathan Nieder \n---\nTypo in v1 log message fixed (s/wiould/would/).\n\n cache.h | 1 +\n setup.c | 4 +++-\n 2 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 33decd9..222d9cf 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1117,6 +1117,7 @@ const char *split_cmdline_strerror(int cmdline_errno);\n /* git.c */\n struct startup_info {\n \tint have_repository;\n+\tconst char *prefix;\n };\n extern struct startup_info *startup_info;\n \ndiff --git a/setup.c b/setup.c\nindex a3b76de..833db12 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -512,8 +512,10 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \tconst char *prefix;\n \n \tprefix = setup_git_directory_gently_1(nongit_ok);\n-\tif (startup_info)\n+\tif (startup_info) {\n \t\tstartup_info->have_repository = !nongit_ok || !*nongit_ok;\n+\t\tstartup_info->prefix = prefix;\n+\t}\n \treturn prefix;\n }\n \n-- \n1.7.2.3\n"},{"id":"156880","messageId":"20101130031549.GG5326@burratino","threadId":"25483","inReplyTo":"20101130025223.GA5326@burratino","subject":"[PATCH 6/6] update-index: migrate to parse-options API","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-30T03:15:49Z","receivedAt":"2010-11-30T03:15:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"--refresh and --really-refresh accept flags (like -q) and modify\nan error indicator. It might make sense to make the error\nindicator global, but just pass the flags and a pointer to the error\nindicator in a struct instead.\n\n--cacheinfo wants 3 arguments. Use the OPTION_LOWLEVEL_CALLBACK\nextension to grab them.\n\n--assume-unchanged and --no-assume-unchanged probably should use the\nOPT_UYN feature; but use a callback for now so the existing MARK_FLAG\nand UNMARK_FLAG values can be used.\n\n--stdin and --index-info are still constrained to be the last argument\n(implemented using the OPTION_LOWLEVEL_CALLBACK extension).\n\n--unresolve and --again consume all arguments that come after them\n(also using OPTION_LOWLEVEL_CALLBACK).\n\nThe order of options matters. Each path on the command line is\naffected only by the options that come before it. A custom\nargument-parsing loop with parse_options_step() brings that about.\n\nIn exchange for all the fuss, we get the usual perks: support for\nun-sticked options, better usage error messages, more useful -h\noutput, and argument parsing code that should be easier to tweak\nin the future.\n\nSigned-off-by: Jonathan Nieder \n---\nLots of clarity improvements:\n - simpler usage message\n - callback names end in _callback instead of _cb\n - redundant argument checking is gone\n - some strategic use of vertical whitespace\n - error messages more consistent with opterror() output\n - wording tweaks in -h output\n - removed debug noise\n\nThat's it. Thanks to Stephen for many useful comments that made\nthis possible.\n\nGood night.\nJonathan\n\n builtin/update-index.c | 391 ++++++++++++++++++++++++++++++------------------\n 1 files changed, 242 insertions(+), 149 deletions(-)\n\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 62d9f3f..0f19891 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -10,6 +10,7 @@\n #include \"builtin.h\"\n #include \"refs.h\"\n #include \"resolve-undo.h\"\n+#include \"parse-options.h\"\n \n /*\n * Default to not allowing changes to the list of files. The\n@@ -397,8 +398,10 @@ static void read_index_info(int line_termination)\n \tstrbuf_release(&uq);\n }\n \n-static const char update_index_usage[] =\n-\"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] [--] [...]\";\n+static const char * const update_index_usage[] = {\n+\t\"git update-index [options] [--] [...]\",\n+\tNULL\n+};\n \n static unsigned char head_sha1[20];\n static unsigned char merge_head_sha1[20];\n@@ -578,16 +581,210 @@ static int do_reupdate(int ac, const char **av,\n \treturn 0;\n }\n \n+struct refresh_params {\n+\tunsigned int flags;\n+\tint *has_errors;\n+};\n+\n+static int refresh(struct refresh_params *o, unsigned int flag)\n+{\n+\tsetup_work_tree();\n+\t*o->has_errors |= refresh_cache(o->flags | flag);\n+\treturn 0;\n+}\n+\n+static int refresh_callback(const struct option *opt,\n+\t\t\t\tconst char *arg, int unset)\n+{\n+\treturn refresh(opt->value, 0);\n+}\n+\n+static int really_refresh_callback(const struct option *opt,\n+\t\t\t\tconst char *arg, int unset)\n+{\n+\treturn refresh(opt->value, REFRESH_REALLY);\n+}\n+\n+static int chmod_callback(const struct option *opt,\n+\t\t\t\tconst char *arg, int unset)\n+{\n+\tchar *flip = opt->value;\n+\tif ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2])\n+\t\treturn error(\"option 'chmod' expects \\\"+x\\\" or \\\"-x\\\"\");\n+\t*flip = arg[0];\n+\treturn 0;\n+}\n+\n+static int resolve_undo_clear_callback(const struct option *opt,\n+\t\t\t\tconst char *arg, int unset)\n+{\n+\tresolve_undo_clear();\n+\treturn 0;\n+}\n+\n+static int cacheinfo_callback(struct parse_opt_ctx_t *ctx,\n+\t\t\t\tconst struct option *opt, int unset)\n+{\n+\tunsigned char sha1[20];\n+\tunsigned int mode;\n+\n+\tif (ctx->argc <= 3)\n+\t\treturn error(\"option 'cacheinfo' expects three arguments\");\n+\tif (strtoul_ui(*++ctx->argv, 8, &mode) ||\n+\t get_sha1_hex(*++ctx->argv, sha1) ||\n+\t add_cacheinfo(mode, sha1, *++ctx->argv, 0))\n+\t\tdie(\"git update-index: --cacheinfo cannot add %s\", *ctx->argv);\n+\tctx->argc -= 3;\n+\treturn 0;\n+}\n+\n+static int stdin_cacheinfo_callback(struct parse_opt_ctx_t *ctx,\n+\t\t\t const struct option *opt, int unset)\n+{\n+\tint *line_termination = opt->value;\n+\n+\tif (ctx->argc != 1)\n+\t\treturn error(\"option '%s' must be the last argument\", opt->long_name);\n+\tallow_add = allow_replace = allow_remove = 1;\n+\tread_index_info(*line_termination);\n+\treturn 0;\n+}\n+\n+static int stdin_callback(struct parse_opt_ctx_t *ctx,\n+\t\t\t\tconst struct option *opt, int unset)\n+{\n+\tint *read_from_stdin = opt->value;\n+\n+\tif (ctx->argc != 1)\n+\t\treturn error(\"option '%s' must be the last argument\", opt->long_name);\n+\t*read_from_stdin = 1;\n+\treturn 0;\n+}\n+\n+static int unresolve_callback(struct parse_opt_ctx_t *ctx,\n+\t\t\t\tconst struct option *opt, int flags)\n+{\n+\tint *has_errors = opt->value;\n+\tconst char *prefix = startup_info->prefix;\n+\n+\t/* consume remaining arguments. */\n+\t*has_errors = do_unresolve(ctx->argc, ctx->argv,\n+\t\t\t\tprefix, prefix ? strlen(prefix) : 0);\n+\tif (*has_errors)\n+\t\tactive_cache_changed = 0;\n+\n+\tctx->argv += ctx->argc - 1;\n+\tctx->argc = 1;\n+\treturn 0;\n+}\n+\n+static int reupdate_callback(struct parse_opt_ctx_t *ctx,\n+\t\t\t\tconst struct option *opt, int flags)\n+{\n+\tint *has_errors = opt->value;\n+\tconst char *prefix = startup_info->prefix;\n+\n+\t/* consume remaining arguments. */\n+\tsetup_work_tree();\n+\t*has_errors = do_reupdate(ctx->argc, ctx->argv,\n+\t\t\t\tprefix, prefix ? strlen(prefix) : 0);\n+\tif (*has_errors)\n+\t\tactive_cache_changed = 0;\n+\n+\tctx->argv += ctx->argc - 1;\n+\tctx->argc = 1;\n+\treturn 0;\n+}\n+\n int cmd_update_index(int argc, const char **argv, const char *prefix)\n {\n-\tint i, newfd, entries, has_errors = 0, line_termination = '\\n';\n-\tint allow_options = 1;\n+\tint newfd, entries, has_errors = 0, line_termination = '\\n';\n \tint read_from_stdin = 0;\n \tint prefix_length = prefix ? strlen(prefix) : 0;\n \tchar set_executable_bit = 0;\n-\tunsigned int refresh_flags = 0;\n+\tstruct refresh_params refresh_args = {0, &has_errors};\n \tint lock_error = 0;\n \tstruct lock_file *lock_file;\n+\tstruct parse_opt_ctx_t ctx;\n+\tint parseopt_state = PARSE_OPT_UNKNOWN;\n+\tstruct option options[] = {\n+\t\tOPT_BIT('q', NULL, &refresh_args.flags,\n+\t\t\t\"continue refresh even when index needs update\",\n+\t\t\tREFRESH_QUIET),\n+\t\tOPT_BIT(0, \"ignore-submodules\", &refresh_args.flags,\n+\t\t\t\"refresh: ignore submodules\",\n+\t\t\tREFRESH_IGNORE_SUBMODULES),\n+\t\tOPT_SET_INT(0, \"add\", &allow_add,\n+\t\t\t\"do not ignore new files\", 1),\n+\t\tOPT_SET_INT(0, \"replace\", &allow_replace,\n+\t\t\t\"let files replace directories and vice-versa\", 1),\n+\t\tOPT_SET_INT(0, \"remove\", &allow_remove,\n+\t\t\t\"notice files missing from worktree\", 1),\n+\t\tOPT_BIT(0, \"unmerged\", &refresh_args.flags,\n+\t\t\t\"refresh even if index contains unmerged entries\",\n+\t\t\tREFRESH_UNMERGED),\n+\t\t{OPTION_CALLBACK, 0, \"refresh\", &refresh_args, NULL,\n+\t\t\t\"refresh stat information\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG,\n+\t\t\trefresh_callback},\n+\t\t{OPTION_CALLBACK, 0, \"really-refresh\", &refresh_args, NULL,\n+\t\t\t\"like --refresh, but ignore assume-unchanged setting\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG,\n+\t\t\treally_refresh_callback},\n+\t\t{OPTION_LOWLEVEL_CALLBACK, 0, \"cacheinfo\", NULL,\n+\t\t\t\" \",\n+\t\t\t\"add the specified entry to the index\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n+\t\t\t(parse_opt_cb *) cacheinfo_callback},\n+\t\t{OPTION_CALLBACK, 0, \"chmod\", &set_executable_bit, \"(+/-)x\",\n+\t\t\t\"override the executable bit of the listed files\",\n+\t\t\tPARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n+\t\t\tchmod_callback},\n+\t\t{OPTION_SET_INT, 0, \"assume-unchanged\", &mark_valid_only, NULL,\n+\t\t\t\"mark files as \\\"not changing\\\"\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},\n+\t\t{OPTION_SET_INT, 0, \"no-assume-unchanged\", &mark_valid_only, NULL,\n+\t\t\t\"clear assumed-unchanged bit\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG},\n+\t\t{OPTION_SET_INT, 0, \"skip-worktree\", &mark_skip_worktree_only, NULL,\n+\t\t\t\"mark files as \\\"index-only\\\"\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},\n+\t\t{OPTION_SET_INT, 0, \"no-skip-worktree\", &mark_skip_worktree_only, NULL,\n+\t\t\t\"clear skip-worktree bit\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG},\n+\t\tOPT_SET_INT(0, \"info-only\", &info_only,\n+\t\t\t\"add to index only; do not add content to object database\", 1),\n+\t\tOPT_SET_INT(0, \"force-remove\", &force_remove,\n+\t\t\t\"remove named paths even if present in worktree\", 1),\n+\t\tOPT_SET_INT('z', NULL, &line_termination,\n+\t\t\t\"with --stdin: input lines are terminated by null bytes\", '\\0'),\n+\t\t{OPTION_LOWLEVEL_CALLBACK, 0, \"stdin\", &read_from_stdin, NULL,\n+\t\t\t\"read list of paths to be updated from standard input\",\n+\t\t\tPARSE_OPT_NONEG | PARSE_OPT_NOARG,\n+\t\t\t(parse_opt_cb *) stdin_callback},\n+\t\t{OPTION_LOWLEVEL_CALLBACK, 0, \"index-info\", &line_termination, NULL,\n+\t\t\t\"add entries from standard input to the index\",\n+\t\t\tPARSE_OPT_NONEG | PARSE_OPT_NOARG,\n+\t\t\t(parse_opt_cb *) stdin_cacheinfo_callback},\n+\t\t{OPTION_LOWLEVEL_CALLBACK, 0, \"unresolve\", &has_errors, NULL,\n+\t\t\t\"repopulate stages #2 and #3 for the listed paths\",\n+\t\t\tPARSE_OPT_NONEG | PARSE_OPT_NOARG,\n+\t\t\t(parse_opt_cb *) unresolve_callback},\n+\t\t{OPTION_LOWLEVEL_CALLBACK, 'g', \"again\", &has_errors, NULL,\n+\t\t\t\"only update entries that differ from HEAD\",\n+\t\t\tPARSE_OPT_NONEG | PARSE_OPT_NOARG,\n+\t\t\t(parse_opt_cb *) reupdate_callback},\n+\t\tOPT_BIT(0, \"ignore-missing\", &refresh_args.flags,\n+\t\t\t\"ignore files missing from worktree\",\n+\t\t\tREFRESH_IGNORE_MISSING),\n+\t\tOPT_SET_INT(0, \"verbose\", &verbose,\n+\t\t\t\"report actions to standard output\", 1),\n+\t\t{OPTION_CALLBACK, 0, \"clear-resolve-undo\", NULL, NULL,\n+\t\t\t\"(for porcelains) forget saved unresolved conflicts\",\n+\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG,\n+\t\t\tresolve_undo_clear_callback},\n+\t\tOPT_END()\n+\t};\n \n \tgit_config(git_default_config, NULL);\n \n@@ -602,151 +799,48 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \tif (entries < 0)\n \t\tdie(\"cache corrupted\");\n \n-\tfor (i = 1 ; i < argc; i++) {\n-\t\tconst char *path = argv[i];\n-\t\tconst char *p;\n+\t/*\n+\t * Custom copy of parse_options() because we want to handle\n+\t * filename arguments as they come.\n+\t */\n+\tparse_options_start(&ctx, argc, argv, prefix,\n+\t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n+\twhile (ctx.argc) {\n+\t\tif (parseopt_state != PARSE_OPT_DONE)\n+\t\t\tparseopt_state = parse_options_step(&ctx, options,\n+\t\t\t\t\t\t\t update_index_usage);\n+\t\tif (!ctx.argc)\n+\t\t\tbreak;\n+\t\tswitch (parseopt_state) {\n+\t\tcase PARSE_OPT_HELP:\n+\t\t\texit(129);\n+\t\tcase PARSE_OPT_NON_OPTION:\n+\t\tcase PARSE_OPT_DONE:\n+\t\t{\n+\t\t\tconst char *path = ctx.argv[0];\n+\t\t\tconst char *p;\n \n-\t\tif (allow_options && *path == '-') {\n-\t\t\tif (!strcmp(path, \"--\")) {\n-\t\t\t\tallow_options = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"-q\")) {\n-\t\t\t\trefresh_flags |= REFRESH_QUIET;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--ignore-submodules\")) {\n-\t\t\t\trefresh_flags |= REFRESH_IGNORE_SUBMODULES;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--add\")) {\n-\t\t\t\tallow_add = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--replace\")) {\n-\t\t\t\tallow_replace = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--remove\")) {\n-\t\t\t\tallow_remove = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--unmerged\")) {\n-\t\t\t\trefresh_flags |= REFRESH_UNMERGED;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--refresh\")) {\n-\t\t\t\tsetup_work_tree();\n-\t\t\t\thas_errors |= refresh_cache(refresh_flags);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--really-refresh\")) {\n-\t\t\t\tsetup_work_tree();\n-\t\t\t\thas_errors |= refresh_cache(REFRESH_REALLY | refresh_flags);\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--cacheinfo\")) {\n-\t\t\t\tunsigned char sha1[20];\n-\t\t\t\tunsigned int mode;\n-\n-\t\t\t\tif (i+3 >= argc)\n-\t\t\t\t\tdie(\"git update-index: --cacheinfo \");\n-\n-\t\t\t\tif (strtoul_ui(argv[i+1], 8, &mode) ||\n-\t\t\t\t get_sha1_hex(argv[i+2], sha1) ||\n-\t\t\t\t add_cacheinfo(mode, sha1, argv[i+3], 0))\n-\t\t\t\t\tdie(\"git update-index: --cacheinfo\"\n-\t\t\t\t\t \" cannot add %s\", argv[i+3]);\n-\t\t\t\ti += 3;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--chmod=-x\") ||\n-\t\t\t !strcmp(path, \"--chmod=+x\")) {\n-\t\t\t\tif (argc <= i+1)\n-\t\t\t\t\tdie(\"git update-index: %s \", path);\n-\t\t\t\tset_executable_bit = path[8];\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--assume-unchanged\")) {\n-\t\t\t\tmark_valid_only = MARK_FLAG;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--no-assume-unchanged\")) {\n-\t\t\t\tmark_valid_only = UNMARK_FLAG;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--no-skip-worktree\")) {\n-\t\t\t\tmark_skip_worktree_only = UNMARK_FLAG;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--skip-worktree\")) {\n-\t\t\t\tmark_skip_worktree_only = MARK_FLAG;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--info-only\")) {\n-\t\t\t\tinfo_only = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--force-remove\")) {\n-\t\t\t\tforce_remove = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"-z\")) {\n-\t\t\t\tline_termination = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--stdin\")) {\n-\t\t\t\tif (i != argc - 1)\n-\t\t\t\t\tdie(\"--stdin must be at the end\");\n-\t\t\t\tread_from_stdin = 1;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--index-info\")) {\n-\t\t\t\tif (i != argc - 1)\n-\t\t\t\t\tdie(\"--index-info must be at the end\");\n-\t\t\t\tallow_add = allow_replace = allow_remove = 1;\n-\t\t\t\tread_index_info(line_termination);\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--unresolve\")) {\n-\t\t\t\thas_errors = do_unresolve(argc - i, argv + i,\n-\t\t\t\t\t\t\t prefix, prefix_length);\n-\t\t\t\tif (has_errors)\n-\t\t\t\t\tactive_cache_changed = 0;\n-\t\t\t\tgoto finish;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--again\") || !strcmp(path, \"-g\")) {\n-\t\t\t\tsetup_work_tree();\n-\t\t\t\thas_errors = do_reupdate(argc - i, argv + i,\n-\t\t\t\t\t\t\t prefix, prefix_length);\n-\t\t\t\tif (has_errors)\n-\t\t\t\t\tactive_cache_changed = 0;\n-\t\t\t\tgoto finish;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--ignore-missing\")) {\n-\t\t\t\trefresh_flags |= REFRESH_IGNORE_MISSING;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--verbose\")) {\n-\t\t\t\tverbose = 1;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"--clear-resolve-undo\")) {\n-\t\t\t\tresolve_undo_clear();\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (!strcmp(path, \"-h\") || !strcmp(path, \"--help\"))\n-\t\t\t\tusage(update_index_usage);\n-\t\t\tdie(\"unknown option %s\", path);\n+\t\t\tsetup_work_tree();\n+\t\t\tp = prefix_path(prefix, prefix_length, path);\n+\t\t\tupdate_one(p, NULL, 0);\n+\t\t\tif (set_executable_bit)\n+\t\t\t\tchmod_path(set_executable_bit, p);\n+\t\t\tif (p < path || p > path + strlen(path))\n+\t\t\t\tfree((char *)p);\n+\t\t\tctx.argc--;\n+\t\t\tctx.argv++;\n+\t\t\tbreak;\n+\t\t}\n+\t\tcase PARSE_OPT_UNKNOWN:\n+\t\t\tif (ctx.argv[0][1] == '-')\n+\t\t\t\terror(\"unknown option '%s'\", ctx.argv[0] + 2);\n+\t\t\telse\n+\t\t\t\terror(\"unknown switch '%c'\", *ctx.opt);\n+\t\t\tusage_with_options(update_index_usage, options);\n \t\t}\n-\t\tsetup_work_tree();\n-\t\tp = prefix_path(prefix, prefix_length, path);\n-\t\tupdate_one(p, NULL, 0);\n-\t\tif (set_executable_bit)\n-\t\t\tchmod_path(set_executable_bit, p);\n-\t\tif (p < path || p > path + strlen(path))\n-\t\t\tfree((char *)p);\n \t}\n+\targc = parse_options_end(&ctx);\n+\n \tif (read_from_stdin) {\n \t\tstruct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;\n \n@@ -770,10 +864,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\tstrbuf_release(&buf);\n \t}\n \n- finish:\n \tif (active_cache_changed) {\n \t\tif (newfd < 0) {\n-\t\t\tif (refresh_flags & REFRESH_QUIET)\n+\t\t\tif (refresh_args.flags & REFRESH_QUIET)\n \t\t\t\texit(128);\n \t\t\tunable_to_lock_index_die(get_index_file(), lock_error);\n \t\t}\n-- \n1.7.2.3\n"},{"id":"156887","messageId":"4CF4B21B.5030401@gmail.com","threadId":"25483","inReplyTo":"20101130025551.GB5326@burratino","subject":"Re: [PATCH 1/6] parse-options: sanity check PARSE_OPT_NOARG flag","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2010-11-30T08:13:15Z","receivedAt":"2010-11-30T08:13:15Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"On 11/29/10 18:55, Jonathan Nieder wrote:\n> +static void check_flags(const struct option *opt)\n> +{\n> +\tswitch (opt->type) {\n> +\tcase OPTION_BOOLEAN:\n> +\tcase OPTION_BIT:\n> +\tcase OPTION_NEGBIT:\n> +\tcase OPTION_SET_INT:\n> +\tcase OPTION_SET_PTR:\n> +\tcase OPTION_NUMBER:\n> +\t\tbreak;\n> +\tdefault:\t/* (usually accepts an argument) */\n> +\t\treturn;\n> +\t}\n> +\tif ((opt->flags & (PARSE_OPT_OPTARG | PARSE_OPT_NOARG)) == PARSE_OPT_NOARG)\n> +\t\treturn;\n> +\tdie(\"BUG: option '-%c%s' should not accept an argument\",\n> +\t\t\t\t!opt->short_name ? '-' : opt->short_name,\n> +\t\t\t\t!opt->short_name ? opt->long_name : \"\");\n> +}\n> +\n\nThis check should probably go into parse_options_check() and be run once\nfor each invocation of parse_options_start()... Oh that isn't good.\n\nLooks like parse_options_check() is being called for each\nparse_options_step(). Here's a patch to fix that. Junio, this can\nprobably be applied to maint.\n\n---8<------>8-----\nSubject: [PATCH] parse-options: Don't call parse_options_check() so much\n\nparse_options_check() is being called for each invocation of\nparse_options_step() which can be quite a bit for some commands. The\ncommit introducing this function cb9d398 (parse-options: add\nparse_options_check to validate option specs., 2009-06-09) had the\ncorrect motivation and explicitly states that parse_options_check()\nshould be called from parse_options_start(). However, the implementation\ndiffers from the motivation. Fix it.\n\nSigned-off-by: Stephen Boyd \n---\n builtin/blame.c | 2 +-\n builtin/shortlog.c | 2 +-\n parse-options.c | 7 +++----\n parse-options.h | 2 +-\n 4 files changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex f5fccc1..19a2ebf 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2310,7 +2310,7 @@ int cmd_blame(int argc, const char **argv, const\nchar *prefix)\n \tdashdash_pos = 0;\n\n \tparse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |\n-\t\t\t PARSE_OPT_KEEP_ARGV0);\n+\t\t\t PARSE_OPT_KEEP_ARGV0, options);\n \tfor (;;) {\n \t\tswitch (parse_options_step(&ctx, options, blame_opt_usage)) {\n \t\tcase PARSE_OPT_HELP:\ndiff --git a/builtin/shortlog.c b/builtin/shortlog.c\nindex 2135b0d..8473391 100644\n--- a/builtin/shortlog.c\n+++ b/builtin/shortlog.c\n@@ -269,7 +269,7 @@ int cmd_shortlog(int argc, const char **argv, const\nchar *prefix)\n \tshortlog_init(&log);\n \tinit_revisions(&rev, prefix);\n \tparse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |\n-\t\t\t PARSE_OPT_KEEP_ARGV0);\n+\t\t\t PARSE_OPT_KEEP_ARGV0, options);\n\n \tfor (;;) {\n \t\tswitch (parse_options_step(&ctx, options, shortlog_usage)) {\ndiff --git a/parse-options.c b/parse-options.c\nindex 0fa79bc..196ba71 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -338,7 +338,7 @@ static void parse_options_check(const struct option\n*opts)\n\n void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t int argc, const char **argv, const char *prefix,\n-\t\t\t int flags)\n+\t\t\t int flags, const struct option *options)\n {\n \tmemset(ctx, 0, sizeof(*ctx));\n \tctx->argc = argc - 1;\n@@ -350,6 +350,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n \tif ((flags & PARSE_OPT_KEEP_UNKNOWN) &&\n \t (flags & PARSE_OPT_STOP_AT_NON_OPTION))\n \t\tdie(\"STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together\");\n+\tparse_options_check(options);\n }\n\n static int usage_with_options_internal(struct parse_opt_ctx_t *,\n@@ -362,8 +363,6 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n {\n \tint internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);\n\n-\tparse_options_check(options);\n-\n \t/* we must reset ->opt, unknown short option leave it dangling */\n \tctx->opt = NULL;\n\n@@ -452,7 +451,7 @@ int parse_options(int argc, const char **argv, const\nchar *prefix,\n {\n \tstruct parse_opt_ctx_t ctx;\n\n-\tparse_options_start(&ctx, argc, argv, prefix, flags);\n+\tparse_options_start(&ctx, argc, argv, prefix, flags, options);\n \tswitch (parse_options_step(&ctx, options, usagestr)) {\n \tcase PARSE_OPT_HELP:\n \t\texit(129);\ndiff --git a/parse-options.h b/parse-options.h\nindex ae8647d..828c056 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -180,7 +180,7 @@ struct parse_opt_ctx_t {\n\n extern void parse_options_start(struct parse_opt_ctx_t *ctx,\n \t\t\t\tint argc, const char **argv, const char *prefix,\n-\t\t\t\tint flags);\n+\t\t\t\tint flags, const struct option *options);\n\n extern int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\t const struct option *options,\n-- \n1.7.3.2.614.g03864\n"},{"id":"156889","messageId":"4CF4B22B.4060307@gmail.com","threadId":"25483","inReplyTo":"20101130030441.GC5326@burratino","subject":"Re: [PATCH 2/6] parse-options: do not infer PARSE_OPT_NOARG from option type","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2010-11-30T08:13:31Z","receivedAt":"2010-11-30T08:13:31Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"On 11/29/10 19:04, Jonathan Nieder wrote:\n> From: Stephen Boyd \n> \n> Simplify the \"takes no value\" error path by relying on PARSE_OPT_NOARG\n> being set correctly. That is:\n> \n> - if the PARSE_OPT_NOARG flag is set, reject --opt=value\n> regardless of the option type;\n> - if the PARSE_OPT_NOARG flag is unset, accept --opt=value\n> regardless of the option type.\n> \n> This way, the accepted usage more closely matches the usage advertised\n> with --help-all.\n> \n> No functional change intended, since the NOARG flag is only used\n> with \"boolean-only\" option types in existing parse_options callers.\n> \n\nSigned-off-by: Stephen Boyd \n\n> Signed-off-by: Jonathan Nieder \n> ---\n\nThanks for the nice words.\n"},{"id":"156888","messageId":"4CF4B234.9060206@gmail.com","threadId":"25483","inReplyTo":"20101130031549.GG5326@burratino","subject":"Re: [PATCH 6/6] update-index: migrate to parse-options API","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2010-11-30T08:13:40Z","receivedAt":"2010-11-30T08:13:40Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"On 11/29/10 19:15, Jonathan Nieder wrote:\n> +\t\t{OPTION_LOWLEVEL_CALLBACK, 0, \"cacheinfo\", NULL,\n> +\t\t\t\" \",\n> +\t\t\t\"add the specified entry to the index\",\n> +\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n> +\t\t\t(parse_opt_cb *) cacheinfo_callback},\n\nDoesn't this take arguments and thus shouldn't be marked\nPARSE_OPT_NOARG? Confused.\n\n> @@ -602,151 +799,48 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n> \tif (entries < 0)\n> \t\tdie(\"cache corrupted\");\n> \n> -\tfor (i = 1 ; i < argc; i++) {\n> -\t\tconst char *path = argv[i];\n> -\t\tconst char *p;\n> +\t/*\n> +\t * Custom copy of parse_options() because we want to handle\n> +\t * filename arguments as they come.\n> +\t */\n> +\tparse_options_start(&ctx, argc, argv, prefix,\n> +\t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n\nThis will need to take options too, sorry.\n"},{"id":"156896","messageId":"20101130160059.GA31844@burratino","threadId":"25483","inReplyTo":"4CF4B234.9060206@gmail.com","subject":"[PATCH] parse-options: always show arghelp when LITERAL_ARGHELP is set","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-11-30T16:00:59Z","receivedAt":"2010-11-30T16:00:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The PARSE_OPT_LITERAL_ARGHELP flag allows a program to override the\nusual \" for mandatory, [argument] for optional\" markup\nin its help message. Extend it by allowing the usual \"no text for\ndisallowed\" to be overridden, too (for options with PARSE_OPT_NOARG |\nPARSE_OPT_LITERAL_ARGHELP, which was previously an unsupported\ncombination).\n\nSo now a person can impose ugly usage messages like\n\n\t--refresh [--] ...\n\t don't add, only refresh the index\n\nbut more importantly, update-index can correctly advertise\n\n\t--cacheinfo \n\t add the specified entry to the index\n\nwithout unsetting PARSE_OPT_NOARG and making that '--cacheinfo='\n'' ''.\n\nNoticed-by: Stephen Boyd \nSigned-off-by: Jonathan Nieder \n---\nStephen Boyd wrote:\n> On 11/29/10 19:15, Jonathan Nieder wrote:\n>> +\t\t{OPTION_LOWLEVEL_CALLBACK, 0, \"cacheinfo\", NULL,\n>> +\t\t\t\" \",\n>> +\t\t\t\"add the specified entry to the index\",\n>> +\t\t\tPARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n>> +\t\t\t(parse_opt_cb *) cacheinfo_callback},\n>\n> Doesn't this take arguments and thus shouldn't be marked\n> PARSE_OPT_NOARG? Confused.\n\nYes, that deserves a comment.\n\n\t\t\tPARSE_OPT_NOARG |\t/* disallow sticky --cacheinfo= form */\n\t\t\tPARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,\n\n>> @@ -602,151 +799,48 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n[...]\n>> +\tparse_options_start(&ctx, argc, argv, prefix,\n>> +\t\t\t PARSE_OPT_STOP_AT_NON_OPTION);\n>\n> This will need to take options too, sorry.\n\nThat would just be a merge artifact, no? :) Thanks for a pointer.\n\nIf all goes well, I'll reroll the series with your patch later today.\n\n parse-options.c | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex df8299c..71ebd9e 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -552,7 +552,8 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \t\tif (opts->type == OPTION_NUMBER)\n \t\t\tpos += fprintf(outfile, \"-NUM\");\n \n-\t\tif (!(opts->flags & PARSE_OPT_NOARG))\n+\t\tif ((opts->flags & PARSE_OPT_LITERAL_ARGHELP) ||\n+\t\t !(opts->flags & PARSE_OPT_NOARG))\n \t\t\tpos += usage_argh(opts, outfile);\n \n \t\tif (pos <= USAGE_OPTS_WIDTH)\n-- \n1.7.2.3\n"},{"id":"157045","messageId":"7v7hftt7vy.fsf@alter.siamese.dyndns.org","threadId":"25483","inReplyTo":"4CF4B21B.5030401@gmail.com","subject":"Re: [PATCH 1/6] parse-options: sanity check PARSE_OPT_NOARG flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-01T23:01:37Z","receivedAt":"2010-12-01T23:01:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Boyd writes:\n\n> Looks like parse_options_check() is being called for each\n> parse_options_step(). Here's a patch to fix that. Junio, this can\n> probably be applied to maint.\n\nLooks correct, but why is it a maint material?\n\n> ---8<------>8-----\n> Subject: [PATCH] parse-options: Don't call parse_options_check() so much\n>\n> parse_options_check() is being called for each invocation of\n> parse_options_step() which can be quite a bit for some commands. The\n> commit introducing this function cb9d398 (parse-options: add\n> parse_options_check to validate option specs., 2009-06-09) had the\n> correct motivation and explicitly states that parse_options_check()\n> should be called from parse_options_start(). However, the implementation\n> differs from the motivation. Fix it.\n>\n> Signed-off-by: Stephen Boyd \n> ---\n> builtin/blame.c | 2 +-\n> builtin/shortlog.c | 2 +-\n> parse-options.c | 7 +++----\n> parse-options.h | 2 +-\n> 4 files changed, 6 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index f5fccc1..19a2ebf 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2310,7 +2310,7 @@ int cmd_blame(int argc, const char **argv, const\n> char *prefix)\n> \tdashdash_pos = 0;\n>\n> \tparse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |\n> -\t\t\t PARSE_OPT_KEEP_ARGV0);\n> +\t\t\t PARSE_OPT_KEEP_ARGV0, options);\n> \tfor (;;) {\n> \t\tswitch (parse_options_step(&ctx, options, blame_opt_usage)) {\n> \t\tcase PARSE_OPT_HELP:\n> diff --git a/builtin/shortlog.c b/builtin/shortlog.c\n> index 2135b0d..8473391 100644\n> --- a/builtin/shortlog.c\n> +++ b/builtin/shortlog.c\n> @@ -269,7 +269,7 @@ int cmd_shortlog(int argc, const char **argv, const\n> char *prefix)\n> \tshortlog_init(&log);\n> \tinit_revisions(&rev, prefix);\n> \tparse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |\n> -\t\t\t PARSE_OPT_KEEP_ARGV0);\n> +\t\t\t PARSE_OPT_KEEP_ARGV0, options);\n>\n> \tfor (;;) {\n> \t\tswitch (parse_options_step(&ctx, options, shortlog_usage)) {\n> diff --git a/parse-options.c b/parse-options.c\n> index 0fa79bc..196ba71 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -338,7 +338,7 @@ static void parse_options_check(const struct option\n> *opts)\n>\n> void parse_options_start(struct parse_opt_ctx_t *ctx,\n> \t\t\t int argc, const char **argv, const char *prefix,\n> -\t\t\t int flags)\n> +\t\t\t int flags, const struct option *options)\n> {\n> \tmemset(ctx, 0, sizeof(*ctx));\n> \tctx->argc = argc - 1;\n> @@ -350,6 +350,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,\n> \tif ((flags & PARSE_OPT_KEEP_UNKNOWN) &&\n> \t (flags & PARSE_OPT_STOP_AT_NON_OPTION))\n> \t\tdie(\"STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together\");\n> +\tparse_options_check(options);\n> }\n>\n> static int usage_with_options_internal(struct parse_opt_ctx_t *,\n> @@ -362,8 +363,6 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n> {\n> \tint internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);\n>\n> -\tparse_options_check(options);\n> -\n> \t/* we must reset ->opt, unknown short option leave it dangling */\n> \tctx->opt = NULL;\n>\n> @@ -452,7 +451,7 @@ int parse_options(int argc, const char **argv, const\n> char *prefix,\n> {\n> \tstruct parse_opt_ctx_t ctx;\n>\n> -\tparse_options_start(&ctx, argc, argv, prefix, flags);\n> +\tparse_options_start(&ctx, argc, argv, prefix, flags, options);\n> \tswitch (parse_options_step(&ctx, options, usagestr)) {\n> \tcase PARSE_OPT_HELP:\n> \t\texit(129);\n> diff --git a/parse-options.h b/parse-options.h\n> index ae8647d..828c056 100644\n> --- a/parse-options.h\n> +++ b/parse-options.h\n> @@ -180,7 +180,7 @@ struct parse_opt_ctx_t {\n>\n> extern void parse_options_start(struct parse_opt_ctx_t *ctx,\n> \t\t\t\tint argc, const char **argv, const char *prefix,\n> -\t\t\t\tint flags);\n> +\t\t\t\tint flags, const struct option *options);\n>\n> extern int parse_options_step(struct parse_opt_ctx_t *ctx,\n> \t\t\t const struct option *options,\n> -- \n> 1.7.3.2.614.g03864\n"},{"id":"157059","messageId":"7v39qht698.fsf@alter.siamese.dyndns.org","threadId":"25483","inReplyTo":"4CF4B21B.5030401@gmail.com","subject":"Re: [PATCH 1/6] parse-options: sanity check PARSE_OPT_NOARG flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-12-01T23:36:51Z","receivedAt":"2010-12-01T23:36:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stephen Boyd writes:\n\n> On 11/29/10 18:55, Jonathan Nieder wrote:\n>> +static void check_flags(const struct option *opt)\n>> +{\n>> +\tswitch (opt->type) {\n>> +\tcase OPTION_BOOLEAN:\n>> +\tcase OPTION_BIT:\n>> +\tcase OPTION_NEGBIT:\n>> +\tcase OPTION_SET_INT:\n>> +\tcase OPTION_SET_PTR:\n>> +\tcase OPTION_NUMBER:\n>> +\t\tbreak;\n>> +\tdefault:\t/* (usually accepts an argument) */\n>> +\t\treturn;\n>> +\t}\n>> +\tif ((opt->flags & (PARSE_OPT_OPTARG | PARSE_OPT_NOARG)) == PARSE_OPT_NOARG)\n>> +\t\treturn;\n>> +\tdie(\"BUG: option '-%c%s' should not accept an argument\",\n>> +\t\t\t\t!opt->short_name ? '-' : opt->short_name,\n>> +\t\t\t\t!opt->short_name ? opt->long_name : \"\");\n>> +}\n>> +\n>\n> This check should probably go into parse_options_check()...\n\nVery good suggestion---that way we can check and get diagnosis for all the\nerrors, not just dying on the first one.\n"},{"id":"157173","messageId":"4CF89DC7.2050806@gmail.com","threadId":"25483","inReplyTo":"7v7hftt7vy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/6] parse-options: sanity check PARSE_OPT_NOARG flag","fromName":"Stephen Boyd","fromEmail":"bebarino@gmail.com","sentAt":"2010-12-03T07:35:35Z","receivedAt":"2010-12-03T07:35:35Z","isPatch":true,"sender":{"key":"bebarino@gmail.com","avatar":"https://avatars.githubusercontent.com/u/38832?v=4"},"body":"On 12/01/10 15:01, Junio C Hamano wrote:\n> Stephen Boyd writes:\n> \n>> Looks like parse_options_check() is being called for each\n>> parse_options_step(). Here's a patch to fix that. Junio, this can\n>> probably be applied to maint.\n> \n> Looks correct, but why is it a maint material?\n> \n\nPossible performance regression? Which is why its qualified with probably.\n"}]}