{"thread":{"id":"25479","subject":"[PATCH 1/3] run_builtin(): save \"-h\" detection result for later use","startedAt":"2010-10-19T13:35:18Z","lastAt":"2010-10-20T01:18:22Z","messageCount":12,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Jonathan Nieder","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"153786","messageId":"1287495320-27278-1-git-send-email-pclouds@gmail.com","threadId":"25479","inReplyTo":null,"subject":"[PATCH 1/3] run_builtin(): save \"-h\" detection result for later use","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-10-19T13:35:18Z","receivedAt":"2010-10-19T13:35:18Z","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, which has yet to be set up.\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 need\nto do more initialization than git_config().\n\ngit_config() is a bit of a special case: it is perfectly legitimate to\nuse it without access to a valid repository. In the future, we should\nmake git_config() aware of the repository search status so that it can\nskip reading $GIT_DIR/config if no repository is found.\n---\n cache.h |    1 +\n git.c   |    8 ++++----\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 33decd9..43763c1 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; /* print help and exit, except git_config(), repo must not be touched */\n };\n extern struct startup_info *startup_info;\n \ndiff --git a/git.c b/git.c\nindex 50a1401..5962cdd 100644\n--- a/git.c\n+++ b/git.c\n@@ -246,13 +246,13 @@ 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\tif (p->option & RUN_SETUP)\n \t\t\tprefix = setup_git_directory();\n \t\tif (p->option & RUN_SETUP_GENTLY) {\n@@ -267,7 +267,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\");\n-- \n1.7.0.2.445.gcbdb3\n"},{"id":"153788","messageId":"1287495320-27278-2-git-send-email-pclouds@gmail.com","threadId":"25479","inReplyTo":"1287495320-27278-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/3] builtins: utilize startup_info->help where possible","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-10-19T13:35:19Z","receivedAt":"2010-10-19T13:35:19Z","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.\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 <pattern>', 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":"153787","messageId":"1287495320-27278-3-git-send-email-pclouds@gmail.com","threadId":"25479","inReplyTo":"1287495320-27278-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/3] 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-19T13:35:20Z","receivedAt":"2010-10-19T13:35:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"These commands need more than just git_config() before parsing\ncommmand line arguments. Some of these activities will unconditionally\nlook into a repository. When startup_info->help is TRUE, no repository\nis set up and the caller expects callees to print help usage and exit,\nno more.\n\nDo as expected.\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          |   20 ++++++++++++++++++++\n 9 files changed, 48 insertions(+), 3 deletions(-)\n create mode 100755 t/t3905-help.sh\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..f040171 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -191,6 +191,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \n \tgit_config(gc_config, NULL);\n \n+\tif (startup_info->help)\n+\t\tusage_with_options(builtin_gc_usage, builtin_gc_options);\n+\n \tif (pack_refs < 0)\n \t\tpack_refs = !is_bare_repository();\n \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\nnew file mode 100755\nindex 0000000..c7e80c5\n--- /dev/null\n+++ b/t/t3905-help.sh\n@@ -0,0 +1,20 @@\n+#!/bin/sh\n+\n+test_description='tests that git foo -h should work even in broken repos'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\tgit config core.repositoryformatversion 99 &&\n+\ttest_must_fail git rev-parse HEAD\n+'\n+\n+CMDS=\"branch checkout-index commit gc ls-files merge update-index upload-archive\"\n+for cmd in $CMDS; do\n+\ttest_expect_success \"$cmd -h\" \"\n+\t\ttest_must_fail git $cmd -h &&\n+\t\ttest \\$exit_code = 129\n+\t\"\n+done\n+\n+test_done\n-- \n1.7.0.2.445.gcbdb3\n"},{"id":"153795","messageId":"7vvd4y16t8.fsf@alter.siamese.dyndns.org","threadId":"25479","inReplyTo":"1287495320-27278-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 3/3] builtins: check for startup_info->help, print and exit early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-19T16:33:07Z","receivedAt":"2010-10-19T16:33:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n\n> These commands need more than just git_config() before parsing\n> commmand line arguments. Some of these activities will unconditionally\n> look into a repository. When startup_info->help is TRUE, no repository\n> is set up and the caller expects callees to print help usage and exit,\n> no more.\n>\n> Do as expected.\n> ---\n\nNo sign-off given to any of the three patches...\n\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 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\nI can just say:\n\n    $ cd / && git branch -h\n    usage: git branch [options] ...\n\nwithout your patch just fine (and no I am not insane to make / controlled\nby git).\n\nThe same for checkout-index, commit, ls-files, etc.\n\n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index c304638..f040171 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -191,6 +191,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>  \n>  \tgit_config(gc_config, NULL);\n>  \n> +\tif (startup_info->help)\n> +\t\tusage_with_options(builtin_gc_usage, builtin_gc_options);\n> +\n>  \tif (pack_refs < 0)\n>  \t\tpack_refs = !is_bare_repository();\n\nThis one is curious.  Why do you need a call to git_config() in \"gc\" only?\nYou don't do that for e.g. \"branch\".\n\nWhile I do not see anything glaringly wrong with this change, I am not\nsure I am getting the point of these patches.\n"},{"id":"153798","messageId":"20101019164826.GA25139@burratino","threadId":"25479","inReplyTo":"1287495320-27278-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/3] run_builtin(): save \"-h\" detection result for later use","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-19T16:48:26Z","receivedAt":"2010-10-19T16:48:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nNguyễn Thái Ngọc Duy wrote:\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()\n\nand even during parse_options() in some weird cases.\n\nTaking this patch as a cover letter, I wonder about the impact\n(because I forgot).  Is this to avoid, e.g., erroring out from a\nrepository with invalid HEAD when the user just wanted to get help?\nExample commands with improved behavior?  It would be nice to be able\nto add a test or two. [*]\n\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; /* print help and exit, except git_config(), repo must not be touched */\n>  };\n\nSince this is data, not code, I think it is clearer to just explain\nwhat the value represents.  The commit message and access sites can\nexplain why it matters.  Maybe something like this?\n\n\tunsigned have_repository:1;\n\tunsigned help:1;\t/* git <command> -h? */\n\nA better technical writer could probably find a better way to say it.\n\n> --- a/git.c\n> +++ b/git.c\n> @@ -246,13 +246,13 @@ 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[...]\n\nFor what it's worth,\nAcked-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks.\n\n[*] The reader might recall a long-term goal of clarifying .git dir\naccess semantics:\nhttp://thread.gmane.org/gmane.comp.version-control.git/149771/focus=152745\nGiven that, what need is there to ask about the patch's impact?  One\nanswer: I am asking about side-effects rather than the goal of the\npatch.  Positive side-effects would be indicators of a good design.\n"},{"id":"153800","messageId":"20101019172953.GC25139@burratino","threadId":"25479","inReplyTo":"1287495320-27278-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 2/3] builtins: utilize startup_info->help where possible","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-19T17:29:53Z","receivedAt":"2010-10-19T17:29:53Z","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> It helps reduce false alarms while I'm looking for \"git foo -h\" code\n> path that accesses repository.\n\nNot sure I understand.  Is the idea that use of startup_info->help\nis a marker for \"I've checked this code path\"?\n\nIf that were the only reason, I don't think I'd like the idea.\n\nAs it is, I'm a bit conflicted: what if we decide to short-circuit\n\"git foo --help-all\" in the future just like we short-circuit\n\"git foo -h\" now?  Would that require a separate flag?\n\nIn other words, I'm not sure startup_info->help is a good abstraction.\nMaybe (modulo names) it would be better to do\n\nstruct startup_info {\n\t...\n\tconst char *short_circuit;\t/* \"-h\", \"--help-all\", \"--no-index\", or NULL */\n};\n\nand use\n\n\tif (!strcmp(startup_info->short_circuit, \"-h\"))\n\nto allow relaxing the argc == 2 check later?\n"},{"id":"153804","messageId":"20101019175520.GD25139@burratino","threadId":"25479","inReplyTo":"1287495320-27278-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 3/3] builtins: check for startup_info->help, print and exit early","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-19T17:55:20Z","receivedAt":"2010-10-19T17:55:20Z","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> --- 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)\n\nOne has to work hard to trigger this one.\n\n\tgit init foo\n\tcd foo\n\t>.git/refs/heads/master\n\tgit branch -h\n\nProduces:\n\n\tfatal: Failed to resolve HEAD as a valid ref.\n\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;\n\n\tgit init foo\n\tcd foo\n\t>.git/index\n\tgit checkout-index -h\n\nProduces:\n\n\tfatal: index file smaller than expected\n\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n\nI'm stopping here.  But maybe some test cases like this would make the\nseries clearer.\n\nHope that helps,\nJonathan\n"},{"id":"153811","messageId":"20101019185014.GG25139@burratino","threadId":"25479","inReplyTo":"20101019172953.GC25139@burratino","subject":"Re: [PATCH 2/3] builtins: utilize startup_info->help where possible","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-19T18:50:15Z","receivedAt":"2010-10-19T18:50:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Nguyễn Thái Ngọc Duy wrote:\n\n>> It helps reduce false alarms while I'm looking for \"git foo -h\" code\n>> path that accesses repository.\n>\n> Not sure I understand.  Is the idea that use of startup_info->help\n> is a marker for \"I've checked this code path\"?\n> \n> If that were the only reason, I don't think I'd like the idea.\n> \n> As it is, I'm a bit conflicted: what if we decide to short-circuit\n> \"git foo --help-all\" in the future just like we short-circuit\n> \"git foo -h\" now?  Would that require a separate flag?\n\nTo be clear: I'm a bit conflicted but not extremely so.  The help\nflag has the benefit of simplicity and of allowing expansion to\n\n\tgit foo -h --gobbledegook\n\nif later someone cares and a code audit shows it to be safe.\n"},{"id":"153814","messageId":"7vlj5uyp4x.fsf@alter.siamese.dyndns.org","threadId":"25479","inReplyTo":"20101019172953.GC25139@burratino","subject":"Re: [PATCH 2/3] builtins: utilize startup_info->help where possible","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-10-19T19:10:54Z","receivedAt":"2010-10-19T19:10:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> In other words, I'm not sure startup_info->help is a good abstraction.\n> Maybe (modulo names) it would be better to do\n>\n> struct startup_info {\n> \t...\n> \tconst char *short_circuit;\t/* \"-h\", \"--help-all\", \"--no-index\", or NULL */\n> };\n\nI am not sure short-circuit is a good abstraction either; will we have\nonly one aspect of whatever we can short-circuit forever?\n"},{"id":"153840","messageId":"AANLkTinvSKAyXkzjUgxvrDWvuHWpa+46XVxTu=tWck53@mail.gmail.com","threadId":"25479","inReplyTo":"20101019172953.GC25139@burratino","subject":"Re: [PATCH 2/3] builtins: utilize startup_info->help where possible","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-10-20T01:13:06Z","receivedAt":"2010-10-20T01:13:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2010/10/20 Jonathan Nieder <jrnieder@gmail.com>:\n> Nguyễn Thái Ngọc Duy wrote:\n>\n>> It helps reduce false alarms while I'm looking for \"git foo -h\" code\n>> path that accesses repository.\n>\n> Not sure I understand.  Is the idea that use of startup_info->help\n> is a marker for \"I've checked this code path\"?\n>\n> If that were the only reason, I don't think I'd like the idea.\n\nThat. And simpler check. I mean \"if (startup_info->help)\" takes a tiny\nbit less energy for me to understand than \"if (argc == 2 && argv[1] ==\n\"-h\")\". It's also good for grepping.\n\n> As it is, I'm a bit conflicted: what if we decide to short-circuit\n> \"git foo --help-all\" in the future just like we short-circuit\n> \"git foo -h\" now?  Would that require a separate flag?\n\nAs long as it's help related, startup_info->help can be turned to a\nbit set. Although I'm not sure if --help-all or any other option will\nbe implemented anytime soon. Remember it has to be implemented for\n_all_ builtin commands, or we need to introduce NO_DASH_H in\nrun_builtin() to skip the shortcut for commands that do not support\nit.\n\n> In other words, I'm not sure startup_info->help is a good abstraction.\n> Maybe (modulo names) it would be better to do\n>\n> struct startup_info {\n>        ...\n>        const char *short_circuit;      /* \"-h\", \"--help-all\", \"--no-index\", or NULL */\n> };\n>\n> and use\n>\n>        if (!strcmp(startup_info->short_circuit, \"-h\"))\n>\n> to allow relaxing the argc == 2 check later?\n\nCan it be relaxed later when someone comes up with \"--help-all\" or something?\n-- \nDuy\n"},{"id":"153842","messageId":"20101020011820.GA6872@burratino","threadId":"25479","inReplyTo":"AANLkTinvSKAyXkzjUgxvrDWvuHWpa+46XVxTu=tWck53@mail.gmail.com","subject":"Re: [PATCH 2/3] builtins: utilize startup_info->help where possible","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-10-20T01:18:20Z","receivedAt":"2010-10-20T01:18:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyen Thai Ngoc Duy wrote:\n\n> That. And simpler check. I mean \"if (startup_info->help)\" takes a tiny\n> bit less energy for me to understand than \"if (argc == 2 && argv[1] ==\n> \"-h\")\". It's also good for grepping.\n\nMm, that still does not motivate the churn to me.  But \"we can change\nthe meaning to 'if (argc >= 2 && !strcmp(argv[1], \"-h\"))' some day\"\nmotivates it okay, I think.\n\n> 2010/10/20 Jonathan Nieder <jrnieder@gmail.com>:\n\n>> Maybe (modulo names) it would be better to do\n[...]\n> Can it be relaxed later when someone comes up with \"--help-all\" or something?\n\nYes, I was just thinking out loud.  By \"relaxed\" I meant the above\n(changing == to >= in one place).  Now that I've thought it over,\nI don't mind startup_info->help so much.\n\nHope that helps, and sorry for the noise.\nJonathan\n"},{"id":"153841","messageId":"AANLkTik7=q3UyxxY+PGhokzCSDJyb9ZRjgjM9SoZ37vd@mail.gmail.com","threadId":"25479","inReplyTo":"7vvd4y16t8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] builtins: check for startup_info->help, print and exit early","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2010-10-20T01:18:22Z","receivedAt":"2010-10-20T01:18:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2010/10/19 Junio C Hamano <gitster@pobox.com>:\n> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n>\n>> These commands need more than just git_config() before parsing\n>> commmand line arguments. Some of these activities will unconditionally\n>> look into a repository. When startup_info->help is TRUE, no repository\n>> is set up and the caller expects callees to print help usage and exit,\n>> no more.\n>>\n>> Do as expected.\n>> ---\n>\n> No sign-off given to any of the three patches...\n\nOops, I removed old signoffs and accidentally mine too.\n\n>> diff --git a/builtin/branch.c b/builtin/branch.c\n>> index 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>>               OPT_END(),\n>>       };\n>>\n>> +     if (startup_info->help)\n>> +             usage_with_options(builtin_branch_usage, options);\n>> +\n>>       git_config(git_branch_config, NULL);\n>\n> I can just say:\n>\n>    $ cd / && git branch -h\n>    usage: git branch [options] ...\n>\n> without your patch just fine (and no I am not insane to make / controlled\n> by git).\n>\n> The same for checkout-index, commit, ls-files, etc.\n>\n>> diff --git a/builtin/gc.c b/builtin/gc.c\n>> index c304638..f040171 100644\n>> --- a/builtin/gc.c\n>> +++ b/builtin/gc.c\n>> @@ -191,6 +191,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n>>\n>>       git_config(gc_config, NULL);\n>>\n>> +     if (startup_info->help)\n>> +             usage_with_options(builtin_gc_usage, builtin_gc_options);\n>> +\n>>       if (pack_refs < 0)\n>>               pack_refs = !is_bare_repository();\n>\n> This one is curious.  Why do you need a call to git_config() in \"gc\" only?\n> You don't do that for e.g. \"branch\".\n\nHmm.. I don't think that git_config() is needed. It does no harm, but\nno gain either.\n\n> While I do not see anything glaringly wrong with this change, I am not\n> sure I am getting the point of these patches.\n\nAs Jonathan pointed out, the series makes \"git foo -h\" works even in\nspecial cases where setup code may terminate program early. Apparently\nmy test in hurry does not work great.\n-- \nDuy\n"}]}