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 <redacted>
---
Second try. Better tests.
environment.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
@@ -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);
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 <redacted>
---
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
@@ -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.logtest_must_failgit$2-h&&+test\$exit_code=129&&+!grep'defaults to'$2.log+"+}++test_helpfailurebranch+test_helpfailurecheckout-index+test_helpfailurecommit+test_helpfailuregc+test_helpfailurels-files+test_helpfailuremerge+test_helpfailureupdate-index+test_helpfailureupload-archive++test_done
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 <redacted>
---
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(-)
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 <redacted>
---
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(-)
@@ -589,6 +589,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)intlock_error=0;structlock_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() */
From: Junio C Hamano <hidden> Date: 2016-06-15 22:49:50
Nguyễn Thái Ngọc Duy [off-list ref] 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.
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
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
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:50
From: Nguyễn Thái Ngọc Duy <redacted>
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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/branch.c | 3 +++
t/t3200-branch.sh | 11 +++++++++++
2 files changed, 14 insertions(+), 0 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:50
From: Nguyễn Thái Ngọc Duy <redacted>
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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
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
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:50
From: Nguyễn Thái Ngọc Duy <redacted>
"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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/commit.c | 6 ++++++
t/t7508-status.sh | 24 ++++++++++++++++++++++++
2 files changed, 30 insertions(+), 0 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:50
From: Nguyễn Thái Ngọc Duy <redacted>
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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/gc.c | 3 +++
t/t6500-gc.sh | 28 ++++++++++++++++++++++++++++
2 files changed, 31 insertions(+), 0 deletions(-)
create mode 100755 t/t6500-gc.sh
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:50
From: Nguyễn Thái Ngọc Duy <redacted>
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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
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
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:50
From: Nguyễn Thái Ngọc Duy <redacted>
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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/merge.c | 2 ++
t/t7600-merge.sh | 11 +++++++++++
2 files changed, 13 insertions(+), 0 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:50
From: Nguyễn Thái Ngọc Duy <redacted>
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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
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
@@ -589,6 +589,9 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)intlock_error=0;structlock_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() */
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:50
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.
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
@@ -26,6 +26,17 @@ test_expect_success \!test-f.git/refs/heads/--help'+test_expect_success'branch -h in broken repository''+mkdirbroken&&+(+cdbroken&&+gitinit&&+>.git/refs/heads/master&&+test_expect_code129gitbranch-h>usage2>&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.
From: Junio C Hamano <hidden> Date: 2016-06-15 22:49:51
Jonathan Nieder [off-list ref] 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.
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
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:51
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 <redacted>
---
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:51
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
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:52
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 <redacted>
---
parse-options.c | 6 +++---
parse-options.h | 9 +++++++++
2 files changed, 12 insertions(+), 3 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:52
From: Nguyễn Thái Ngọc Duy <redacted>
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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
cache.h | 1 +
setup.c | 4 +++-
2 files changed, 4 insertions(+), 1 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:49:52
--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 <redacted>
---
builtin/update-index.c | 389 +++++++++++++++++++++++++++++------------------
1 files changed, 240 insertions(+), 149 deletions(-)
@@ -578,16 +581,207 @@ static int do_reupdate(int ac, const char **av,return0;}+structrefresh_params{+unsignedintflags;+int*has_errors;+};++staticintrefresh_cb_1(conststructoption*opt,constchar*arg,intunset,+unsignedintflag)+{+structrefresh_params*o=opt->value;+setup_work_tree();+*o->has_errors|=refresh_cache(o->flags|flag);+return0;+}++staticintrefresh_cb(conststructoption*opt,constchar*arg,intunset)+{+returnrefresh_cb_1(opt,arg,unset,0);+}++staticintreally_refresh_cb(conststructoption*opt,constchar*arg,intunset)+{+returnrefresh_cb_1(opt,arg,unset,REFRESH_REALLY);+}++staticintchmod_arg_cb(conststructoption*opt,constchar*arg,intunset)+{+char*set_executable_bit=opt->value;+if((arg[0]!='-'&&arg[0]!='+')||arg[1]!='x'||arg[2])+returnerror("argument to --chmod must be +x or -x");+*set_executable_bit=arg[0];+return0;+}++staticintresolve_undo_clear_cb(conststructoption*opt,constchar*arg,intunset)+{+resolve_undo_clear();+return0;+}++staticintcacheinfo_cb(structparse_opt_ctx_t*ctx,conststructoption*opt,+intflags)+{+unsignedcharsha1[20];+unsignedintmode;+if(flags&OPT_UNSET)+returnerror("--cacheinfo cannot be negated");+if(ctx->opt)+returnerror("--cacheinfo does not accept an attached argument");+if(ctx->argc<=3)+returnerror("--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;+return0;+}++staticintstdin_cacheinfo_cb(structparse_opt_ctx_t*ctx,+conststructoption*opt,intflags)+{+int*line_termination=opt->value;+if(ctx->argc!=1||ctx->opt)+returnerror("--%s must be at the end",opt->long_name);+if(flags&OPT_UNSET)+returnerror("--%s cannot be negated",opt->long_name);+allow_add=allow_replace=allow_remove=1;+read_index_info(*line_termination);+return0;+}++staticintlast_arg_cb(structparse_opt_ctx_t*ctx,conststructoption*opt,+intflags)+{+int*read_from_stdin=opt->value;+if(ctx->argc!=1)+returnerror("--%s must be at the end",opt->long_name);+if(flags&OPT_UNSET)+returnerror("--%s cannot be negated",opt->long_name);+*read_from_stdin=1;+return0;+}++staticintunresolve_cb(structparse_opt_ctx_t*ctx,conststructoption*opt,+intflags)+{+int*has_errors=opt->value;+constchar*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;+return0;+}++staticintreupdate_cb(structparse_opt_ctx_t*ctx,conststructoption*opt,+intflags)+{+int*has_errors=opt->value;+constchar*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;+return0;+}+intcmd_update_index(intargc,constchar**argv,constchar*prefix){-inti,newfd,entries,has_errors=0,line_termination='\n';-intallow_options=1;+intnewfd,entries,has_errors=0,line_termination='\n';intread_from_stdin=0;intprefix_length=prefix?strlen(prefix):0;charset_executable_bit=0;-unsignedintrefresh_flags=0;+structrefresh_paramsrefresh_args={0,&has_errors};intlock_error=0;structlock_file*lock_file;+structparse_opt_ctx_tctx;+intparseopt_state=PARSE_OPT_UNKNOWN;+structoptionoptions[]={+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++){-constchar*path=argv[i];-constchar*p;+/*+*Customcopyofparse_options()becausewewanttohandle+*filenameargumentsastheycome.+*/+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){+casePARSE_OPT_HELP:+exit(129);+casePARSE_OPT_NON_OPTION:+casePARSE_OPT_DONE:+{+constchar*path=ctx.argv[0];+constchar*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")){-unsignedcharsha1[20];-unsignedintmode;--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;-gotofinish;-}-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;-gotofinish;-}-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;+}+casePARSE_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){structstrbufbuf=STRBUF_INIT,nbuf=STRBUF_INIT;
On Sun, Oct 24, 2010 at 3:13 PM, Jonathan Nieder [off-list ref] wrote:
Jonathan Nieder wrote:
quoted
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
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");
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.
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),
@@ -51,6 +48,9 @@ static int get_value(struct parse_opt_ctx_t *p,constintunset=flags&OPT_UNSET;interr;+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).
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:07
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.
[...]
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.
[...]
quoted
+static int last_arg_cb(struct parse_opt_ctx_t *ctx, const struct option *opt,
+ int flags)
+{
+ int *read_from_stdin = opt->value;
+ if (ctx->argc != 1)
+ return error("--%s must be at the end", opt->long_name);
Thinking out loud, this might be better served as an option flag
(PARSE_OPT_LAST_ARG?) to make it a bit more generic. Especially since
you use it twice.
Agreed but I'm not doing it now, since I don't want to encourage other
commands. Will reconsider.
quoted
+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.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:08
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(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:08
From: Stephen Boyd <redacted>
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 <redacted>
---
parse-options.c | 19 ++-----------------
1 files changed, 2 insertions(+), 17 deletions(-)
@@ -77,23 +77,8 @@ static int get_value(struct parse_opt_ctx_t *p,returnopterror(opt,"takes no value",flags);if(unset&&(opt->flags&PARSE_OPT_NONEG))returnopterror(opt,"isn't available",flags);--if(!(flags&OPT_SHORT)&&p->opt){-switch(opt->type){-caseOPTION_CALLBACK:-if(!(opt->flags&PARSE_OPT_NOARG))-break;-/* FALLTHROUGH */-caseOPTION_BOOLEAN:-caseOPTION_BIT:-caseOPTION_NEGBIT:-caseOPTION_SET_INT:-caseOPTION_SET_PTR:-returnopterror(opt,"takes no value",flags);-default:-break;-}-}+if(!(flags&OPT_SHORT)&&p->opt&&(opt->flags&PARSE_OPT_NOARG))+returnopterror(opt,"takes no value",flags);switch(opt->type){caseOPTION_BIT:
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:08
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 <redacted>
---
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(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:08
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 [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
---
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(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:08
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 <redacted>
---
As before.
parse-options.c | 3 ++-
parse-options.h | 1 +
2 files changed, 3 insertions(+), 1 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:08
From: Nguyễn Thái Ngọc Duy <redacted>
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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
Typo in v1 log message fixed (s/wiould/would/).
cache.h | 1 +
setup.c | 4 +++-
2 files changed, 4 insertions(+), 1 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:08
--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 <redacted>
---
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(-)
@@ -578,16 +581,210 @@ static int do_reupdate(int ac, const char **av,return0;}+structrefresh_params{+unsignedintflags;+int*has_errors;+};++staticintrefresh(structrefresh_params*o,unsignedintflag)+{+setup_work_tree();+*o->has_errors|=refresh_cache(o->flags|flag);+return0;+}++staticintrefresh_callback(conststructoption*opt,+constchar*arg,intunset)+{+returnrefresh(opt->value,0);+}++staticintreally_refresh_callback(conststructoption*opt,+constchar*arg,intunset)+{+returnrefresh(opt->value,REFRESH_REALLY);+}++staticintchmod_callback(conststructoption*opt,+constchar*arg,intunset)+{+char*flip=opt->value;+if((arg[0]!='-'&&arg[0]!='+')||arg[1]!='x'||arg[2])+returnerror("option 'chmod' expects \"+x\" or \"-x\"");+*flip=arg[0];+return0;+}++staticintresolve_undo_clear_callback(conststructoption*opt,+constchar*arg,intunset)+{+resolve_undo_clear();+return0;+}++staticintcacheinfo_callback(structparse_opt_ctx_t*ctx,+conststructoption*opt,intunset)+{+unsignedcharsha1[20];+unsignedintmode;++if(ctx->argc<=3)+returnerror("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;+return0;+}++staticintstdin_cacheinfo_callback(structparse_opt_ctx_t*ctx,+conststructoption*opt,intunset)+{+int*line_termination=opt->value;++if(ctx->argc!=1)+returnerror("option '%s' must be the last argument",opt->long_name);+allow_add=allow_replace=allow_remove=1;+read_index_info(*line_termination);+return0;+}++staticintstdin_callback(structparse_opt_ctx_t*ctx,+conststructoption*opt,intunset)+{+int*read_from_stdin=opt->value;++if(ctx->argc!=1)+returnerror("option '%s' must be the last argument",opt->long_name);+*read_from_stdin=1;+return0;+}++staticintunresolve_callback(structparse_opt_ctx_t*ctx,+conststructoption*opt,intflags)+{+int*has_errors=opt->value;+constchar*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;+return0;+}++staticintreupdate_callback(structparse_opt_ctx_t*ctx,+conststructoption*opt,intflags)+{+int*has_errors=opt->value;+constchar*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;+return0;+}+intcmd_update_index(intargc,constchar**argv,constchar*prefix){-inti,newfd,entries,has_errors=0,line_termination='\n';-intallow_options=1;+intnewfd,entries,has_errors=0,line_termination='\n';intread_from_stdin=0;intprefix_length=prefix?strlen(prefix):0;charset_executable_bit=0;-unsignedintrefresh_flags=0;+structrefresh_paramsrefresh_args={0,&has_errors};intlock_error=0;structlock_file*lock_file;+structparse_opt_ctx_tctx;+intparseopt_state=PARSE_OPT_UNKNOWN;+structoptionoptions[]={+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++){-constchar*path=argv[i];-constchar*p;+/*+*Customcopyofparse_options()becausewewanttohandle+*filenameargumentsastheycome.+*/+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){+casePARSE_OPT_HELP:+exit(129);+casePARSE_OPT_NON_OPTION:+casePARSE_OPT_DONE:+{+constchar*path=ctx.argv[0];+constchar*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")){-unsignedcharsha1[20];-unsignedintmode;--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;-gotofinish;-}-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;-gotofinish;-}-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;+}+casePARSE_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){structstrbufbuf=STRBUF_INIT,nbuf=STRBUF_INIT;
From: Stephen Boyd <hidden> Date: 2016-06-15 22:50:08
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 <redacted>
---
builtin/blame.c | 2 +-
builtin/shortlog.c | 2 +-
parse-options.c | 7 +++----
parse-options.h | 2 +-
4 files changed, 6 insertions(+), 7 deletions(-)
From: Stephen Boyd <hidden> Date: 2016-06-15 22:50:08
On 11/29/10 19:04, Jonathan Nieder wrote:
From: Stephen Boyd <redacted>
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.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:50:08
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 [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
---
Stephen Boyd wrote:
On 11/29/10 19:15, Jonathan Nieder wrote:
quoted
+ {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,
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(-)