[PATCH v2 1/4] setup_git_env: print the true $GIT_DIR for debugging

[PATCH v2 1/4] setup_git_env: print the true $GIT_DIR for debugging

[Nguyễn Thái Ngọc Duy]

Ideally, GIT_DIR_ENVIRONMENT should be set before setup_git_env() is
called and "setup: GIT_DIR defaults to .git" should never be printed
out. If it is printed, $GIT_DIR is set up automatically, unexpectedly,
elsewhere.

Checking for that line is a good way to know if setup code works
properly.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Second try. Better tests.

 environment.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/environment.c b/environment.c
index de5581f..e34adc3 100644
--- a/environment.c
+++ b/environment.c
@@ -91,8 +91,12 @@ static void setup_git_env(void)
 		git_dir = read_gitfile_gently(DEFAULT_GIT_DIR_ENVIRONMENT);
 		git_dir = git_dir ? xstrdup(git_dir) : NULL;
 	}
-	if (!git_dir)
+	if (!git_dir) {
+		trace_printf("setup: GIT_DIR defaults to .git\n");
 		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
+	}
+	else
+		trace_printf("setup: GIT_DIR set to %s\n", git_dir);
 	git_object_dir = getenv(DB_ENVIRONMENT);
 	if (!git_object_dir) {
 		git_object_dir = xmalloc(strlen(git_dir) + 9);
-- 
1.7.0.2.445.gcbdb3

[PATCH v2 2/4] run_builtin(): save "-h" detection result for later use

[Nguyễn Thái Ngọc Duy]

When run_builtin() sees "-h" as the first argument, it assumes:

 - this is the call for help usage
 - the real git command will only print help usage then exit

So it skips all setup in this case.  Unfortunately, some commands do
other things before calling parse_options(), which is often where the
help usage is printed.  Some of those things may try to access the
repository unnecessarily. If a repository is broken, the command may
die() before it prints help usage, not really helpful.

Make real commands aware of this fast path so that they can handle it
properly (i.e., print help usage then exit immediately) if they were
going to do more initialization than git_config().

Demonstrating "git foo -h" fails depends on individual commands and
is generally difficult to do. Instead GIT_TRACE is used to check
if a command does set repo. If it does, it is supposed to fail if
repo setup code chokes.

"git upload-archive" fails for another reason, but will be fixed too
when "git upload-archive -h" is converted to use startup_info->help

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache.h         |    1 +
 git.c           |   18 ++++++++++++++----
 t/t3905-help.sh |   24 ++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 4 deletions(-)
 create mode 100755 t/t3905-help.sh

diff --git a/cache.h b/cache.h
index 33decd9..bb57e34 100644
--- a/cache.h
+++ b/cache.h
@@ -1117,6 +1117,7 @@ const char *split_cmdline_strerror(int cmdline_errno);
 /* git.c */
 struct startup_info {
 	int have_repository;
+	int help; /* git foo -h */
 };
 extern struct startup_info *startup_info;
 
diff --git a/git.c b/git.c
index 50a1401..bb67540 100644
--- a/git.c
+++ b/git.c
@@ -246,13 +246,23 @@ struct cmd_struct {
 
 static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 {
-	int status, help;
+	int status;
 	struct stat st;
 	const char *prefix;
 
 	prefix = NULL;
-	help = argc == 2 && !strcmp(argv[1], "-h");
-	if (!help) {
+	startup_info->help = argc == 2 && !strcmp(argv[1], "-h");
+	if (startup_info->help) {
+		/*
+		 * Fast path for "git foo -h", no setup is done.
+		 * Other functions might set .git up automatically
+		 * and potentially die() along the way. It's best
+		 * to check this flag from the beginning, print its
+		 * help usage and exit, nothing more.
+		 */
+		;
+	}
+	else {
 		if (p->option & RUN_SETUP)
 			prefix = setup_git_directory();
 		if (p->option & RUN_SETUP_GENTLY) {
@@ -267,7 +277,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	}
 	commit_pager_choice();
 
-	if (!help && p->option & NEED_WORK_TREE)
+	if (!startup_info->help && p->option & NEED_WORK_TREE)
 		setup_work_tree();
 
 	trace_argv_printf(argv, "trace: built-in: git");
diff --git a/t/t3905-help.sh b/t/t3905-help.sh
new file mode 100755
index 0000000..0dcbedf
--- /dev/null
+++ b/t/t3905-help.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+
+test_description='tests that git foo -h should work even in potentially broken repos'
+
+. ./test-lib.sh
+
+test_help() {
+	test_expect_"$1" "$2 -h" "
+		GIT_TRACE=\"`pwd`\"/$2.log test_must_fail git $2 -h &&
+		test \$exit_code = 129 &&
+		! grep 'defaults to' $2.log
+	"
+}
+
+test_help failure branch
+test_help failure checkout-index
+test_help failure commit
+test_help failure gc
+test_help failure ls-files
+test_help failure merge
+test_help failure update-index
+test_help failure upload-archive
+
+test_done
-- 
1.7.0.2.445.gcbdb3

Re: [PATCH v2 2/4] run_builtin(): save "-h" detection result for later use

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> When run_builtin() sees "-h" as the first argument, it assumes:
>
>  - this is the call for help usage
>  - the real git command will only print help usage then exit
>
> So it skips all setup in this case.  Unfortunately, some commands do
> other things before calling parse_options(), which is often where the
> help usage is printed.  Some of those things may try to access the
> repository unnecessarily. If a repository is broken, the command may
> die() before it prints help usage, not really helpful.

What does die() message say in that case?  If it says "your repository is
broken", that may be more useful than giving a help message.  I dunno.

> Demonstrating "git foo -h" fails depends on individual commands and
> is generally difficult to do. Instead GIT_TRACE is used to check
> if a command does set repo. If it does, it is supposed to fail if
> repo setup code chokes.

Hmm, I am not sure I understand this one.  If you are interested in
changing the behaviour of these commands when run with "-h" in a corrupt
repository, perhaps you can deliberately corrupt the test repository in
the trash directory you start with, and run these commands there, no?
For a good measure, you could use CEILING_DIRECTORIES to make sure the
tests do not climb up to the project repository.

Re: [PATCH v2 2/4] run_builtin(): save "-h" detection result for later use

[Nguyen Thai Ngoc Duy]

2010/10/22 Junio C Hamano <gitster@pobox.com>:
> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> When run_builtin() sees "-h" as the first argument, it assumes:
>>
>>  - this is the call for help usage
>>  - the real git command will only print help usage then exit
>>
>> So it skips all setup in this case.  Unfortunately, some commands do
>> other things before calling parse_options(), which is often where the
>> help usage is printed.  Some of those things may try to access the
>> repository unnecessarily. If a repository is broken, the command may
>> die() before it prints help usage, not really helpful.
>
> What does die() message say in that case?  If it says "your repository is
> broken", that may be more useful than giving a help message.  I dunno.

I should have written "if repository access is broken" (i.e. .git has
not been found, then somewhere access to .git is requested and .git is
set up automatically). But I'm chasing a ghost here. And the impact to
"-h" is probably nothing (how can accessing a wrong .git impacts a
static help string?). I'll take the series back. There are more
important things to work on than this.
--
Duy

[PATCH v2 3/4] builtins: utilize startup_info->help where possible

[Nguyễn Thái Ngọc Duy]

It helps reduce false alarms while I'm looking for "git foo -h" code
path that accesses repository. Anyway it looks like a good thing to
do. If one day people like to have "git foo --help" as an alternative
to "git foo -h", it would be easy.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/check-ref-format.c |    2 +-
 builtin/grep.c             |    2 +-
 builtin/index-pack.c       |    2 +-
 builtin/log.c              |    6 +-----
 builtin/merge-ours.c       |    2 +-
 builtin/pack-redundant.c   |    2 +-
 builtin/show-ref.c         |    2 +-
 7 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index ae3f281..c6511e3 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -58,7 +58,7 @@ static int check_ref_format_print(const char *arg)
 
 int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
 {
-	if (argc == 2 && !strcmp(argv[1], "-h"))
+	if (startup_info->help)
 		usage(builtin_check_ref_format_usage);
 
 	if (argc == 3 && !strcmp(argv[1], "--branch"))
diff --git a/builtin/grep.c b/builtin/grep.c
index da32f3d..ec39909 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -934,7 +934,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
 	 * 'git grep -h', unlike 'git grep -h <pattern>', is a request
 	 * to show usage information and exit.
 	 */
-	if (argc == 2 && !strcmp(argv[1], "-h"))
+	if (startup_info->help)
 		usage_with_options(grep_usage, options);
 
 	memset(&opt, 0, sizeof(opt));
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index e243d9d..649ad18 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -881,7 +881,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 	struct pack_idx_entry **idx_objects;
 	unsigned char pack_sha1[20];
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
+	if (startup_info->help)
 		usage(index_pack_usage);
 
 	read_replace_refs = 0;
diff --git a/builtin/log.c b/builtin/log.c
index 22d1290..a7ba9ed 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -69,11 +69,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
 	if (default_date_mode)
 		rev->date_mode = parse_date_format(default_date_mode);
 
-	/*
-	 * Check for -h before setup_revisions(), or "git log -h" will
-	 * fail when run without a git directory.
-	 */
-	if (argc == 2 && !strcmp(argv[1], "-h"))
+	if (startup_info->help)
 		usage(builtin_log_usage);
 	argc = setup_revisions(argc, argv, rev, opt);
 
diff --git a/builtin/merge-ours.c b/builtin/merge-ours.c
index 6844116..8e0777b 100644
--- a/builtin/merge-ours.c
+++ b/builtin/merge-ours.c
@@ -20,7 +20,7 @@ static const char *diff_index_args[] = {
 
 int cmd_merge_ours(int argc, const char **argv, const char *prefix)
 {
-	if (argc == 2 && !strcmp(argv[1], "-h"))
+	if (startup_info->help)
 		usage(builtin_merge_ours_usage);
 
 	/*
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 41e1615..3f090b2 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -601,7 +601,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix)
 	unsigned char *sha1;
 	char buf[42]; /* 40 byte sha1 + \n + \0 */
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
+	if (startup_info->help)
 		usage(pack_redundant_usage);
 
 	for (i = 1; i < argc; i++) {
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index be9b512..7083fa9 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -204,7 +204,7 @@ static const struct option show_ref_options[] = {
 
 int cmd_show_ref(int argc, const char **argv, const char *prefix)
 {
-	if (argc == 2 && !strcmp(argv[1], "-h"))
+	if (startup_info->help)
 		usage_with_options(show_ref_usage, show_ref_options);
 
 	argc = parse_options(argc, argv, prefix, show_ref_options,
-- 
1.7.0.2.445.gcbdb3

[PATCH v2 4/4] builtins: check for startup_info->help, print and exit early

[Nguyễn Thái Ngọc Duy]

By ensuring no access to repo is done in "git foo -h" case, the commands
have better chance of really printing out help usage. Access to repo is not
necessary and may terminate program if it finds something wrong.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/branch.c         |    3 +++
 builtin/checkout-index.c |    3 +++
 builtin/commit.c         |    6 ++++++
 builtin/gc.c             |    3 +++
 builtin/ls-files.c       |    3 +++
 builtin/merge.c          |    3 +++
 builtin/update-index.c   |    3 +++
 builtin/upload-archive.c |    7 ++++---
 t/t3905-help.sh          |   16 ++++++++--------
 9 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 87976f0..9f152ed 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -667,6 +667,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
+	if (startup_info->help)
+		usage_with_options(builtin_branch_usage, options);
+
 	git_config(git_branch_config, NULL);
 
 	if (branch_use_color == -1)
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index a7a5ee1..7f25cd7 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -241,6 +241,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	if (startup_info->help)
+		usage_with_options(builtin_checkout_index_usage, builtin_checkout_index_options);
+
 	git_config(git_default_config, NULL);
 	state.base_dir = "";
 	prefix_length = prefix ? strlen(prefix) : 0;
diff --git a/builtin/commit.c b/builtin/commit.c
index 66fdd22..8b086f2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1070,6 +1070,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
+	if (startup_info->help)
+		usage_with_options(builtin_status_usage, builtin_status_options);
+
 	if (null_termination && status_format == STATUS_FORMAT_LONG)
 		status_format = STATUS_FORMAT_PORCELAIN;
 
@@ -1255,6 +1258,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	int allow_fast_forward = 1;
 	struct wt_status s;
 
+	if (startup_info->help)
+		usage_with_options(builtin_commit_usage, builtin_commit_options);
+
 	wt_status_prepare(&s);
 	git_config(git_commit_config, &s);
 	in_merge = file_exists(git_path("MERGE_HEAD"));
diff --git a/builtin/gc.c b/builtin/gc.c
index c304638..5a9d0da 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -189,6 +189,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	if (startup_info->help)
+		usage_with_options(builtin_gc_usage, builtin_gc_options);
+
 	git_config(gc_config, NULL);
 
 	if (pack_refs < 0)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index bb4f612..814da51 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -530,6 +530,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		OPT_END()
 	};
 
+	if (startup_info->help)
+		usage_with_options(ls_files_usage, builtin_ls_files_options);
+
 	memset(&dir, 0, sizeof(dir));
 	prefix = cmd_prefix;
 	if (prefix)
diff --git a/builtin/merge.c b/builtin/merge.c
index 2dba3b9..0169694 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -917,6 +917,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	const char *best_strategy = NULL, *wt_strategy = NULL;
 	struct commit_list **remotes = &remoteheads;
 
+	if (startup_info->help)
+		usage_with_options(builtin_merge_usage, builtin_merge_options);
+
 	if (read_cache_unmerged()) {
 		die_resolve_conflict("merge");
 	}
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 3ab214d..46a53f5 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -589,6 +589,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	int lock_error = 0;
 	struct lock_file *lock_file;
 
+	if (startup_info->help)
+		usage(update_index_usage);
+
 	git_config(git_default_config, NULL);
 
 	/* We can't free this memory, it becomes part of a linked list parsed atexit() */
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 73f788e..d4f4741 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -26,9 +26,6 @@ static int run_upload_archive(int argc, const char **argv, const char *prefix)
 	int sent_argc;
 	int len;
 
-	if (argc != 2)
-		usage(upload_archive_usage);
-
 	if (strlen(argv[1]) + 1 > sizeof(buf))
 		die("insanely long repository name");
 
@@ -98,6 +95,10 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
 {
 	pid_t writer;
 	int fd1[2], fd2[2];
+
+	if (startup_info->help || argc != 2)
+		usage(upload_archive_usage);
+
 	/*
 	 * Set up sideband subprocess.
 	 *
diff --git a/t/t3905-help.sh b/t/t3905-help.sh
index 0dcbedf..6cab6b9 100755
--- a/t/t3905-help.sh
+++ b/t/t3905-help.sh
@@ -12,13 +12,13 @@ test_help() {
 	"
 }
 
-test_help failure branch
-test_help failure checkout-index
-test_help failure commit
-test_help failure gc
-test_help failure ls-files
-test_help failure merge
-test_help failure update-index
-test_help failure upload-archive
+test_help success branch
+test_help success checkout-index
+test_help success commit
+test_help success gc
+test_help success ls-files
+test_help success merge
+test_help success update-index
+test_help success upload-archive
 
 test_done
-- 
1.7.0.2.445.gcbdb3

[PATCH en/and-cascade-tests 0/7]

[Jonathan Nieder]

Nguyễn Thái Ngọc Duy wrote:

>  builtin/branch.c         |    3 +++
>  builtin/checkout-index.c |    3 +++
>  builtin/commit.c         |    6 ++++++
>  builtin/gc.c             |    3 +++
>  builtin/ls-files.c       |    3 +++
>  builtin/merge.c          |    3 +++
>  builtin/update-index.c   |    3 +++
>  builtin/upload-archive.c |    7 ++++---

Okay, all of them survive a reroll except upload-archive.  As for
upload-archive:

 - it already doesn't support "-h"
 - it is such low-level plumbing, I don't really mind that.  Maybe
   someone is using a repository named "-h" (though I hope not, of
   course).

Maybe these patches should be squashed.  I separated them because they
were easier to write and it would be easier to cc the right people
this way.

The primary motivation for the series is that the repository checker
from the nd/setup series will not be happy without some change like
this.  In the git <foo> -h codepath, repository setup is not run so
repository access is forbidden.

The secondary motivation is to create better behavior in situations of
repository corruption.  When I type "git checkout -h", I am asking for
a usage message, not a repository self-check.  Especially when trying
to repair a repository, commands that do not do what they are asked to
are generally frustrating to use.

Patches apply on top of "test-lib: make test_expect_code a test
command" from the en/and-cascade-tests topic.

Enjoy,
Jonathan

Nguyễn Thái Ngọc Duy (7):
  branch -h: show usage even in an invalid repository
  checkout-index -h: show usage even in an invalid repository
  commit/status -h: show usage even with broken configuration
  gc -h: show usage even with broken configuration
  ls-files -h: show usage even with corrupt index
  merge -h: show usage even with corrupt index
  update-index -h: show usage even with corrupt index

 builtin/branch.c                |    3 +++
 builtin/checkout-index.c        |    3 +++
 builtin/commit.c                |    6 ++++++
 builtin/gc.c                    |    3 +++
 builtin/ls-files.c              |    3 +++
 builtin/merge.c                 |    2 ++
 builtin/update-index.c          |    3 +++
 t/t2006-checkout-index-basic.sh |   24 ++++++++++++++++++++++++
 t/t2107-update-index-basic.sh   |   32 ++++++++++++++++++++++++++++++++
 t/t3004-ls-files-basic.sh       |   39 +++++++++++++++++++++++++++++++++++++++
 t/t3200-branch.sh               |   11 +++++++++++
 t/t6500-gc.sh                   |   28 ++++++++++++++++++++++++++++
 t/t7508-status.sh               |   24 ++++++++++++++++++++++++
 t/t7600-merge.sh                |   11 +++++++++++
 14 files changed, 192 insertions(+), 0 deletions(-)
 create mode 100755 t/t2006-checkout-index-basic.sh
 create mode 100755 t/t2107-update-index-basic.sh
 create mode 100755 t/t3004-ls-files-basic.sh
 create mode 100755 t/t6500-gc.sh

-- 
1.7.2.3

[PATCH 1/7] branch -h: show usage even in an invalid repository

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

There is no need for "git branch -h" to try to access a repository.

In the spirit of v1.6.6-rc0~34^2~3 (Let 'git <command> -h' show usage
without a git dir, 2009-11-09).  This brings git one step closer to
passing the following (automatically verifiable) test:

 Before any repository access (aside from git_config()), a
 function from the setup_git_directory_* family has been run

and thus one step closer to being able to use an automatic repository
access checker.

[jn: simplified; new commit message, test]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/branch.c  |    3 +++
 t/t3200-branch.sh |   11 +++++++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 87976f0..0e50556 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -667,6 +667,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage_with_options(builtin_branch_usage, options);
+
 	git_config(git_branch_config, NULL);
 
 	if (branch_use_color == -1)
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index f54a533..f308235 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -26,6 +26,17 @@ test_expect_success \
      ! test -f .git/refs/heads/--help
 '
 
+test_expect_success 'branch -h in broken repository' '
+	mkdir broken &&
+	(
+		cd broken &&
+		git init &&
+		>.git/refs/heads/master &&
+		test_expect_code 129 git branch -h >usage 2>&1
+	) &&
+	grep "[Uu]sage" broken/usage
+'
+
 test_expect_success \
     'git branch abc should create a branch' \
     'git branch abc && test -f .git/refs/heads/abc'
-- 
1.7.2.3

Re: [PATCH 1/7] branch -h: show usage even in an invalid repository

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> [jn: simplified; new commit message, test]

Overall (not just this 1/7) the messages justify the changes a lot
better.  Thanks for a pleasant read ;-).

> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index f54a533..f308235 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -26,6 +26,17 @@ test_expect_success \
>       ! test -f .git/refs/heads/--help
>  '
>  
> +test_expect_success 'branch -h in broken repository' '
> +	mkdir broken &&
> +	(
> +		cd broken &&
> +		git init &&
> +		>.git/refs/heads/master &&
> +		test_expect_code 129 git branch -h >usage 2>&1
> +	) &&
> +	grep "[Uu]sage" broken/usage
> +'

A handful of points that struck me (not just test, but taking tests from
other patches together) as slightly odd, all minor:

 - Why '[Uu]sage"?  Should we make the messages consistent further?

 - The final "grep" check may want to make sure that the message is free of
   "fatal" (or "error", "warning", etc.) as well?

 - Each test seems to anticipate a specific kind of breakage tailored for
   the command being tested (e.g. "branch" test does not corrupt config
   nor the index).  Perhaps "lib-corrupt.sh" helper to run the same test
   in variety of corrupt repositories would help improve coverage?  [*1*]

 - Some tests redirect both the standard output and the standard error
   (like this patch) and check the combined result, while some others
   (e.g. 2/7) check only the standard error stream.  Don't we want to be
   testing them more uniformly?

>  test_expect_success \
>      'git branch abc should create a branch' \
>      'git branch abc && test -f .git/refs/heads/abc'

Don't we want to rather use resolve-ref or "rev-parse --verify", just in
case we may later change "git branch" to update packed-refs directly?


[Footnote]

*1* I am of two minds, as for example a corrupt "gc.pruneexpire" cannot
possibly matter for correct operation of "git branch", but am just
throwing out an idea to see if somebody else have clever ideas.

Re: [PATCH 1/7] branch -h: show usage even in an invalid repository

[Jonathan Nieder]

Junio C Hamano wrote:

>  - Some tests redirect both the standard output and the standard error
>    (like this patch) and check the combined result, while some others
>    (e.g. 2/7) check only the standard error stream.  Don't we want to be
>    testing them more uniformly?

Good point.  The current state is:

 - Messages from a mistake in usage are reported to stderr.
 - Messages from git <command> -h are reported to stdout or
   stderr, with a slight preference for stdout.

A nicer behavior would be

 - Messages from a mistake in usage are reported to stderr.
 - Messages from git <command> -h are reported to stdout.

Here's a helper to make it easier for commands that use parse-options
to adopt that nicer behavior.  It writes its output to stdout, so it
should only be used to be used to handle the -h option.  Example:

	if (argc == 2 && !strcmp(argv[1], "-h"))
		usage_for_help_opt(builtin_foo_usage, options);

	... some scary commands that should not run with -h ...

	argc = parse_options(argc, ...
	if (argc != 1)
		usage_with_options(builtin_foo_usage, options);

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---

diff --git a/parse-options.c b/parse-options.c
index 0fa79bc..e92fcfe 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -567,6 +567,13 @@ void usage_with_options(const char * const *usagestr,
 	exit(129);
 }
 
+void usage_for_help_opt(const char * const *usagestr,
+			const struct option *opts)
+{
+	usage_with_options_internal(NULL, usagestr, opts, 0, 0);
+	exit(129);
+}
+
 void usage_msg_opt(const char *msg,
 		   const char * const *usagestr,
 		   const struct option *options)
diff --git a/parse-options.h b/parse-options.h
index d982f0f..de69826 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -152,6 +152,9 @@ extern int parse_options(int argc, const char **argv, const char *prefix,
 extern NORETURN void usage_with_options(const char * const *usagestr,
                                         const struct option *options);
 
+extern NORETURN void usage_for_help_opt(const char * const *usagestr,
+                                        const struct option *options);
+
 extern NORETURN void usage_msg_opt(const char *msg,
 				   const char * const *usagestr,
 				   const struct option *options);

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

[Jonathan Nieder]

Jonathan Nieder wrote:

> Here's a helper to make it easier for commands that use parse-options
> to adopt that nicer behavior.  It writes its output to stdout, so it
> should only be used to be used to handle the -h option.

Alas, "git update-index" does not use parse-options yet.  But that
is easily enough changed...

Patch 1 introduces an OPTION_LOWLEVEL_CALLBACK backdoor to
parse-options, so new option types (like the three-argument type
used by update-index --cacheinfo) can be supported without tempting
inventors of other commands through mention in the public API.

Patch 2 is a toy patch I previously send to Bo.  It makes
parse_option_step() distinguish between stops due to end of options
and stops due to appearance of a nonoption; update-index benefits
from something like this because it needs to deal with filename
parameters as they appear.

Patch 3 converts update-index to parse-options.  The commit message
mentions some potential simplifications.

All three patches ought to introduce new tests, but none do.  I
hope that does not hinder reading them too much.  Thoughts?

Jonathan Nieder (3):
  parse-options: allow git commands to invent new option types
  parse-options: make resuming easier after
    PARSE_OPT_STOP_AT_NON_OPTION
  update-index: migrate to parse-options API

Nguyễn Thái Ngọc Duy (1):
  setup: save prefix (original cwd relative to toplevel) in
    startup_info

 builtin/update-index.c |  389 +++++++++++++++++++++++++++++------------------
 cache.h                |    1 +
 parse-options.c        |    9 +-
 parse-options.h        |   10 ++
 setup.c                |    4 +-
 5 files changed, 259 insertions(+), 154 deletions(-)

-- 
1.7.2.3

[PATCH 1/4] parse-options: allow git commands to invent new option types

[Jonathan Nieder]

parse-options provides a variety of option behaviors, including
OPTION_CALLBACK, which should take care of just about any sane
behavior.  All supported behaviors obey the following constraint:

 A --foo option can only accept (and base its behavior on)
 one argument, which would be the following command-line
 argument in the "unsticked" form.

Alas, some existing git commands have options that do not obey that
constraint.  For example, update-index --cacheinfo takes three
arguments, and update-index --resolve takes all later parameters as
arguments.

Introduce a new option type (OPTION_LOWLEVEL_CALLBACK) to support
such unusual options.  parse_options() callers can implement an
arbitrary custom get_value() function to override the usual one and
pass it through the callback field for options of interest.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 parse-options.c |    6 +++---
 parse-options.h |    9 +++++++++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 0fa79bc..7907306 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -8,9 +8,6 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,
 			       const char * const *usagestr,
 			       const struct option *opts, int err);
 
-#define OPT_SHORT 1
-#define OPT_UNSET 2
-
 static int opterror(const struct option *opt, const char *reason, int flags)
 {
 	if (flags & OPT_SHORT)
@@ -51,6 +48,9 @@ static int get_value(struct parse_opt_ctx_t *p,
 	const int unset = flags & OPT_UNSET;
 	int err;
 
+	if (opt->type == OPTION_LOWLEVEL_CALLBACK)
+		return (*(parse_opt_ll_cb *)opt->callback)(p, opt, flags);
+
 	if (unset && p->opt)
 		return opterror(opt, "takes no value", flags);
 	if (unset && (opt->flags & PARSE_OPT_NONEG))
diff --git a/parse-options.h b/parse-options.h
index d982f0f..fa400da 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -17,6 +17,7 @@ enum parse_opt_type {
 	OPTION_STRING,
 	OPTION_INTEGER,
 	OPTION_CALLBACK,
+	OPTION_LOWLEVEL_CALLBACK,
 	OPTION_FILENAME
 };
 
@@ -40,8 +41,16 @@ enum parse_opt_option_flags {
 	PARSE_OPT_SHELL_EVAL = 256
 };
 
+enum parse_opt_ll_flags {
+	OPT_SHORT = 1,
+	OPT_UNSET = 2
+};
+
 struct option;
+struct parse_opt_ctx_t;
 typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
+typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
+			    const struct option *opt, int flags);
 
 /*
  * `type`::
-- 
1.7.2.3

Re: [PATCH 1/4] parse-options: allow git commands to invent new option types

[Stephen Boyd]

On 10/24/10 01:15, Jonathan Nieder wrote:
> diff --git a/parse-options.c b/parse-options.c
> index 0fa79bc..7907306 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -8,9 +8,6 @@ static int parse_options_usage(struct parse_opt_ctx_t *ctx,
>  			       const char * const *usagestr,
>  			       const struct option *opts, int err);
>  
> -#define OPT_SHORT 1
> -#define OPT_UNSET 2
> -
>  static int opterror(const struct option *opt, const char *reason, int flags)
>  {
>  	if (flags & OPT_SHORT)
> @@ -51,6 +48,9 @@ static int get_value(struct parse_opt_ctx_t *p,
>  	const int unset = flags & OPT_UNSET;
>  	int err;
>  
> +	if (opt->type == OPTION_LOWLEVEL_CALLBACK)
> +		return (*(parse_opt_ll_cb *)opt->callback)(p, opt, flags);
> +

(I read patch 4 before this one)

Being able to modify the context within a callback is nice. Having to
know if the option is short or long and and checking for validity seems
like something that should be handled within the parse options library
itself.

Is there an actual use case where someone needs to completely override
get_value()? If you move this into the case statement then we get the
generic error checking of get_value() with the benefits of being able to
modify the context within a callback. We could also probably use the
return value of the low level callback to indicate whether or not to
take some action after parsing the option. Perhaps something like
quiting the option parsing loop when encountering such an option?

This reminds me, we can probably simplify that "takes no value" error
path in get_value() (see below).

> diff --git a/parse-options.h b/parse-options.h
> index d982f0f..fa400da 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -17,6 +17,7 @@ enum parse_opt_type {
>  	OPTION_STRING,
>  	OPTION_INTEGER,
>  	OPTION_CALLBACK,
> +	OPTION_LOWLEVEL_CALLBACK,
>  	OPTION_FILENAME
>  };

Don't forget to document what this option does in the comments of this
file and in api-parse-options.txt

>  
> @@ -40,8 +41,16 @@ enum parse_opt_option_flags {
>  	PARSE_OPT_SHELL_EVAL = 256
>  };
>  
> +enum parse_opt_ll_flags {
> +	OPT_SHORT = 1,
> +	OPT_UNSET = 2
> +};
> +

I hope this isn't necessary.

Hope this pasted ok.
--->8----

diff --git a/parse-options.c b/parse-options.c
index 0fa79bc..7234c11 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -56,22 +56,8 @@ static int get_value(struct parse_opt_ctx_t *p,
        if (unset && (opt->flags & PARSE_OPT_NONEG))
                return opterror(opt, "isn't available", flags);

-       if (!(flags & OPT_SHORT) && p->opt) {
-               switch (opt->type) {
-               case OPTION_CALLBACK:
-                       if (!(opt->flags & PARSE_OPT_NOARG))
-                               break;
-                       /* FALLTHROUGH */
-               case OPTION_BOOLEAN:
-               case OPTION_BIT:
-               case OPTION_NEGBIT:
-               case OPTION_SET_INT:
-               case OPTION_SET_PTR:
+       if (!(flags & OPT_SHORT) && p->opt && (opt->flags &
PARSE_OPT_NOARG))
                        return opterror(opt, "takes no value", flags);
-               default:
-                       break;
-               }
-       }

        switch (opt->type) {
        case OPTION_BIT:

Re: [PATCH 1/4] parse-options: allow git commands to invent new option types

[Jonathan Nieder]

Stephen Boyd wrote:

> Is there an actual use case where someone needs to completely override
> get_value()? If you move this into the case statement then we get the
> generic error checking of get_value() with the benefits of being able to
> modify the context within a callback.

Yes, I like this idea.

> We could also probably use the
> return value of the low level callback to indicate whether or not to
> take some action after parsing the option. Perhaps something like
> quiting the option parsing loop when encountering such an option?

I'd rather not, since that would involve anticipating needs in advance.
This is meant to be a back door for ugly and unusual option types that
the mainstream API does not take care of yet.  It seems best to allow
arbitrary effects.

[...]
>> -#define OPT_SHORT 1
>> -#define OPT_UNSET 2
[...]
>> +++ b/parse-options.h
>> @@ -40,8 +41,16 @@ enum parse_opt_option_flags {
>>  	PARSE_OPT_SHELL_EVAL = 256
>>  };
>>  
>> +enum parse_opt_ll_flags {
>> +	OPT_SHORT = 1,
>> +	OPT_UNSET = 2
>> +};
>> +
>
> I hope this isn't necessary.

Because of namespace or something else?

> This reminds me, we can probably simplify that "takes no value" error
> path in get_value() (see below).

Nice.

Thanks,
Jonathan

[PATCH 2/4] parse-options: make resuming easier after PARSE_OPT_STOP_AT_NON_OPTION

[Jonathan Nieder]

Introduce a PARSE_OPT_NON_OPTION state, so parse_option_step()
callers can easily distinguish between non-options and other
reasons for option parsing termination (like "--").

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 parse-options.c |    3 ++-
 parse-options.h |    1 +
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 7907306..cba982a 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -374,7 +374,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			if (parse_nodash_opt(ctx, arg, options) == 0)
 				continue;
 			if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)
-				break;
+				return PARSE_OPT_NON_OPTION;
 			ctx->out[ctx->cpidx++] = ctx->argv[0];
 			continue;
 		}
@@ -456,6 +456,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
 		exit(129);
+	case PARSE_OPT_NON_OPTION:
 	case PARSE_OPT_DONE:
 		break;
 	default: /* PARSE_OPT_UNKNOWN */
diff --git a/parse-options.h b/parse-options.h
index fa400da..07a0543 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -170,6 +170,7 @@ extern NORETURN void usage_msg_opt(const char *msg,
 enum {
 	PARSE_OPT_HELP = -1,
 	PARSE_OPT_DONE,
+	PARSE_OPT_NON_OPTION,
 	PARSE_OPT_UNKNOWN
 };
 
-- 
1.7.2.3

[PATCH 3/4] setup: save prefix (original cwd relative to toplevel) in startup_info

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Save the path from the original cwd to the cwd at the end of the
setup procedure in the startup_info struct introduced in e37c1329
(2010-08-05).  The value cannot vary from thread to thread anyway,
since the cwd is global.

So now in your builtin command, instead of passing prefix around,
when you want to convert a user-supplied path to a cwd-relative
path, you can use startup_info->prefix directly.

Caveat: As with the return value from setup_git_directory_gently(),
startup_info->prefix would be NULL when the original cwd is not a
subdir of the toplevel.

Longer term, this woiuld allow the prefix to be reused when several
noncooperating functions require access to the same repository (for
example, when accessing configuration before running a builtin).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 cache.h |    1 +
 setup.c |    4 +++-
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/cache.h b/cache.h
index 33decd9..222d9cf 100644
--- a/cache.h
+++ b/cache.h
@@ -1117,6 +1117,7 @@ const char *split_cmdline_strerror(int cmdline_errno);
 /* git.c */
 struct startup_info {
 	int have_repository;
+	const char *prefix;
 };
 extern struct startup_info *startup_info;
 
diff --git a/setup.c b/setup.c
index a3b76de..833db12 100644
--- a/setup.c
+++ b/setup.c
@@ -512,8 +512,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	const char *prefix;
 
 	prefix = setup_git_directory_gently_1(nongit_ok);
-	if (startup_info)
+	if (startup_info) {
 		startup_info->have_repository = !nongit_ok || !*nongit_ok;
+		startup_info->prefix = prefix;
+	}
 	return prefix;
 }
 
-- 
1.7.2.3

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

[Jonathan Nieder]

--refresh and --really-refresh accept flags (like -q) and modify
an error indicator.  It might make sense to make the error
indicator global; but just pass the flags and a pointer to the error
indicator in a struct instead.

--cacheinfo wants 3 arguments.  Use the new OPTION_LOWLEVEL_CALLBACK
extension to grab them.

--assume-unchanged and --no-assume-unchanged probably should use the
OPT_UYN feature; but use a callback for now so the existing MARK_FLAG
and UNMARK_FLAG values can be used.

--stdin and --index-info are still constrained to be the last argument
(implemented using the OPTION_LOWLEVEL_CALLBACK extension).

--unresolve and --again consume all arguments that come after them
(also using OPTION_LOWLEVEL_CALLBACK).

The order of options matters.  Each path on the command line is
affected only by the options that come before it; use a custom
argument-parsing loop with parse_options_step() to bring that about.

In exchange for all the fuss, we get the usual perks: support for
un-sticked options, better usage error messages, more useful -h
output, and argument parsing code that should be easier to tweak
in the future.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/update-index.c |  389 +++++++++++++++++++++++++++++------------------
 1 files changed, 240 insertions(+), 149 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 62d9f3f..021d054 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -10,6 +10,7 @@
 #include "builtin.h"
 #include "refs.h"
 #include "resolve-undo.h"
+#include "parse-options.h"
 
 /*
  * Default to not allowing changes to the list of files. The
@@ -397,8 +398,10 @@ static void read_index_info(int line_termination)
 	strbuf_release(&uq);
 }
 
-static const char update_index_usage[] =
-"git update-index [-q] [--add] [--replace] [--remove] [--unmerged] [--refresh] [--really-refresh] [--cacheinfo] [--chmod=(+|-)x] [--assume-unchanged] [--skip-worktree|--no-skip-worktree] [--info-only] [--force-remove] [--stdin] [--index-info] [--unresolve] [--again | -g] [--ignore-missing] [-z] [--verbose] [--] [<file>...]";
+static const char * const update_index_usage[] = {
+"git update-index [-q] [--add] [--replace] [--remove] [--unmerged] [--refresh] [--really-refresh] [--cacheinfo] [--chmod=(+|-)x] [--assume-unchanged] [--skip-worktree|--no-skip-worktree] [--info-only] [--force-remove] [--stdin] [--index-info] [--unresolve] [--again | -g] [--ignore-missing] [-z] [--verbose] [--] [<file>...]",
+	NULL
+};
 
 static unsigned char head_sha1[20];
 static unsigned char merge_head_sha1[20];
@@ -578,16 +581,207 @@ static int do_reupdate(int ac, const char **av,
 	return 0;
 }
 
+struct refresh_params {
+	unsigned int flags;
+	int *has_errors;
+};
+
+static int refresh_cb_1(const struct option *opt, const char *arg, int unset,
+			unsigned int flag)
+{
+	struct refresh_params *o = opt->value;
+	setup_work_tree();
+	*o->has_errors |= refresh_cache(o->flags | flag);
+	return 0;
+}
+
+static int refresh_cb(const struct option *opt, const char *arg, int unset)
+{
+	return refresh_cb_1(opt, arg, unset, 0);
+}
+
+static int really_refresh_cb(const struct option *opt, const char *arg, int unset)
+{
+	return refresh_cb_1(opt, arg, unset, REFRESH_REALLY);
+}
+
+static int chmod_arg_cb(const struct option *opt, const char *arg, int unset)
+{
+	char *set_executable_bit = opt->value;
+	if ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2])
+		return error("argument to --chmod must be +x or -x");
+	*set_executable_bit = arg[0];
+	return 0;
+}
+
+static int resolve_undo_clear_cb(const struct option *opt, const char *arg, int unset)
+{
+	resolve_undo_clear();
+	return 0;
+}
+
+static int cacheinfo_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,
+			int flags)
+{
+	unsigned char sha1[20];
+	unsigned int mode;
+	if (flags & OPT_UNSET)
+		return error("--cacheinfo cannot be negated");
+	if (ctx->opt)
+		return error("--cacheinfo does not accept an attached argument");
+	if (ctx->argc <= 3)
+		return error("--cacheinfo requires three arguments");
+	if (strtoul_ui(*++ctx->argv, 8, &mode) ||
+	    get_sha1_hex(*++ctx->argv, sha1) ||
+	    add_cacheinfo(mode, sha1, *++ctx->argv, 0))
+		die("git update-index: --cacheinfo cannot add %s", *ctx->argv);
+	ctx->argc -= 3;
+	return 0;
+}
+
+static int stdin_cacheinfo_cb(struct parse_opt_ctx_t *ctx,
+			      const struct option *opt, int flags)
+{
+	int *line_termination = opt->value;
+	if (ctx->argc != 1 || ctx->opt)
+		return error("--%s must be at the end", opt->long_name);
+	if (flags & OPT_UNSET)
+		return error("--%s cannot be negated", opt->long_name);
+	allow_add = allow_replace = allow_remove = 1;
+	read_index_info(*line_termination);
+	return 0;
+}
+
+static int last_arg_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,
+		       int flags)
+{
+	int *read_from_stdin = opt->value;
+	if (ctx->argc != 1)
+		return error("--%s must be at the end", opt->long_name);
+	if (flags & OPT_UNSET)
+		return error("--%s cannot be negated", opt->long_name);
+	*read_from_stdin = 1;
+	return 0;
+}
+
+static int unresolve_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,
+			int flags)
+{
+	int *has_errors = opt->value;
+	const char *prefix = startup_info->prefix;
+	*has_errors = do_unresolve(ctx->argc, ctx->argv,
+				   prefix, !prefix ? 0 : strlen(prefix));
+	ctx->argv += ctx->argc - 1;
+	ctx->argc = 1;
+	if (*has_errors)
+		active_cache_changed = 0;
+	return 0;
+}
+
+static int reupdate_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,
+		       int flags)
+{
+	int *has_errors = opt->value;
+	const char *prefix = startup_info->prefix;
+	setup_work_tree();
+	*has_errors = do_reupdate(ctx->argc, ctx->argv,
+				  prefix, !prefix ? 0 : strlen(prefix));
+	ctx->argv += ctx->argc - 1;
+	ctx->argc = 1;
+	if (*has_errors)
+		active_cache_changed = 0;
+	return 0;
+}
+
 int cmd_update_index(int argc, const char **argv, const char *prefix)
 {
-	int i, newfd, entries, has_errors = 0, line_termination = '\n';
-	int allow_options = 1;
+	int newfd, entries, has_errors = 0, line_termination = '\n';
 	int read_from_stdin = 0;
 	int prefix_length = prefix ? strlen(prefix) : 0;
 	char set_executable_bit = 0;
-	unsigned int refresh_flags = 0;
+	struct refresh_params refresh_args = {0, &has_errors};
 	int lock_error = 0;
 	struct lock_file *lock_file;
+	struct parse_opt_ctx_t ctx;
+	int parseopt_state = PARSE_OPT_UNKNOWN;
+	struct option options[] = {
+		OPT_BIT('q', NULL, &refresh_args.flags,
+			"continue refresh even when index needs update",
+			REFRESH_QUIET),
+		OPT_BIT(0, "ignore-submodules", &refresh_args.flags,
+			"refresh: ignore submodules",
+			REFRESH_IGNORE_SUBMODULES),
+		OPT_SET_INT(0, "add", &allow_add,
+			"do not ignore new files", 1),
+		OPT_SET_INT(0, "replace", &allow_replace,
+			"let files replace directories and vice-versa", 1),
+		OPT_SET_INT(0, "remove", &allow_remove,
+			"notice files missing from worktree", 1),
+		OPT_BIT(0, "unmerged", &refresh_args.flags,
+			"refresh even if index contains unmerged entries",
+			REFRESH_UNMERGED),
+		{OPTION_CALLBACK, 0, "refresh", &refresh_args, NULL,
+			"refresh stat information",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, refresh_cb},
+		{OPTION_CALLBACK, 0, "really-refresh", &refresh_args, NULL,
+			"like --refresh, but ignore assume-unchanged setting",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, really_refresh_cb},
+		{OPTION_LOWLEVEL_CALLBACK, 0, "cacheinfo", NULL,
+			"<mode> <object> <path>",
+			"add the specified entry to the index",
+			PARSE_OPT_LITERAL_ARGHELP, (parse_opt_cb *)cacheinfo_cb},
+		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, "(+/-)x",
+			"override the executable bit of the listed files",
+			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
+			chmod_arg_cb},
+		{OPTION_SET_INT, 0, "assume-unchanged", &mark_valid_only, NULL,
+			"mark files as \"assumed to be unmodified\"",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL,
+			MARK_FLAG},
+		{OPTION_SET_INT, 0, "no-assume-unchanged", &mark_valid_only, NULL,
+			"unset \"assumed unchanged\" bit",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL,
+			UNMARK_FLAG},
+		{OPTION_SET_INT, 0, "skip-worktree", &mark_skip_worktree_only, NULL,
+			"mark files as \"index-only\"",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL,
+			MARK_FLAG},
+		{OPTION_SET_INT, 0, "no-skip-worktree", &mark_skip_worktree_only, NULL,
+			"unset \"skip worktree\" big",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL,
+			UNMARK_FLAG},
+		OPT_SET_INT(0, "info-only", &info_only,
+			"add to index only; do not add content to object database", 1),
+		OPT_SET_INT(0, "force-remove", &force_remove,
+			"remove named paths even if present in worktree", 1),
+		OPT_SET_INT('z', NULL, &line_termination,
+			"with --stdin: input lines are terminated by null bytes", '\0'),
+		{OPTION_LOWLEVEL_CALLBACK, 0, "stdin", &read_from_stdin, NULL,
+			"read list of paths to be updated from standard input",
+			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
+			(parse_opt_cb *)last_arg_cb},
+		{OPTION_LOWLEVEL_CALLBACK, 0, "index-info", &line_termination, NULL,
+			"add entries from standard input to the index",
+			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
+			(parse_opt_cb *)stdin_cacheinfo_cb},
+		{OPTION_LOWLEVEL_CALLBACK, 0, "unresolve", &has_errors, NULL,
+			"repopulate stages #2 and #3 for the listed paths",
+			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
+			(parse_opt_cb *)unresolve_cb},
+		{OPTION_LOWLEVEL_CALLBACK, 'g', "again", &has_errors, NULL,
+			"only update entries that differ from HEAD",
+			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
+			(parse_opt_cb *)reupdate_cb},
+		OPT_BIT(0, "ignore-missing", &refresh_args.flags,
+			"ignore files missing from worktree",
+			REFRESH_IGNORE_MISSING),
+		OPT_SET_INT(0, "verbose", &verbose,
+			"report actions to standard output", 1),
+		{OPTION_CALLBACK, 0, "clear-resolve-undo", NULL, NULL,
+			"(for porcelains) forget saved unresolved conflicts",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, resolve_undo_clear_cb},
+		OPT_END()
+	};
 
 	git_config(git_default_config, NULL);
 
@@ -602,151 +796,49 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	if (entries < 0)
 		die("cache corrupted");
 
-	for (i = 1 ; i < argc; i++) {
-		const char *path = argv[i];
-		const char *p;
+	/*
+	 * Custom copy of parse_options() because we want to handle
+	 * filename arguments as they come.
+	 */
+	parse_options_start(&ctx, argc, argv, prefix,
+			    PARSE_OPT_STOP_AT_NON_OPTION);
+	while (ctx.argc) {
+		if (parseopt_state != PARSE_OPT_DONE)
+			parseopt_state = parse_options_step(&ctx, options,
+							    update_index_usage);
+		if (!ctx.argc)
+			break;
+		switch (parseopt_state) {
+		case PARSE_OPT_HELP:
+			exit(129);
+		case PARSE_OPT_NON_OPTION:
+		case PARSE_OPT_DONE:
+		{
+			const char *path = ctx.argv[0];
+			const char *p;
 
-		if (allow_options && *path == '-') {
-			if (!strcmp(path, "--")) {
-				allow_options = 0;
-				continue;
-			}
-			if (!strcmp(path, "-q")) {
-				refresh_flags |= REFRESH_QUIET;
-				continue;
-			}
-			if (!strcmp(path, "--ignore-submodules")) {
-				refresh_flags |= REFRESH_IGNORE_SUBMODULES;
-				continue;
-			}
-			if (!strcmp(path, "--add")) {
-				allow_add = 1;
-				continue;
-			}
-			if (!strcmp(path, "--replace")) {
-				allow_replace = 1;
-				continue;
-			}
-			if (!strcmp(path, "--remove")) {
-				allow_remove = 1;
-				continue;
-			}
-			if (!strcmp(path, "--unmerged")) {
-				refresh_flags |= REFRESH_UNMERGED;
-				continue;
-			}
-			if (!strcmp(path, "--refresh")) {
-				setup_work_tree();
-				has_errors |= refresh_cache(refresh_flags);
-				continue;
-			}
-			if (!strcmp(path, "--really-refresh")) {
-				setup_work_tree();
-				has_errors |= refresh_cache(REFRESH_REALLY | refresh_flags);
-				continue;
-			}
-			if (!strcmp(path, "--cacheinfo")) {
-				unsigned char sha1[20];
-				unsigned int mode;
-
-				if (i+3 >= argc)
-					die("git update-index: --cacheinfo <mode> <sha1> <path>");
-
-				if (strtoul_ui(argv[i+1], 8, &mode) ||
-				    get_sha1_hex(argv[i+2], sha1) ||
-				    add_cacheinfo(mode, sha1, argv[i+3], 0))
-					die("git update-index: --cacheinfo"
-					    " cannot add %s", argv[i+3]);
-				i += 3;
-				continue;
-			}
-			if (!strcmp(path, "--chmod=-x") ||
-			    !strcmp(path, "--chmod=+x")) {
-				if (argc <= i+1)
-					die("git update-index: %s <path>", path);
-				set_executable_bit = path[8];
-				continue;
-			}
-			if (!strcmp(path, "--assume-unchanged")) {
-				mark_valid_only = MARK_FLAG;
-				continue;
-			}
-			if (!strcmp(path, "--no-assume-unchanged")) {
-				mark_valid_only = UNMARK_FLAG;
-				continue;
-			}
-			if (!strcmp(path, "--no-skip-worktree")) {
-				mark_skip_worktree_only = UNMARK_FLAG;
-				continue;
-			}
-			if (!strcmp(path, "--skip-worktree")) {
-				mark_skip_worktree_only = MARK_FLAG;
-				continue;
-			}
-			if (!strcmp(path, "--info-only")) {
-				info_only = 1;
-				continue;
-			}
-			if (!strcmp(path, "--force-remove")) {
-				force_remove = 1;
-				continue;
-			}
-			if (!strcmp(path, "-z")) {
-				line_termination = 0;
-				continue;
-			}
-			if (!strcmp(path, "--stdin")) {
-				if (i != argc - 1)
-					die("--stdin must be at the end");
-				read_from_stdin = 1;
-				break;
-			}
-			if (!strcmp(path, "--index-info")) {
-				if (i != argc - 1)
-					die("--index-info must be at the end");
-				allow_add = allow_replace = allow_remove = 1;
-				read_index_info(line_termination);
-				break;
-			}
-			if (!strcmp(path, "--unresolve")) {
-				has_errors = do_unresolve(argc - i, argv + i,
-							  prefix, prefix_length);
-				if (has_errors)
-					active_cache_changed = 0;
-				goto finish;
-			}
-			if (!strcmp(path, "--again") || !strcmp(path, "-g")) {
-				setup_work_tree();
-				has_errors = do_reupdate(argc - i, argv + i,
-							 prefix, prefix_length);
-				if (has_errors)
-					active_cache_changed = 0;
-				goto finish;
-			}
-			if (!strcmp(path, "--ignore-missing")) {
-				refresh_flags |= REFRESH_IGNORE_MISSING;
-				continue;
-			}
-			if (!strcmp(path, "--verbose")) {
-				verbose = 1;
-				continue;
-			}
-			if (!strcmp(path, "--clear-resolve-undo")) {
-				resolve_undo_clear();
-				continue;
-			}
-			if (!strcmp(path, "-h") || !strcmp(path, "--help"))
-				usage(update_index_usage);
-			die("unknown option %s", path);
+			trace_printf("trace: update-index %s\n", path);
+			setup_work_tree();
+			p = prefix_path(prefix, prefix_length, path);
+			update_one(p, NULL, 0);
+			if (set_executable_bit)
+				chmod_path(set_executable_bit, p);
+			if (p < path || p > path + strlen(path))
+				free((char *)p);
+			ctx.argc--;
+			ctx.argv++;
+			break;
+		}
+		case PARSE_OPT_UNKNOWN:
+			if (ctx.argv[0][1] == '-')
+				error("unknown option '%s'", ctx.argv[0] + 2);
+			else
+				error("unknown switch '%c'", *ctx.opt);
+			usage_with_options(update_index_usage, options);
 		}
-		setup_work_tree();
-		p = prefix_path(prefix, prefix_length, path);
-		update_one(p, NULL, 0);
-		if (set_executable_bit)
-			chmod_path(set_executable_bit, p);
-		if (p < path || p > path + strlen(path))
-			free((char *)p);
 	}
+	argc = parse_options_end(&ctx);
+
 	if (read_from_stdin) {
 		struct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;
 
@@ -770,10 +862,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		strbuf_release(&buf);
 	}
 
- finish:
 	if (active_cache_changed) {
 		if (newfd < 0) {
-			if (refresh_flags & REFRESH_QUIET)
+			if (refresh_args.flags & REFRESH_QUIET)
 				exit(128);
 			unable_to_lock_index_die(get_index_file(), lock_error);
 		}
-- 
1.7.2.3

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

[Stephen Boyd]

On 10/24/10 01:18, Jonathan Nieder wrote:
> ---
>  builtin/update-index.c |  389 +++++++++++++++++++++++++++++------------------
>  1 files changed, 240 insertions(+), 149 deletions(-)
> 

I would suspect that the code would get smaller. Too many callbacks?

>  
> -static const char update_index_usage[] =
> -"git update-index [-q] [--add] [--replace] [--remove] [--unmerged] [--refresh] [--really-refresh] [--cacheinfo] [--chmod=(+|-)x] [--assume-unchanged] [--skip-worktree|--no-skip-worktree] [--info-only] [--force-remove] [--stdin] [--index-info] [--unresolve] [--again | -g] [--ignore-missing] [-z] [--verbose] [--] [<file>...]";
> +static const char * const update_index_usage[] = {
> +"git update-index [-q] [--add] [--replace] [--remove] [--unmerged] [--refresh] [--really-refresh] [--cacheinfo] [--chmod=(+|-)x] [--assume-unchanged] [--skip-worktree|--no-skip-worktree] [--info-only] [--force-remove] [--stdin] [--index-info] [--unresolve] [--again | -g] [--ignore-missing] [-z] [--verbose] [--] [<file>...]",
> +	NULL
> +};

Please drop all this extraneous option stuff since it's already shown in
the -h output. The usage should list the different command modes. I know
that I've failed to do this in the past (and I should probably fix those).

Would

	git update-index [<options>] -- <file>

be enough?


> +static int cacheinfo_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,
> +			int flags)
> +{
> +	unsigned char sha1[20];
> +	unsigned int mode;
> +	if (flags & OPT_UNSET)
> +		return error("--cacheinfo cannot be negated");

This shouldn't be possible right? I thought parse options made sure
NONEG options couldn't be negated... <goes and looks at patch 1>. Oh. It
seems like there will be a lot of duplicated code that way. Maybe we can
fixup patch 1 a bit so this isn't necessary.

> +	if (ctx->opt)
> +		return error("--cacheinfo does not accept an attached argument");

And this too.

> +	if (ctx->argc <= 3)
> +		return error("--cacheinfo requires three arguments");
> +	if (strtoul_ui(*++ctx->argv, 8, &mode) ||
> +	    get_sha1_hex(*++ctx->argv, sha1) ||
> +	    add_cacheinfo(mode, sha1, *++ctx->argv, 0))
> +		die("git update-index: --cacheinfo cannot add %s", *ctx->argv);

Hmm, it would be neat if die() always prefixed the death message with
the command in which it died.

> +	ctx->argc -= 3;
> +	return 0;
> +}
> +
> +static int stdin_cacheinfo_cb(struct parse_opt_ctx_t *ctx,
> +			      const struct option *opt, int flags)
> +{
> +	int *line_termination = opt->value;
> +	if (ctx->argc != 1 || ctx->opt)
> +		return error("--%s must be at the end", opt->long_name);
> +	if (flags & OPT_UNSET)
> +		return error("--%s cannot be negated", opt->long_name);
> +	allow_add = allow_replace = allow_remove = 1;
> +	read_index_info(*line_termination);
> +	return 0;
> +}
> +
> +static int last_arg_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,
> +		       int flags)
> +{
> +	int *read_from_stdin = opt->value;
> +	if (ctx->argc != 1)
> +		return error("--%s must be at the end", opt->long_name);

Thinking out loud, this might be better served as an option flag
(PARSE_OPT_LAST_ARG?) to make it a bit more generic. Especially since
you use it twice.

> +	if (flags & OPT_UNSET)
> +		return error("--%s cannot be negated", opt->long_name);
> +	*read_from_stdin = 1;
> +	return 0;
> +}

And then this callback would go away and you could use a custom
OPTION_SET_PTR (or probably OPTION_SET_INT) right?


> +static int reupdate_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,
> +		       int flags)
> +{
> +	int *has_errors = opt->value;
> +	const char *prefix = startup_info->prefix;

Doesn't the context also contain this? I know this is why you included
patch 3, but it doesn't seem strictly necessary to use startup_info over
ctx.

> +	setup_work_tree();
> +	*has_errors = do_reupdate(ctx->argc, ctx->argv,
> +				  prefix, !prefix ? 0 : strlen(prefix));
> +	ctx->argv += ctx->argc - 1;
> +	ctx->argc = 1;

At first I thought you forgot to make this a -= here. Then I realized
you're doing this to make parse options skip to the end of processing.
Perhaps you can just return PARSE_OPT_FINISH (equal to 1?) or something
to indicate to parse options that you're done parsing options entirely?
Or put a comment there so I don't get confused again.


> +	struct option options[] = {
> +		OPT_BIT('q', NULL, &refresh_args.flags,
> +			"continue refresh even when index needs update",
> +			REFRESH_QUIET),
[snip]
> +		OPT_SET_INT(0, "verbose", &verbose,
> +			"report actions to standard output", 1),
> +		{OPTION_CALLBACK, 0, "clear-resolve-undo", NULL, NULL,
> +			"(for porcelains) forget saved unresolved conflicts",
> +			PARSE_OPT_NOARG | PARSE_OPT_NONEG, resolve_undo_clear_cb},
> +		OPT_END()
> +	};

Any reason for OPT_SET_INT over OPT_BOOLEAN? Just curious.

> -			if (!strcmp(path, "-h") || !strcmp(path, "--help"))
> -				usage(update_index_usage);
> -			die("unknown option %s", path);
> +			trace_printf("trace: update-index %s\n", path);

Maybe useful; but doubtful considering we already get the whole command
line already. Debugging stuff?

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

[Jonathan Nieder]

Hi,

Stephen Boyd wrote:
> On 10/24/10 01:18, Jonathan Nieder wrote:

>>  builtin/update-index.c |  389 +++++++++++++++++++++++++++++------------------
>>  1 files changed, 240 insertions(+), 149 deletions(-)
>
> I would suspect that the code would get smaller. Too many callbacks?

That and added option descriptions.

>> +"git update-index [-q] [--add] [--replace] [--remove] [--unmerged] [--refresh] [--really-refresh] [--cacheinfo] [--chmod=(+|-)x] [--assume-unchanged] [--skip-worktree|--no-skip-worktree] [--info-only] [--force-remove] [--stdin] [--index-info] [--unresolve] [--again | -g] [--ignore-missing] [-z] [--verbose] [--] [<file>...]",
>> +	NULL
>> +};
>
> Please drop all this extraneous option stuff since it's already shown in
> the -h output.

Good idea.  I put

	git update-index [options] [--] [<file>...]

even though that doesn't explain the actual syntax (especially
oddities like --stdin and --unresolve) at all. :)

> This shouldn't be possible right? I thought parse options made sure
> NONEG options couldn't be negated... <goes and looks at patch 1>. Oh.

Fixed.

[...]
>> +static int last_arg_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,
>> +		       int flags)
>> +{
>> +	int *read_from_stdin = opt->value;
>> +	if (ctx->argc != 1)
>> +		return error("--%s must be at the end", opt->long_name);
>
> Thinking out loud, this might be better served as an option flag
> (PARSE_OPT_LAST_ARG?) to make it a bit more generic. Especially since
> you use it twice.

Agreed but I'm not doing it now, since I don't want to encourage other
commands.  Will reconsider.

>> +static int reupdate_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,
>> +		       int flags)
>> +{
>> +	int *has_errors = opt->value;
>> +	const char *prefix = startup_info->prefix;
>
> Doesn't the context also contain this? I know this is why you included
> patch 3, but it doesn't seem strictly necessary to use startup_info over
> ctx.

I figure patch 3 is inevitable anyway.  It didn't seem right to
peek at the context, which only includes it as an implementation
detail of OPTION_FILENAME support.

Maybe eliminating ctx->prefix would be a good follow-up patch.

>> +	setup_work_tree();
>> +	*has_errors = do_reupdate(ctx->argc, ctx->argv,
>> +				  prefix, !prefix ? 0 : strlen(prefix));
>> +	ctx->argv += ctx->argc - 1;
>> +	ctx->argc = 1;
>
> At first I thought you forgot to make this a -= here.

Yep, needs a comment.

>> +	struct option options[] = {
>> +		OPT_BIT('q', NULL, &refresh_args.flags,
>> +			"continue refresh even when index needs update",
>> +			REFRESH_QUIET),
> [snip]
>> +		OPT_SET_INT(0, "verbose", &verbose,
>> +			"report actions to standard output", 1),
>> +		{OPTION_CALLBACK, 0, "clear-resolve-undo", NULL, NULL,
>> +			"(for porcelains) forget saved unresolved conflicts",
>> +			PARSE_OPT_NOARG | PARSE_OPT_NONEG, resolve_undo_clear_cb},
>> +		OPT_END()
>> +	};
>
> Any reason for OPT_SET_INT over OPT_BOOLEAN? Just curious.

OPT_BOOLEAN means OPT_INCR and there is only one level of verbosity
here.

>> +			trace_printf("trace: update-index %s\n", path);
[...]
>               Debugging stuff?

Yes, good catch.

Thanks again for all the comments, and sorry to take so long to get
back to this.

Regards,
Jonathan

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

[Nguyen Thai Ngoc Duy]

On Sun, Oct 24, 2010 at 3:13 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Jonathan Nieder wrote:
>
>> Here's a helper to make it easier for commands that use parse-options
>> to adopt that nicer behavior.  It writes its output to stdout, so it
>> should only be used to be used to handle the -h option.
>
> Alas, "git update-index" does not use parse-options yet.  But that
> is easily enough changed...

I don't know parse-options well enough to give comments. If I did, I
would have converted update-index long ago (hit it a few times and
always ran away). Thanks for the conversion.
-- 
Duy

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jonathan Nieder wrote:
>
>> Here's a helper to make it easier for commands that use parse-options
>> to adopt that nicer behavior.  It writes its output to stdout, so it
>> should only be used to be used to handle the -h option.
>
> Alas, "git update-index" does not use parse-options yet.  But that
> is easily enough changed...
>
> Patch 1 introduces an OPTION_LOWLEVEL_CALLBACK backdoor to
> parse-options, so new option types (like the three-argument type
> used by update-index --cacheinfo) can be supported without tempting
> inventors of other commands through mention in the public API.

I like the idea of this one.  I imagine it can also be used to finally
whip the command line parsing of "log -L" series Thomas mentored into a
reasonable shape, which needs to parse the -L option that takes <range>
and an optional <path> (not <pathspec>) as arguments.

[PATCH/RFCv2 0/6] update-index: migrate to parse-options API

[Jonathan Nieder]

Here's a reroll.  Changes explained in the comments following each
patch.

Thanks to Stephen for a great deal of helpful advice.  The result
turns out to be less surgery than I thought from the version in pu,
mostly because I am trying to be conservative (meaning further
simplifications are welcome as patches on top, natch).

Based against ca20906, as before.

Jonathan Nieder (4):
  parse-options: sanity check PARSE_OPT_NOARG flag
  parse-options: allow git commands to invent new option types
  parse-options: make resuming easier after
    PARSE_OPT_STOP_AT_NON_OPTION
  update-index: migrate to parse-options API

Nguyễn Thái Ngọc Duy (1):
  setup: save prefix (original cwd relative to toplevel) in
    startup_info

Stephen Boyd (1):
  parse-options: eliminate implicit PARSE_OPT_NOARG for built-in option
    types

 builtin/update-index.c |  391 ++++++++++++++++++++++++++++++------------------
 cache.h                |    1 +
 parse-options.c        |   47 ++++---
 parse-options.h        |    9 +-
 setup.c                |    4 +-
 5 files changed, 283 insertions(+), 169 deletions(-)

[PATCH 1/6] parse-options: sanity check PARSE_OPT_NOARG flag

[Jonathan Nieder]

Some option types cannot use an argument --- boolean options that
would set a bit or flag or increment a counter, for example.  If
configured in the flag word to accept an argument anyway, the result
is an argument that is advertised in "program -h" output only to be
rejected by parse-options::get_value.

Luckily all current users of these option types use PARSE_OPT_NOARG
and do not use PARSE_OPT_OPTARG.  Add a check to ensure that that
remains true.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
New patch, preparing for patch 2.  Maybe the check should be
implemented elsewhere (ideally compile time) so all flags can be
checked rather than just the flags that happen to be used in a given
test run.

 parse-options.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 0fa79bc..1806bb3 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -44,6 +44,26 @@ static void fix_filename(const char *prefix, const char **file)
 	*file = xstrdup(prefix_filename(prefix, strlen(prefix), *file));
 }
 
+static void check_flags(const struct option *opt)
+{
+	switch (opt->type) {
+	case OPTION_BOOLEAN:
+	case OPTION_BIT:
+	case OPTION_NEGBIT:
+	case OPTION_SET_INT:
+	case OPTION_SET_PTR:
+	case OPTION_NUMBER:
+		break;
+	default:	/* (usually accepts an argument) */
+		return;
+	}
+	if ((opt->flags & (PARSE_OPT_OPTARG | PARSE_OPT_NOARG)) == PARSE_OPT_NOARG)
+		return;
+	die("BUG: option '-%c%s' should not accept an argument",
+				!opt->short_name ? '-' : opt->short_name,
+				!opt->short_name ? opt->long_name : "");
+}
+
 static int get_value(struct parse_opt_ctx_t *p,
 		     const struct option *opt, int flags)
 {
@@ -51,6 +71,8 @@ static int get_value(struct parse_opt_ctx_t *p,
 	const int unset = flags & OPT_UNSET;
 	int err;
 
+	check_flags(opt);
+
 	if (unset && p->opt)
 		return opterror(opt, "takes no value", flags);
 	if (unset && (opt->flags & PARSE_OPT_NONEG))
-- 
1.7.2.3

Re: [PATCH 1/6] parse-options: sanity check PARSE_OPT_NOARG flag

[Stephen Boyd]

On 11/29/10 18:55, Jonathan Nieder wrote:
> +static void check_flags(const struct option *opt)
> +{
> +	switch (opt->type) {
> +	case OPTION_BOOLEAN:
> +	case OPTION_BIT:
> +	case OPTION_NEGBIT:
> +	case OPTION_SET_INT:
> +	case OPTION_SET_PTR:
> +	case OPTION_NUMBER:
> +		break;
> +	default:	/* (usually accepts an argument) */
> +		return;
> +	}
> +	if ((opt->flags & (PARSE_OPT_OPTARG | PARSE_OPT_NOARG)) == PARSE_OPT_NOARG)
> +		return;
> +	die("BUG: option '-%c%s' should not accept an argument",
> +				!opt->short_name ? '-' : opt->short_name,
> +				!opt->short_name ? opt->long_name : "");
> +}
> +

This check should probably go into parse_options_check() and be run once
for each invocation of parse_options_start()... Oh that isn't good.

Looks like parse_options_check() is being called for each
parse_options_step(). Here's a patch to fix that. Junio, this can
probably be applied to maint.

---8<------>8-----
Subject: [PATCH] parse-options: Don't call parse_options_check() so much

parse_options_check() is being called for each invocation of
parse_options_step() which can be quite a bit for some commands. The
commit introducing this function cb9d398 (parse-options: add
parse_options_check to validate option specs., 2009-06-09) had the
correct motivation and explicitly states that parse_options_check()
should be called from parse_options_start(). However, the implementation
differs from the motivation. Fix it.

Signed-off-by: Stephen Boyd <bebarino@gmail.com>
---
 builtin/blame.c    |    2 +-
 builtin/shortlog.c |    2 +-
 parse-options.c    |    7 +++----
 parse-options.h    |    2 +-
 4 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index f5fccc1..19a2ebf 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2310,7 +2310,7 @@ int cmd_blame(int argc, const char **argv, const
char *prefix)
 	dashdash_pos = 0;

 	parse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |
-			    PARSE_OPT_KEEP_ARGV0);
+			    PARSE_OPT_KEEP_ARGV0, options);
 	for (;;) {
 		switch (parse_options_step(&ctx, options, blame_opt_usage)) {
 		case PARSE_OPT_HELP:
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 2135b0d..8473391 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -269,7 +269,7 @@ int cmd_shortlog(int argc, const char **argv, const
char *prefix)
 	shortlog_init(&log);
 	init_revisions(&rev, prefix);
 	parse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |
-			    PARSE_OPT_KEEP_ARGV0);
+			    PARSE_OPT_KEEP_ARGV0, options);

 	for (;;) {
 		switch (parse_options_step(&ctx, options, shortlog_usage)) {
diff --git a/parse-options.c b/parse-options.c
index 0fa79bc..196ba71 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -338,7 +338,7 @@ static void parse_options_check(const struct option
*opts)

 void parse_options_start(struct parse_opt_ctx_t *ctx,
 			 int argc, const char **argv, const char *prefix,
-			 int flags)
+			 int flags, const struct option *options)
 {
 	memset(ctx, 0, sizeof(*ctx));
 	ctx->argc = argc - 1;
@@ -350,6 +350,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
 	if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
 	    (flags & PARSE_OPT_STOP_AT_NON_OPTION))
 		die("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
+	parse_options_check(options);
 }

 static int usage_with_options_internal(struct parse_opt_ctx_t *,
@@ -362,8 +363,6 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 {
 	int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);

-	parse_options_check(options);
-
 	/* we must reset ->opt, unknown short option leave it dangling */
 	ctx->opt = NULL;

@@ -452,7 +451,7 @@ int parse_options(int argc, const char **argv, const
char *prefix,
 {
 	struct parse_opt_ctx_t ctx;

-	parse_options_start(&ctx, argc, argv, prefix, flags);
+	parse_options_start(&ctx, argc, argv, prefix, flags, options);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
 		exit(129);
diff --git a/parse-options.h b/parse-options.h
index ae8647d..828c056 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -180,7 +180,7 @@ struct parse_opt_ctx_t {

 extern void parse_options_start(struct parse_opt_ctx_t *ctx,
 				int argc, const char **argv, const char *prefix,
-				int flags);
+				int flags, const struct option *options);

 extern int parse_options_step(struct parse_opt_ctx_t *ctx,
 			      const struct option *options,
-- 
1.7.3.2.614.g03864

Re: [PATCH 1/6] parse-options: sanity check PARSE_OPT_NOARG flag

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> writes:

> Looks like parse_options_check() is being called for each
> parse_options_step(). Here's a patch to fix that. Junio, this can
> probably be applied to maint.

Looks correct, but why is it a maint material?

> ---8<------>8-----
> Subject: [PATCH] parse-options: Don't call parse_options_check() so much
>
> parse_options_check() is being called for each invocation of
> parse_options_step() which can be quite a bit for some commands. The
> commit introducing this function cb9d398 (parse-options: add
> parse_options_check to validate option specs., 2009-06-09) had the
> correct motivation and explicitly states that parse_options_check()
> should be called from parse_options_start(). However, the implementation
> differs from the motivation. Fix it.
>
> Signed-off-by: Stephen Boyd <bebarino@gmail.com>
> ---
>  builtin/blame.c    |    2 +-
>  builtin/shortlog.c |    2 +-
>  parse-options.c    |    7 +++----
>  parse-options.h    |    2 +-
>  4 files changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index f5fccc1..19a2ebf 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2310,7 +2310,7 @@ int cmd_blame(int argc, const char **argv, const
> char *prefix)
>  	dashdash_pos = 0;
>
>  	parse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |
> -			    PARSE_OPT_KEEP_ARGV0);
> +			    PARSE_OPT_KEEP_ARGV0, options);
>  	for (;;) {
>  		switch (parse_options_step(&ctx, options, blame_opt_usage)) {
>  		case PARSE_OPT_HELP:
> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index 2135b0d..8473391 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -269,7 +269,7 @@ int cmd_shortlog(int argc, const char **argv, const
> char *prefix)
>  	shortlog_init(&log);
>  	init_revisions(&rev, prefix);
>  	parse_options_start(&ctx, argc, argv, prefix, PARSE_OPT_KEEP_DASHDASH |
> -			    PARSE_OPT_KEEP_ARGV0);
> +			    PARSE_OPT_KEEP_ARGV0, options);
>
>  	for (;;) {
>  		switch (parse_options_step(&ctx, options, shortlog_usage)) {
> diff --git a/parse-options.c b/parse-options.c
> index 0fa79bc..196ba71 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -338,7 +338,7 @@ static void parse_options_check(const struct option
> *opts)
>
>  void parse_options_start(struct parse_opt_ctx_t *ctx,
>  			 int argc, const char **argv, const char *prefix,
> -			 int flags)
> +			 int flags, const struct option *options)
>  {
>  	memset(ctx, 0, sizeof(*ctx));
>  	ctx->argc = argc - 1;
> @@ -350,6 +350,7 @@ void parse_options_start(struct parse_opt_ctx_t *ctx,
>  	if ((flags & PARSE_OPT_KEEP_UNKNOWN) &&
>  	    (flags & PARSE_OPT_STOP_AT_NON_OPTION))
>  		die("STOP_AT_NON_OPTION and KEEP_UNKNOWN don't go together");
> +	parse_options_check(options);
>  }
>
>  static int usage_with_options_internal(struct parse_opt_ctx_t *,
> @@ -362,8 +363,6 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
>  {
>  	int internal_help = !(ctx->flags & PARSE_OPT_NO_INTERNAL_HELP);
>
> -	parse_options_check(options);
> -
>  	/* we must reset ->opt, unknown short option leave it dangling */
>  	ctx->opt = NULL;
>
> @@ -452,7 +451,7 @@ int parse_options(int argc, const char **argv, const
> char *prefix,
>  {
>  	struct parse_opt_ctx_t ctx;
>
> -	parse_options_start(&ctx, argc, argv, prefix, flags);
> +	parse_options_start(&ctx, argc, argv, prefix, flags, options);
>  	switch (parse_options_step(&ctx, options, usagestr)) {
>  	case PARSE_OPT_HELP:
>  		exit(129);
> diff --git a/parse-options.h b/parse-options.h
> index ae8647d..828c056 100644
> --- a/parse-options.h
> +++ b/parse-options.h
> @@ -180,7 +180,7 @@ struct parse_opt_ctx_t {
>
>  extern void parse_options_start(struct parse_opt_ctx_t *ctx,
>  				int argc, const char **argv, const char *prefix,
> -				int flags);
> +				int flags, const struct option *options);
>
>  extern int parse_options_step(struct parse_opt_ctx_t *ctx,
>  			      const struct option *options,
> -- 
> 1.7.3.2.614.g03864

Re: [PATCH 1/6] parse-options: sanity check PARSE_OPT_NOARG flag

[Stephen Boyd]

On 12/01/10 15:01, Junio C Hamano wrote:
> Stephen Boyd <bebarino@gmail.com> writes:
> 
>> Looks like parse_options_check() is being called for each
>> parse_options_step(). Here's a patch to fix that. Junio, this can
>> probably be applied to maint.
> 
> Looks correct, but why is it a maint material?
> 

Possible performance regression? Which is why its qualified with probably.

Re: [PATCH 1/6] parse-options: sanity check PARSE_OPT_NOARG flag

[Junio C Hamano]

Stephen Boyd <bebarino@gmail.com> writes:

> On 11/29/10 18:55, Jonathan Nieder wrote:
>> +static void check_flags(const struct option *opt)
>> +{
>> +	switch (opt->type) {
>> +	case OPTION_BOOLEAN:
>> +	case OPTION_BIT:
>> +	case OPTION_NEGBIT:
>> +	case OPTION_SET_INT:
>> +	case OPTION_SET_PTR:
>> +	case OPTION_NUMBER:
>> +		break;
>> +	default:	/* (usually accepts an argument) */
>> +		return;
>> +	}
>> +	if ((opt->flags & (PARSE_OPT_OPTARG | PARSE_OPT_NOARG)) == PARSE_OPT_NOARG)
>> +		return;
>> +	die("BUG: option '-%c%s' should not accept an argument",
>> +				!opt->short_name ? '-' : opt->short_name,
>> +				!opt->short_name ? opt->long_name : "");
>> +}
>> +
>
> This check should probably go into parse_options_check()...

Very good suggestion---that way we can check and get diagnosis for all the
errors, not just dying on the first one.

[PATCH 2/6] parse-options: do not infer PARSE_OPT_NOARG from option type

[Jonathan Nieder]

From: Stephen Boyd <bebarino@gmail.com>

Simplify the "takes no value" error path by relying on PARSE_OPT_NOARG
being set correctly.  That is:

 - if the PARSE_OPT_NOARG flag is set, reject --opt=value
   regardless of the option type;
 - if the PARSE_OPT_NOARG flag is unset, accept --opt=value
   regardless of the option type.

This way, the accepted usage more closely matches the usage advertised
with --help-all.

No functional change intended, since the NOARG flag is only used
with "boolean-only" option types in existing parse_options callers.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 parse-options.c |   19 ++-----------------
 1 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 1806bb3..bc92d69 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -77,23 +77,8 @@ static int get_value(struct parse_opt_ctx_t *p,
 		return opterror(opt, "takes no value", flags);
 	if (unset && (opt->flags & PARSE_OPT_NONEG))
 		return opterror(opt, "isn't available", flags);
-
-	if (!(flags & OPT_SHORT) && p->opt) {
-		switch (opt->type) {
-		case OPTION_CALLBACK:
-			if (!(opt->flags & PARSE_OPT_NOARG))
-				break;
-			/* FALLTHROUGH */
-		case OPTION_BOOLEAN:
-		case OPTION_BIT:
-		case OPTION_NEGBIT:
-		case OPTION_SET_INT:
-		case OPTION_SET_PTR:
-			return opterror(opt, "takes no value", flags);
-		default:
-			break;
-		}
-	}
+	if (!(flags & OPT_SHORT) && p->opt && (opt->flags & PARSE_OPT_NOARG))
+		return opterror(opt, "takes no value", flags);
 
 	switch (opt->type) {
 	case OPTION_BIT:
-- 
1.7.2.3

Re: [PATCH 2/6] parse-options: do not infer PARSE_OPT_NOARG from option type

[Stephen Boyd]

On 11/29/10 19:04, Jonathan Nieder wrote:
> From: Stephen Boyd <bebarino@gmail.com>
> 
> Simplify the "takes no value" error path by relying on PARSE_OPT_NOARG
> being set correctly.  That is:
> 
>  - if the PARSE_OPT_NOARG flag is set, reject --opt=value
>    regardless of the option type;
>  - if the PARSE_OPT_NOARG flag is unset, accept --opt=value
>    regardless of the option type.
> 
> This way, the accepted usage more closely matches the usage advertised
> with --help-all.
> 
> No functional change intended, since the NOARG flag is only used
> with "boolean-only" option types in existing parse_options callers.
> 

Signed-off-by: Stephen Boyd <bebarino@gmail.com>

> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---

Thanks for the nice words.

[PATCH 3/6] parse-options: allow git commands to invent new option types

[Jonathan Nieder]

parse-options provides a variety of option behaviors, including
OPTION_CALLBACK, which should take care of just about any sane
behavior.  All supported behaviors obey the following constraint:

 A --foo option can only accept (and base its behavior on)
 one argument, which would be the following command-line
 argument in the "unsticked" form.

Alas, some existing git commands have options that do not obey that
constraint.  For example, update-index --cacheinfo takes three
arguments, and update-index --resolve takes all later parameters as
arguments.

Introduces an OPTION_LOWLEVEL_CALLBACK backdoor to parse-options so
such option types can be supported without tempting inventors of other
commands through mention in the public API.  Commands can set the
callback field to a function accepting three arguments: the option
parsing context, the option itself, and a flag indicating whether the
the option was negated.  When the option is encountered, that function
is called to take over from get_value().  The return value should be
zero for success, -1 for usage errors.

Thanks to Stephen Boyd for API guidance.

Improved-by: Stephen Boyd <bebarino@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Changes from v1:

. lowlevel callbacks inherit standard get_value argument
  checking
. lowlevel callbacks cannot tailor error messages based on
  long vs short option
. OPT_UNSET and OPT_SHORT therefore do not need to be exposed
. brief mention in parse-options.h comments.

We can always tweak the API again later.  None of the examples in
update-index even pay attention to the "unset" bit.

 parse-options.c |    3 +++
 parse-options.h |    8 +++++++-
 2 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index bc92d69..38c8fd3 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -81,6 +81,9 @@ static int get_value(struct parse_opt_ctx_t *p,
 		return opterror(opt, "takes no value", flags);
 
 	switch (opt->type) {
+	case OPTION_LOWLEVEL_CALLBACK:
+		return (*(parse_opt_ll_cb *)opt->callback)(p, opt, unset);
+
 	case OPTION_BIT:
 		if (unset)
 			*(int *)opt->value &= ~opt->defval;
diff --git a/parse-options.h b/parse-options.h
index d982f0f..bd0fe16 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -17,6 +17,7 @@ enum parse_opt_type {
 	OPTION_STRING,
 	OPTION_INTEGER,
 	OPTION_CALLBACK,
+	OPTION_LOWLEVEL_CALLBACK,
 	OPTION_FILENAME
 };
 
@@ -43,6 +44,10 @@ enum parse_opt_option_flags {
 struct option;
 typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
 
+struct parse_opt_ctx_t;
+typedef int parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
+				const struct option *opt, int unset);
+
 /*
  * `type`::
  *   holds the type of the option, you must have an OPTION_END last in your
@@ -87,7 +92,8 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
  *				useful for users of OPTION_NEGBIT.
  *
  * `callback`::
- *   pointer to the callback to use for OPTION_CALLBACK.
+ *   pointer to the callback to use for OPTION_CALLBACK or
+ *   OPTION_LOWLEVEL_CALLBACK.
  *
  * `defval`::
  *   default value to fill (*->value) with for PARSE_OPT_OPTARG.
-- 
1.7.2.3

[PATCH 4/6] parse-options: make resuming easier after PARSE_OPT_STOP_AT_NON_OPTION

[Jonathan Nieder]

Introduce a PARSE_OPT_NON_OPTION state, so parse_option_step()
callers can easily distinguish between non-options and other
reasons for option parsing termination (like "--").

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
As before.

 parse-options.c |    3 ++-
 parse-options.h |    1 +
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 38c8fd3..df8299c 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -384,7 +384,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			if (parse_nodash_opt(ctx, arg, options) == 0)
 				continue;
 			if (ctx->flags & PARSE_OPT_STOP_AT_NON_OPTION)
-				break;
+				return PARSE_OPT_NON_OPTION;
 			ctx->out[ctx->cpidx++] = ctx->argv[0];
 			continue;
 		}
@@ -466,6 +466,7 @@ int parse_options(int argc, const char **argv, const char *prefix,
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
 		exit(129);
+	case PARSE_OPT_NON_OPTION:
 	case PARSE_OPT_DONE:
 		break;
 	default: /* PARSE_OPT_UNKNOWN */
diff --git a/parse-options.h b/parse-options.h
index bd0fe16..2cd23af 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -167,6 +167,7 @@ extern NORETURN void usage_msg_opt(const char *msg,
 enum {
 	PARSE_OPT_HELP = -1,
 	PARSE_OPT_DONE,
+	PARSE_OPT_NON_OPTION,
 	PARSE_OPT_UNKNOWN
 };
 
-- 
1.7.2.3

[PATCH 5/6] setup: save prefix (original cwd relative to toplevel) in startup_info

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Save the path from the original cwd to the cwd at the end of the
setup procedure in the startup_info struct introduced in e37c1329
(2010-08-05).  The value cannot vary from thread to thread anyway,
since the cwd is global.

So now in your builtin command, instead of passing prefix around,
when you want to convert a user-supplied path to a cwd-relative
path, you can use startup_info->prefix directly.

Caveat: As with the return value from setup_git_directory_gently(),
startup_info->prefix would be NULL when the original cwd is not a
subdir of the toplevel.

Longer term, this would allow the prefix to be reused when several
noncooperating functions require access to the same repository (for
example, when accessing configuration before running a builtin).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Typo in v1 log message fixed (s/wiould/would/).

 cache.h |    1 +
 setup.c |    4 +++-
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/cache.h b/cache.h
index 33decd9..222d9cf 100644
--- a/cache.h
+++ b/cache.h
@@ -1117,6 +1117,7 @@ const char *split_cmdline_strerror(int cmdline_errno);
 /* git.c */
 struct startup_info {
 	int have_repository;
+	const char *prefix;
 };
 extern struct startup_info *startup_info;
 
diff --git a/setup.c b/setup.c
index a3b76de..833db12 100644
--- a/setup.c
+++ b/setup.c
@@ -512,8 +512,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	const char *prefix;
 
 	prefix = setup_git_directory_gently_1(nongit_ok);
-	if (startup_info)
+	if (startup_info) {
 		startup_info->have_repository = !nongit_ok || !*nongit_ok;
+		startup_info->prefix = prefix;
+	}
 	return prefix;
 }
 
-- 
1.7.2.3

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

[Jonathan Nieder]

--refresh and --really-refresh accept flags (like -q) and modify
an error indicator.  It might make sense to make the error
indicator global, but just pass the flags and a pointer to the error
indicator in a struct instead.

--cacheinfo wants 3 arguments.  Use the OPTION_LOWLEVEL_CALLBACK
extension to grab them.

--assume-unchanged and --no-assume-unchanged probably should use the
OPT_UYN feature; but use a callback for now so the existing MARK_FLAG
and UNMARK_FLAG values can be used.

--stdin and --index-info are still constrained to be the last argument
(implemented using the OPTION_LOWLEVEL_CALLBACK extension).

--unresolve and --again consume all arguments that come after them
(also using OPTION_LOWLEVEL_CALLBACK).

The order of options matters.  Each path on the command line is
affected only by the options that come before it.  A custom
argument-parsing loop with parse_options_step() brings that about.

In exchange for all the fuss, we get the usual perks: support for
un-sticked options, better usage error messages, more useful -h
output, and argument parsing code that should be easier to tweak
in the future.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Lots of clarity improvements:
 - simpler usage message
 - callback names end in _callback instead of _cb
 - redundant argument checking is gone
 - some strategic use of vertical whitespace
 - error messages more consistent with opterror() output
 - wording tweaks in -h output
 - removed debug noise

That's it.  Thanks to Stephen for many useful comments that made
this possible.

Good night.
Jonathan

 builtin/update-index.c |  391 ++++++++++++++++++++++++++++++------------------
 1 files changed, 242 insertions(+), 149 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 62d9f3f..0f19891 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -10,6 +10,7 @@
 #include "builtin.h"
 #include "refs.h"
 #include "resolve-undo.h"
+#include "parse-options.h"
 
 /*
  * Default to not allowing changes to the list of files. The
@@ -397,8 +398,10 @@ static void read_index_info(int line_termination)
 	strbuf_release(&uq);
 }
 
-static const char update_index_usage[] =
-"git update-index [-q] [--add] [--replace] [--remove] [--unmerged] [--refresh] [--really-refresh] [--cacheinfo] [--chmod=(+|-)x] [--assume-unchanged] [--skip-worktree|--no-skip-worktree] [--info-only] [--force-remove] [--stdin] [--index-info] [--unresolve] [--again | -g] [--ignore-missing] [-z] [--verbose] [--] [<file>...]";
+static const char * const update_index_usage[] = {
+	"git update-index [options] [--] [<file>...]",
+	NULL
+};
 
 static unsigned char head_sha1[20];
 static unsigned char merge_head_sha1[20];
@@ -578,16 +581,210 @@ static int do_reupdate(int ac, const char **av,
 	return 0;
 }
 
+struct refresh_params {
+	unsigned int flags;
+	int *has_errors;
+};
+
+static int refresh(struct refresh_params *o, unsigned int flag)
+{
+	setup_work_tree();
+	*o->has_errors |= refresh_cache(o->flags | flag);
+	return 0;
+}
+
+static int refresh_callback(const struct option *opt,
+				const char *arg, int unset)
+{
+	return refresh(opt->value, 0);
+}
+
+static int really_refresh_callback(const struct option *opt,
+				const char *arg, int unset)
+{
+	return refresh(opt->value, REFRESH_REALLY);
+}
+
+static int chmod_callback(const struct option *opt,
+				const char *arg, int unset)
+{
+	char *flip = opt->value;
+	if ((arg[0] != '-' && arg[0] != '+') || arg[1] != 'x' || arg[2])
+		return error("option 'chmod' expects \"+x\" or \"-x\"");
+	*flip = arg[0];
+	return 0;
+}
+
+static int resolve_undo_clear_callback(const struct option *opt,
+				const char *arg, int unset)
+{
+	resolve_undo_clear();
+	return 0;
+}
+
+static int cacheinfo_callback(struct parse_opt_ctx_t *ctx,
+				const struct option *opt, int unset)
+{
+	unsigned char sha1[20];
+	unsigned int mode;
+
+	if (ctx->argc <= 3)
+		return error("option 'cacheinfo' expects three arguments");
+	if (strtoul_ui(*++ctx->argv, 8, &mode) ||
+	    get_sha1_hex(*++ctx->argv, sha1) ||
+	    add_cacheinfo(mode, sha1, *++ctx->argv, 0))
+		die("git update-index: --cacheinfo cannot add %s", *ctx->argv);
+	ctx->argc -= 3;
+	return 0;
+}
+
+static int stdin_cacheinfo_callback(struct parse_opt_ctx_t *ctx,
+			      const struct option *opt, int unset)
+{
+	int *line_termination = opt->value;
+
+	if (ctx->argc != 1)
+		return error("option '%s' must be the last argument", opt->long_name);
+	allow_add = allow_replace = allow_remove = 1;
+	read_index_info(*line_termination);
+	return 0;
+}
+
+static int stdin_callback(struct parse_opt_ctx_t *ctx,
+				const struct option *opt, int unset)
+{
+	int *read_from_stdin = opt->value;
+
+	if (ctx->argc != 1)
+		return error("option '%s' must be the last argument", opt->long_name);
+	*read_from_stdin = 1;
+	return 0;
+}
+
+static int unresolve_callback(struct parse_opt_ctx_t *ctx,
+				const struct option *opt, int flags)
+{
+	int *has_errors = opt->value;
+	const char *prefix = startup_info->prefix;
+
+	/* consume remaining arguments. */
+	*has_errors = do_unresolve(ctx->argc, ctx->argv,
+				prefix, prefix ? strlen(prefix) : 0);
+	if (*has_errors)
+		active_cache_changed = 0;
+
+	ctx->argv += ctx->argc - 1;
+	ctx->argc = 1;
+	return 0;
+}
+
+static int reupdate_callback(struct parse_opt_ctx_t *ctx,
+				const struct option *opt, int flags)
+{
+	int *has_errors = opt->value;
+	const char *prefix = startup_info->prefix;
+
+	/* consume remaining arguments. */
+	setup_work_tree();
+	*has_errors = do_reupdate(ctx->argc, ctx->argv,
+				prefix, prefix ? strlen(prefix) : 0);
+	if (*has_errors)
+		active_cache_changed = 0;
+
+	ctx->argv += ctx->argc - 1;
+	ctx->argc = 1;
+	return 0;
+}
+
 int cmd_update_index(int argc, const char **argv, const char *prefix)
 {
-	int i, newfd, entries, has_errors = 0, line_termination = '\n';
-	int allow_options = 1;
+	int newfd, entries, has_errors = 0, line_termination = '\n';
 	int read_from_stdin = 0;
 	int prefix_length = prefix ? strlen(prefix) : 0;
 	char set_executable_bit = 0;
-	unsigned int refresh_flags = 0;
+	struct refresh_params refresh_args = {0, &has_errors};
 	int lock_error = 0;
 	struct lock_file *lock_file;
+	struct parse_opt_ctx_t ctx;
+	int parseopt_state = PARSE_OPT_UNKNOWN;
+	struct option options[] = {
+		OPT_BIT('q', NULL, &refresh_args.flags,
+			"continue refresh even when index needs update",
+			REFRESH_QUIET),
+		OPT_BIT(0, "ignore-submodules", &refresh_args.flags,
+			"refresh: ignore submodules",
+			REFRESH_IGNORE_SUBMODULES),
+		OPT_SET_INT(0, "add", &allow_add,
+			"do not ignore new files", 1),
+		OPT_SET_INT(0, "replace", &allow_replace,
+			"let files replace directories and vice-versa", 1),
+		OPT_SET_INT(0, "remove", &allow_remove,
+			"notice files missing from worktree", 1),
+		OPT_BIT(0, "unmerged", &refresh_args.flags,
+			"refresh even if index contains unmerged entries",
+			REFRESH_UNMERGED),
+		{OPTION_CALLBACK, 0, "refresh", &refresh_args, NULL,
+			"refresh stat information",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
+			refresh_callback},
+		{OPTION_CALLBACK, 0, "really-refresh", &refresh_args, NULL,
+			"like --refresh, but ignore assume-unchanged setting",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
+			really_refresh_callback},
+		{OPTION_LOWLEVEL_CALLBACK, 0, "cacheinfo", NULL,
+			"<mode> <object> <path>",
+			"add the specified entry to the index",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
+			(parse_opt_cb *) cacheinfo_callback},
+		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, "(+/-)x",
+			"override the executable bit of the listed files",
+			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
+			chmod_callback},
+		{OPTION_SET_INT, 0, "assume-unchanged", &mark_valid_only, NULL,
+			"mark files as \"not changing\"",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},
+		{OPTION_SET_INT, 0, "no-assume-unchanged", &mark_valid_only, NULL,
+			"clear assumed-unchanged bit",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG},
+		{OPTION_SET_INT, 0, "skip-worktree", &mark_skip_worktree_only, NULL,
+			"mark files as \"index-only\"",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, MARK_FLAG},
+		{OPTION_SET_INT, 0, "no-skip-worktree", &mark_skip_worktree_only, NULL,
+			"clear skip-worktree bit",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, UNMARK_FLAG},
+		OPT_SET_INT(0, "info-only", &info_only,
+			"add to index only; do not add content to object database", 1),
+		OPT_SET_INT(0, "force-remove", &force_remove,
+			"remove named paths even if present in worktree", 1),
+		OPT_SET_INT('z', NULL, &line_termination,
+			"with --stdin: input lines are terminated by null bytes", '\0'),
+		{OPTION_LOWLEVEL_CALLBACK, 0, "stdin", &read_from_stdin, NULL,
+			"read list of paths to be updated from standard input",
+			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
+			(parse_opt_cb *) stdin_callback},
+		{OPTION_LOWLEVEL_CALLBACK, 0, "index-info", &line_termination, NULL,
+			"add entries from standard input to the index",
+			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
+			(parse_opt_cb *) stdin_cacheinfo_callback},
+		{OPTION_LOWLEVEL_CALLBACK, 0, "unresolve", &has_errors, NULL,
+			"repopulate stages #2 and #3 for the listed paths",
+			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
+			(parse_opt_cb *) unresolve_callback},
+		{OPTION_LOWLEVEL_CALLBACK, 'g', "again", &has_errors, NULL,
+			"only update entries that differ from HEAD",
+			PARSE_OPT_NONEG | PARSE_OPT_NOARG,
+			(parse_opt_cb *) reupdate_callback},
+		OPT_BIT(0, "ignore-missing", &refresh_args.flags,
+			"ignore files missing from worktree",
+			REFRESH_IGNORE_MISSING),
+		OPT_SET_INT(0, "verbose", &verbose,
+			"report actions to standard output", 1),
+		{OPTION_CALLBACK, 0, "clear-resolve-undo", NULL, NULL,
+			"(for porcelains) forget saved unresolved conflicts",
+			PARSE_OPT_NOARG | PARSE_OPT_NONEG,
+			resolve_undo_clear_callback},
+		OPT_END()
+	};
 
 	git_config(git_default_config, NULL);
 
@@ -602,151 +799,48 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	if (entries < 0)
 		die("cache corrupted");
 
-	for (i = 1 ; i < argc; i++) {
-		const char *path = argv[i];
-		const char *p;
+	/*
+	 * Custom copy of parse_options() because we want to handle
+	 * filename arguments as they come.
+	 */
+	parse_options_start(&ctx, argc, argv, prefix,
+			    PARSE_OPT_STOP_AT_NON_OPTION);
+	while (ctx.argc) {
+		if (parseopt_state != PARSE_OPT_DONE)
+			parseopt_state = parse_options_step(&ctx, options,
+							    update_index_usage);
+		if (!ctx.argc)
+			break;
+		switch (parseopt_state) {
+		case PARSE_OPT_HELP:
+			exit(129);
+		case PARSE_OPT_NON_OPTION:
+		case PARSE_OPT_DONE:
+		{
+			const char *path = ctx.argv[0];
+			const char *p;
 
-		if (allow_options && *path == '-') {
-			if (!strcmp(path, "--")) {
-				allow_options = 0;
-				continue;
-			}
-			if (!strcmp(path, "-q")) {
-				refresh_flags |= REFRESH_QUIET;
-				continue;
-			}
-			if (!strcmp(path, "--ignore-submodules")) {
-				refresh_flags |= REFRESH_IGNORE_SUBMODULES;
-				continue;
-			}
-			if (!strcmp(path, "--add")) {
-				allow_add = 1;
-				continue;
-			}
-			if (!strcmp(path, "--replace")) {
-				allow_replace = 1;
-				continue;
-			}
-			if (!strcmp(path, "--remove")) {
-				allow_remove = 1;
-				continue;
-			}
-			if (!strcmp(path, "--unmerged")) {
-				refresh_flags |= REFRESH_UNMERGED;
-				continue;
-			}
-			if (!strcmp(path, "--refresh")) {
-				setup_work_tree();
-				has_errors |= refresh_cache(refresh_flags);
-				continue;
-			}
-			if (!strcmp(path, "--really-refresh")) {
-				setup_work_tree();
-				has_errors |= refresh_cache(REFRESH_REALLY | refresh_flags);
-				continue;
-			}
-			if (!strcmp(path, "--cacheinfo")) {
-				unsigned char sha1[20];
-				unsigned int mode;
-
-				if (i+3 >= argc)
-					die("git update-index: --cacheinfo <mode> <sha1> <path>");
-
-				if (strtoul_ui(argv[i+1], 8, &mode) ||
-				    get_sha1_hex(argv[i+2], sha1) ||
-				    add_cacheinfo(mode, sha1, argv[i+3], 0))
-					die("git update-index: --cacheinfo"
-					    " cannot add %s", argv[i+3]);
-				i += 3;
-				continue;
-			}
-			if (!strcmp(path, "--chmod=-x") ||
-			    !strcmp(path, "--chmod=+x")) {
-				if (argc <= i+1)
-					die("git update-index: %s <path>", path);
-				set_executable_bit = path[8];
-				continue;
-			}
-			if (!strcmp(path, "--assume-unchanged")) {
-				mark_valid_only = MARK_FLAG;
-				continue;
-			}
-			if (!strcmp(path, "--no-assume-unchanged")) {
-				mark_valid_only = UNMARK_FLAG;
-				continue;
-			}
-			if (!strcmp(path, "--no-skip-worktree")) {
-				mark_skip_worktree_only = UNMARK_FLAG;
-				continue;
-			}
-			if (!strcmp(path, "--skip-worktree")) {
-				mark_skip_worktree_only = MARK_FLAG;
-				continue;
-			}
-			if (!strcmp(path, "--info-only")) {
-				info_only = 1;
-				continue;
-			}
-			if (!strcmp(path, "--force-remove")) {
-				force_remove = 1;
-				continue;
-			}
-			if (!strcmp(path, "-z")) {
-				line_termination = 0;
-				continue;
-			}
-			if (!strcmp(path, "--stdin")) {
-				if (i != argc - 1)
-					die("--stdin must be at the end");
-				read_from_stdin = 1;
-				break;
-			}
-			if (!strcmp(path, "--index-info")) {
-				if (i != argc - 1)
-					die("--index-info must be at the end");
-				allow_add = allow_replace = allow_remove = 1;
-				read_index_info(line_termination);
-				break;
-			}
-			if (!strcmp(path, "--unresolve")) {
-				has_errors = do_unresolve(argc - i, argv + i,
-							  prefix, prefix_length);
-				if (has_errors)
-					active_cache_changed = 0;
-				goto finish;
-			}
-			if (!strcmp(path, "--again") || !strcmp(path, "-g")) {
-				setup_work_tree();
-				has_errors = do_reupdate(argc - i, argv + i,
-							 prefix, prefix_length);
-				if (has_errors)
-					active_cache_changed = 0;
-				goto finish;
-			}
-			if (!strcmp(path, "--ignore-missing")) {
-				refresh_flags |= REFRESH_IGNORE_MISSING;
-				continue;
-			}
-			if (!strcmp(path, "--verbose")) {
-				verbose = 1;
-				continue;
-			}
-			if (!strcmp(path, "--clear-resolve-undo")) {
-				resolve_undo_clear();
-				continue;
-			}
-			if (!strcmp(path, "-h") || !strcmp(path, "--help"))
-				usage(update_index_usage);
-			die("unknown option %s", path);
+			setup_work_tree();
+			p = prefix_path(prefix, prefix_length, path);
+			update_one(p, NULL, 0);
+			if (set_executable_bit)
+				chmod_path(set_executable_bit, p);
+			if (p < path || p > path + strlen(path))
+				free((char *)p);
+			ctx.argc--;
+			ctx.argv++;
+			break;
+		}
+		case PARSE_OPT_UNKNOWN:
+			if (ctx.argv[0][1] == '-')
+				error("unknown option '%s'", ctx.argv[0] + 2);
+			else
+				error("unknown switch '%c'", *ctx.opt);
+			usage_with_options(update_index_usage, options);
 		}
-		setup_work_tree();
-		p = prefix_path(prefix, prefix_length, path);
-		update_one(p, NULL, 0);
-		if (set_executable_bit)
-			chmod_path(set_executable_bit, p);
-		if (p < path || p > path + strlen(path))
-			free((char *)p);
 	}
+	argc = parse_options_end(&ctx);
+
 	if (read_from_stdin) {
 		struct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;
 
@@ -770,10 +864,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 		strbuf_release(&buf);
 	}
 
- finish:
 	if (active_cache_changed) {
 		if (newfd < 0) {
-			if (refresh_flags & REFRESH_QUIET)
+			if (refresh_args.flags & REFRESH_QUIET)
 				exit(128);
 			unable_to_lock_index_die(get_index_file(), lock_error);
 		}
-- 
1.7.2.3

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

[Stephen Boyd]

On 11/29/10 19:15, Jonathan Nieder wrote:
> +		{OPTION_LOWLEVEL_CALLBACK, 0, "cacheinfo", NULL,
> +			"<mode> <object> <path>",
> +			"add the specified entry to the index",
> +			PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
> +			(parse_opt_cb *) cacheinfo_callback},

Doesn't this take arguments and thus shouldn't be marked
PARSE_OPT_NOARG? Confused.

> @@ -602,151 +799,48 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>  	if (entries < 0)
>  		die("cache corrupted");
>  
> -	for (i = 1 ; i < argc; i++) {
> -		const char *path = argv[i];
> -		const char *p;
> +	/*
> +	 * Custom copy of parse_options() because we want to handle
> +	 * filename arguments as they come.
> +	 */
> +	parse_options_start(&ctx, argc, argv, prefix,
> +			    PARSE_OPT_STOP_AT_NON_OPTION);

This will need to take options too, sorry.

[PATCH] parse-options: always show arghelp when LITERAL_ARGHELP is set

[Jonathan Nieder]

The PARSE_OPT_LITERAL_ARGHELP flag allows a program to override the
usual "<argument> for mandatory, [argument] for optional" markup
in its help message.  Extend it by allowing the usual "no text for
disallowed" to be overridden, too (for options with PARSE_OPT_NOARG |
PARSE_OPT_LITERAL_ARGHELP, which was previously an unsupported
combination).

So now a person can impose ugly usage messages like

	--refresh [--] <pathspec>...
	                      don't add, only refresh the index

but more importantly, update-index can correctly advertise

	--cacheinfo <mode> <object> <path>
	                      add the specified entry to the index

without unsetting PARSE_OPT_NOARG and making that '--cacheinfo=<mode>'
'<object>' '<path>'.

Noticed-by: Stephen Boyd <bebarino@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Stephen Boyd wrote:
> On 11/29/10 19:15, Jonathan Nieder wrote:
>> +		{OPTION_LOWLEVEL_CALLBACK, 0, "cacheinfo", NULL,
>> +			"<mode> <object> <path>",
>> +			"add the specified entry to the index",
>> +			PARSE_OPT_NOARG | PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
>> +			(parse_opt_cb *) cacheinfo_callback},
>
> Doesn't this take arguments and thus shouldn't be marked
> PARSE_OPT_NOARG? Confused.

Yes, that deserves a comment.

			PARSE_OPT_NOARG |	/* disallow sticky --cacheinfo=<mode> form */
			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,

>> @@ -602,151 +799,48 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
[...]
>> +	parse_options_start(&ctx, argc, argv, prefix,
>> +			    PARSE_OPT_STOP_AT_NON_OPTION);
>
> This will need to take options too, sorry.

That would just be a merge artifact, no? :)  Thanks for a pointer.

If all goes well, I'll reroll the series with your patch later today.

 parse-options.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index df8299c..71ebd9e 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -552,7 +552,8 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 		if (opts->type == OPTION_NUMBER)
 			pos += fprintf(outfile, "-NUM");
 
-		if (!(opts->flags & PARSE_OPT_NOARG))
+		if ((opts->flags & PARSE_OPT_LITERAL_ARGHELP) ||
+		    !(opts->flags & PARSE_OPT_NOARG))
 			pos += usage_argh(opts, outfile);
 
 		if (pos <= USAGE_OPTS_WIDTH)
-- 
1.7.2.3

[PATCH 2/7] checkout-index -h: show usage even in an invalid repository

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

checkout-index loads the index before parsing options.  Erroring out
is counterproductive at that point if the operator is hunting for a
command to recover useful data from the broken repository.

[jn: new commit message, tests]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/checkout-index.c        |    3 +++
 t/t2006-checkout-index-basic.sh |   24 ++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)
 create mode 100755 t/t2006-checkout-index-basic.sh

diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index a7a5ee1..3bf3422 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -241,6 +241,9 @@ int cmd_checkout_index(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage_with_options(builtin_checkout_index_usage,
+				   builtin_checkout_index_options);
 	git_config(git_default_config, NULL);
 	state.base_dir = "";
 	prefix_length = prefix ? strlen(prefix) : 0;
diff --git a/t/t2006-checkout-index-basic.sh b/t/t2006-checkout-index-basic.sh
new file mode 100755
index 0000000..b855983
--- /dev/null
+++ b/t/t2006-checkout-index-basic.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+
+test_description='basic checkout-index tests
+'
+
+. ./test-lib.sh
+
+test_expect_success 'checkout-index --gobbledegook' '
+	test_expect_code 129 git checkout-index --gobbledegook 2>err &&
+	grep "[Uu]sage" err
+'
+
+test_expect_success 'checkout-index -h in broken repository' '
+	mkdir broken &&
+	(
+		cd broken &&
+		git init &&
+		>.git/index &&
+		test_expect_code 129 git checkout-index -h >usage 2>&1
+	) &&
+	grep "[Uu]sage" broken/usage
+'
+
+test_done
-- 
1.7.2.3

[PATCH 3/7] commit/status -h: show usage even with broken configuration

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

"git status" and "git commit" read .git/config and .gitmodules before
parsing options, but there is no reason to access a repository at all
when the caller just wanted to know what arguments are accepted.

[jn: rewrote the log message and added test]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/commit.c  |    6 ++++++
 t/t7508-status.sh |   24 ++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 66fdd22..0abb430 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1070,6 +1070,9 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		OPT_END(),
 	};
 
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage_with_options(builtin_status_usage, builtin_status_options);
+
 	if (null_termination && status_format == STATUS_FORMAT_LONG)
 		status_format = STATUS_FORMAT_PORCELAIN;
 
@@ -1255,6 +1258,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	int allow_fast_forward = 1;
 	struct wt_status s;
 
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage_with_options(builtin_commit_usage, builtin_commit_options);
+
 	wt_status_prepare(&s);
 	git_config(git_commit_config, &s);
 	in_merge = file_exists(git_path("MERGE_HEAD"));
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index c9300f3..beaae94 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -7,6 +7,30 @@ test_description='git status'
 
 . ./test-lib.sh
 
+test_expect_success 'status -h in broken repository' '
+	mkdir broken &&
+	test_when_finished "rm -fr broken" &&
+	(
+		cd broken &&
+		git init &&
+		echo "[status] showuntrackedfiles = CORRUPT" >>.git/config &&
+		test_expect_code 129 git status -h >usage 2>&1
+	) &&
+	grep "[Uu]sage" broken/usage
+'
+
+test_expect_success 'commit -h in broken repository' '
+	mkdir broken &&
+	test_when_finished "rm -fr broken" &&
+	(
+		cd broken &&
+		git init &&
+		echo "[status] showuntrackedfiles = CORRUPT" >>.git/config &&
+		test_expect_code 129 git commit -h >usage 2>&1
+	) &&
+	grep "[Uu]sage" broken/usage
+'
+
 test_expect_success 'setup' '
 	: >tracked &&
 	: >modified &&
-- 
1.7.2.3

[PATCH 4/7] gc -h: show usage even with broken configuration

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Given a request for command-line usage information rather than some
more substantial action, the only friendly thing to do is to report
the usage information as soon as possible and exit.

Without this change, as "git gc" glances over the repository, it can
be distracted by the desire to report a malformed configuration file.

Noticed while working through reports from Duy's repository access
checker.

[jn: with rewritten log message and tests]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/gc.c  |    3 +++
 t/t6500-gc.sh |   28 ++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 0 deletions(-)
 create mode 100755 t/t6500-gc.sh

diff --git a/builtin/gc.c b/builtin/gc.c
index c304638..93deed5 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -189,6 +189,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage_with_options(builtin_gc_usage, builtin_gc_options);
+
 	git_config(gc_config, NULL);
 
 	if (pack_refs < 0)
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
new file mode 100755
index 0000000..82f3639
--- /dev/null
+++ b/t/t6500-gc.sh
@@ -0,0 +1,28 @@
+#!/bin/sh
+
+test_description='basic git gc tests
+'
+
+. ./test-lib.sh
+
+test_expect_success 'gc empty repository' '
+	git gc
+'
+
+test_expect_success 'gc --gobbledegook' '
+	test_expect_code 129 git gc --nonsense 2>err &&
+	grep "[Uu]sage: git gc" err
+'
+
+test_expect_success 'gc -h with invalid configuration' '
+	mkdir broken &&
+	(
+		cd broken &&
+		git init &&
+		echo "[gc] pruneexpire = CORRUPT" >>.git/config &&
+		test_expect_code 129 git gc -h >usage 2>&1
+	) &&
+	grep "[Uu]sage" broken/usage
+'
+
+test_done
-- 
1.7.2.3

[PATCH 5/7] ls-files -h: show usage even with corrupt index

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Part of a campaign to avoid git <command> -h being distracted by
access to the repository.  A caller hoping to use "git ls-files"
with an alternate index as part of a repair operation may well use
"git ls-files -h" to show usage while planning it out.

[jn: with rewritten log message and tests]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/ls-files.c        |    3 +++
 t/t3004-ls-files-basic.sh |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 0 deletions(-)
 create mode 100755 t/t3004-ls-files-basic.sh

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index bb4f612..87f0b8a 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -530,6 +530,9 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 		OPT_END()
 	};
 
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage_with_options(ls_files_usage, builtin_ls_files_options);
+
 	memset(&dir, 0, sizeof(dir));
 	prefix = cmd_prefix;
 	if (prefix)
diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh
new file mode 100755
index 0000000..490e052
--- /dev/null
+++ b/t/t3004-ls-files-basic.sh
@@ -0,0 +1,39 @@
+#!/bin/sh
+
+test_description='basic ls-files tests
+
+This test runs git ls-files with various unusual or malformed
+command-line arguments.
+'
+
+. ./test-lib.sh
+
+>empty
+
+test_expect_success 'ls-files in empty repository' '
+	git ls-files >actual &&
+	test_cmp empty actual
+'
+
+test_expect_success 'ls-files with nonexistent path' '
+	git ls-files doesnotexist >actual &&
+	test_cmp empty actual
+'
+
+test_expect_success 'ls-files with nonsense option' '
+	test_expect_code 129 git ls-files --nonsense 2>actual &&
+	grep "[Uu]sage: git ls-files" actual
+'
+
+test_expect_success 'ls-files -h in corrupt repository' '
+	mkdir broken &&
+	(
+		cd broken &&
+		git init &&
+		>.git/index &&
+		test_expect_code 129 git ls-files -h >usage 2>&1
+	) &&
+	grep "[Uu]sage: git ls-files " broken/usage
+'
+
+test_done
-- 
1.7.2.3

Re: [PATCH 5/7] ls-files -h: show usage even with corrupt index

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> +test_expect_success 'ls-files with nonexistent path' '
> +	git ls-files doesnotexist >actual &&
> +	test_cmp empty actual
> +'

This technically is not "nonexistent path" but is "a pathspec that matches
nothing".

[PATCH 6/7] merge -h: show usage even with corrupt index

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Part of a campaign to make sure "git <command> -h" works correctly
when run from distractingly bad repositories.

[jn: with rewritten log message and tests]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/merge.c  |    2 ++
 t/t7600-merge.sh |   11 +++++++++++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 5f65c0c..584c94f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -909,6 +909,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	const char *best_strategy = NULL, *wt_strategy = NULL;
 	struct commit_list **remotes = &remoteheads;
 
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage_with_options(builtin_merge_usage, builtin_merge_options);
 	if (read_cache_unmerged()) {
 		die_resolve_conflict("merge");
 	}
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index b4f40e4..b147a1b 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -144,6 +144,17 @@ test_expect_success 'test option parsing' '
 	test_must_fail git merge
 '
 
+test_expect_success 'merge -h with invalid index' '
+	mkdir broken &&
+	(
+		cd broken &&
+		git init &&
+		>.git/index &&
+		test_expect_code 129 git merge -h 2>usage
+	) &&
+	grep "[Uu]sage: git merge" broken/usage
+'
+
 test_expect_success 'reject non-strategy with a git-merge-foo name' '
 	test_must_fail git merge -s index c1
 '
-- 
1.7.2.3

Re: [PATCH 6/7] merge -h: show usage even with corrupt index

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> +test_expect_success 'merge -h with invalid index' '

You say "in broken repository" in 1-3, "with invalid configuration" in 4,
"in corrupt repository" in 5, "with invalid index" here, and "with corrupt
index" in 7.

> +	mkdir broken &&
> +	(
> +		cd broken &&
> +		git init &&
> +		>.git/index &&

I think describing which aspect of brokenness the test is interested in on
the title line is better; let's restate "in broken/corrupt repository"
(1-3, 5) to be more specific, and match others in terminology.  E.g.

 1. corrupt ref
 2. corrupt index
 3. corrupt status.showuntrackedfiles config
 4. corrupt gc.pruneexpire config
 5. corrupt index
 6. corrupt index
 7. corrupt index

[PATCH 7/7] update-index -h: show usage even with corrupt index

[Jonathan Nieder]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

When trying to fix up a corrupt repository, one might prefer that
"update-index -h" print an accurate usage message and exit rather
than reading the repository and complaining about the corruption.

[jn: with rewritten log message and tests]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/update-index.c        |    3 +++
 t/t2107-update-index-basic.sh |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 35 insertions(+), 0 deletions(-)
 create mode 100755 t/t2107-update-index-basic.sh

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 3ab214d..a41d6d7 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -589,6 +589,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 	int lock_error = 0;
 	struct lock_file *lock_file;
 
+	if (argc == 2 && !strcmp(argv[1], "-h"))
+		usage(update_index_usage);
+
 	git_config(git_default_config, NULL);
 
 	/* We can't free this memory, it becomes part of a linked list parsed atexit() */
diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh
new file mode 100755
index 0000000..33f8be0
--- /dev/null
+++ b/t/t2107-update-index-basic.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description='basic update-index tests
+
+Tests for command-line parsing and basic operation.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'update-index --nonsense fails' '
+	test_must_fail git update-index --nonsense 2>msg &&
+	cat msg &&
+	test -s msg
+'
+
+test_expect_failure 'update-index --nonsense dumps usage' '
+	test_expect_code 129 git update-index --nonsense 2>err &&
+	grep "[Uu]sage: git update-index" err
+'
+
+test_expect_success 'update-index -h with corrupt index' '
+	mkdir broken &&
+	(
+		cd broken &&
+		git init &&
+		>.git/index &&
+		test_expect_code 129 git update-index -h >usage 2>&1
+	) &&
+	grep "[Uu]sage: git update-index" broken/usage
+'
+
+test_done
-- 
1.7.2.3

[PATCH en/and-cascade-tests 0/7] avoid repository access during "git <foo> -h"

[Jonathan Nieder]

Jonathan Nieder wrote:

> [Subject: [PATCH en/and-cascade-tests 0/7] ]

Hmph, teaches me to send a series at night time.  The topic is
printing usage information early to avoid repository access during
"git <foo> -h".

Sorry, I should have prepared and sent that earlier.  Hope it
helps still.

Re: [PATCH en/and-cascade-tests 0/7]

[Nguyen Thai Ngoc Duy]

2010/10/22 Jonathan Nieder <jrnieder@gmail.com>:
> Nguyễn Thái Ngọc Duy (7):
>  branch -h: show usage even in an invalid repository
>  checkout-index -h: show usage even in an invalid repository
>  commit/status -h: show usage even with broken configuration
>  gc -h: show usage even with broken configuration
>  ls-files -h: show usage even with corrupt index
>  merge -h: show usage even with corrupt index
>  update-index -h: show usage even with corrupt index

You should take the credit. I bet my code change does not take as much
time as your writing tests. Nice tests by the way. Inspiring.
-- 
Duy