# [PATCH v12 3/5] parse-options.c: make OPTION_COUNTUP respect "unspecified" values 36 messages from 2016-04-02 to 2016-04-12. Participants: Pranit Bauva, Eric Sunshine, Duy Nguyen, Junio C Hamano. Thread: https://gitlist.dev/t/41906 ## Pranit Bauva, 2016-04-02 23:33 Subject: [PATCH v12 3/5] parse-options.c: make OPTION_COUNTUP respect "unspecified" values Message-ID: <01020153d952be68-ee055619-664d-4502-92e9-6aa524e2354b-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020153d952be68-ee055619-664d-4502-92e9-6aa524e2354b-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153d952bd99-d3812bd6-d189-4780-ab48-f015696e9cf0-000000@eu-west-1.amazonses.com> ``` The reason to make it respect "unspecified" values is to give the ability to differentiate whether `--option` or `--no-option` was specified at all. "unspecified" values should be in the form of negative values. If initial value is set to negative and `--option` specified then it will reflect the number of occurrences (counting done from 0), if `--no-option` is specified then it will reflect 0 and if nothing at all is given then it will retain its negative value. This change will not affect existing users of COUNTUP, because they all use the initial value of 0 (or more). Note that builtin/clean.c initializes the variable used with OPT__FORCE (which uses OPT_COUNTUP()) to a negative value, but it is set to either 0 or 1 by reading the configuration before the code calls parse_options(), i.e. as far as parse_options() is concerned, the initial value of the variable is not negative. To test this behavior "verbose" is set to "unspecified" while quiet is set to 0 which will test the new behavior with all sets of values. Helped-by: Jeff King Helped-by: Eric Sunshine Helped-by: Junio C Hamano Signed-off-by: Pranit Bauva --- The discussion about this patch: [1] : http://thread.gmane.org/gmane.comp.version-control.git/289027 Previous version of the patch: [v11] : http://thread.gmane.org/gmane.comp.version-control.git/288820 [v10] : http://thread.gmane.org/gmane.comp.version-control.git/288820 [v9] : http://thread.gmane.org/gmane.comp.version-control.git/288820 [v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061 Changes wrt previous version (v11): - Use bits of commit message provided by Junio. Please Note: The diff might seem improper especially the part where I have introduced some continuous lines but this is a logical error by git diff (nothing could be done about it) and thus the changes will be clearly visible with the original file itself. --- Documentation/technical/api-parse-options.txt | 8 ++++-- parse-options.c | 2 ++ t/t0040-parse-options.sh | 39 ++++++++++++++++++++------- test-parse-options.c | 3 ++- 4 files changed, 39 insertions(+), 13 deletions(-) diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt index 5f0757d..8908bf7 100644 --- a/Documentation/technical/api-parse-options.txt +++ b/Documentation/technical/api-parse-options.txt @@ -144,8 +144,12 @@ There are some macros to easily define options: `OPT_COUNTUP(short, long, &int_var, description)`:: Introduce a count-up option. - `int_var` is incremented on each use of `--option`, and - reset to zero with `--no-option`. + Each use of `--option` increments `int_var`, starting from zero + (even if initially negative), and `--no-option` resets it to + zero. To determine if `--option` or `--no-option` was set at + all, set `int_var` to a negative value, and if it is still + negative after parse_options(), then neither `--option` nor + `--no-option` was seen. `OPT_BIT(short, long, &int_var, description, mask)`:: Introduce a boolean option. diff --git a/parse-options.c b/parse-options.c index 47a9192..312a85d 100644 --- a/parse-options.c +++ b/parse-options.c @@ -110,6 +110,8 @@ static int get_value(struct parse_opt_ctx_t *p, return 0; case OPTION_COUNTUP: + if (*(int *)opt->value < 0) + *(int *)opt->value = 0; *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; return 0; diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index 302c315..bfd8dea 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -63,7 +63,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -211,7 +211,7 @@ magnitude: 0 timestamp: 0 string: 123 abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -234,7 +234,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -263,7 +263,7 @@ magnitude: 0 timestamp: 0 string: 123 abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -302,7 +302,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -322,7 +322,7 @@ magnitude: 0 timestamp: 1 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 1 dry run: no file: (not set) @@ -344,7 +344,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -373,7 +373,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -398,7 +398,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -429,7 +429,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -454,6 +454,25 @@ dry run: no file: (not set) EOF +test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' ' + test-parse-options --no-verbose >output 2>output.err && + test_must_be_empty output.err && + test_cmp expect output +' + +cat >expect <output 2>output.err && test_must_be_empty output.err && diff --git a/test-parse-options.c b/test-parse-options.c index 86afa98..f02c275 100644 --- a/test-parse-options.c +++ b/test-parse-options.c @@ -7,7 +7,8 @@ static int integer = 0; static unsigned long magnitude = 0; static unsigned long timestamp; static int abbrev = 7; -static int verbose = 0, dry_run = 0, quiet = 0; +static int verbose = -1; /* unspecified */ +static int dry_run = 0, quiet = 0; static char *string = NULL; static char *file = NULL; static int ambiguous; -- https://github.com/git/git/pull/218 ``` ## Pranit Bauva, 2016-04-02 23:33 Subject: [PATCH v12 2/5] test-parse-options: print quiet as integer Message-ID: <01020153d952be64-9ca893f1-5a7b-4a2c-a0bd-98abf93a17ac-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020153d952be64-9ca893f1-5a7b-4a2c-a0bd-98abf93a17ac-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153d952bd99-d3812bd6-d189-4780-ab48-f015696e9cf0-000000@eu-west-1.amazonses.com> ``` Current implementation of parse-options.c treats OPT__QUIET() as integer and not boolean and thus it is more appropriate to print it as integer to avoid confusion. Signed-off-by: Pranit Bauva --- t/t0040-parse-options.sh | 26 +++++++++++++------------- test-parse-options.c | 2 +- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index c6f205b..302c315 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -64,7 +64,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF @@ -164,7 +164,7 @@ timestamp: 0 string: 123 abbrev: 7 verbose: 2 -quiet: no +quiet: 0 dry run: yes file: prefix/my.file EOF @@ -184,7 +184,7 @@ timestamp: 0 string: 321 abbrev: 10 verbose: 2 -quiet: no +quiet: 0 dry run: no file: prefix/fi.le EOF @@ -212,7 +212,7 @@ timestamp: 0 string: 123 abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) arg 00: a1 @@ -235,7 +235,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF @@ -264,7 +264,7 @@ timestamp: 0 string: 123 abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF @@ -303,7 +303,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) arg 00: --quux @@ -323,7 +323,7 @@ timestamp: 1 string: (not set) abbrev: 7 verbose: 0 -quiet: yes +quiet: 1 dry run: no file: (not set) arg 00: foo @@ -345,7 +345,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF @@ -374,7 +374,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF @@ -399,7 +399,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF @@ -430,7 +430,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF @@ -449,7 +449,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF diff --git a/test-parse-options.c b/test-parse-options.c index 2c8c8f1..86afa98 100644 --- a/test-parse-options.c +++ b/test-parse-options.c @@ -90,7 +90,7 @@ int main(int argc, char **argv) printf("string: %s\n", string ? string : "(not set)"); printf("abbrev: %d\n", abbrev); printf("verbose: %d\n", verbose); - printf("quiet: %s\n", quiet ? "yes" : "no"); + printf("quiet: %d\n", quiet); printf("dry run: %s\n", dry_run ? "yes" : "no"); printf("file: %s\n", file ? file : "(not set)"); -- https://github.com/git/git/pull/218 ``` ## Pranit Bauva, 2016-04-02 23:33 Subject: [PATCH v12 4/5] t7507-commit-verbose: improve test coverage by testing number of diffs Message-ID: <01020153d952be6c-04f90bd2-291e-4eca-99fd-e897d8601132-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020153d952be6c-04f90bd2-291e-4eca-99fd-e897d8601132-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153d952bd99-d3812bd6-d189-4780-ab48-f015696e9cf0-000000@eu-west-1.amazonses.com> ``` Make the fake "editor" store output of grep in a file so that we can see how many diffs were contained in the message and use them in individual tests where ever it is required. Also use write_script() to create the fake "editor". A subsequent commit will introduce scenarios where it is important to be able to exactly determine how many diffs were present. Helped-by: Eric Sunshine Signed-off-by: Pranit Bauva --- Previous version of this patch: - [v11] : $gmane/28820 - [v10]: $gmane/288820 Changes this version wrt previous one: Include the triple dash which I previously forgot, pointed out by Junio. --- t/t7507-commit-verbose.sh | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh index 2ddf28c..0f28a86 100755 --- a/t/t7507-commit-verbose.sh +++ b/t/t7507-commit-verbose.sh @@ -3,11 +3,10 @@ test_description='verbose commit template' . ./test-lib.sh -cat >check-for-diff <out +exit 0 EOF -chmod +x check-for-diff test_set_editor "$PWD/check-for-diff" cat >message <<'EOF' @@ -23,7 +22,8 @@ test_expect_success 'setup' ' ' test_expect_success 'initial commit shows verbose diff' ' - git commit --amend -v + git commit --amend -v && + test_line_count = 1 out ' test_expect_success 'second commit' ' @@ -39,13 +39,15 @@ check_message() { test_expect_success 'verbose diff is stripped out' ' git commit --amend -v && - check_message message + check_message message && + test_line_count = 1 out ' test_expect_success 'verbose diff is stripped out (mnemonicprefix)' ' git config diff.mnemonicprefix true && git commit --amend -v && - check_message message + check_message message && + test_line_count = 1 out ' cat >diff <<'EOF' -- https://github.com/git/git/pull/218 ``` ## Pranit Bauva, 2016-04-02 23:33 Subject: [PATCH v12 1/5] t0040-test-parse-options.sh: fix style issues Message-ID: <01020153d952bd99-d3812bd6-d189-4780-ab48-f015696e9cf0-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020153d952bd99-d3812bd6-d189-4780-ab48-f015696e9cf0-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153cd2340f8-4665cd5f-cd5c-41ab-a162-20acc43ca52e-000000@eu-west-1.amazonses.com> ``` Signed-off-by: Pranit Bauva --- Changes wrt previous version (v11): - This patch is a split up of patch 1/4 v11 as requested by Junio. - This patch uses the backslash with EOF as suggested by Junio for 2 tests namely "detect possible typos" --- t/t0040-parse-options.sh | 72 ++++++++++++++++++++++++------------------------ 1 file changed, 36 insertions(+), 36 deletions(-) diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index 9be6411..c6f205b 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -7,7 +7,7 @@ test_description='our own option parser' . ./test-lib.sh -cat > expect << EOF +cat >expect < --yes get a boolean @@ -49,7 +49,7 @@ Standard options EOF test_expect_success 'test help' ' - test_must_fail test-parse-options -h > output 2> output.err && + test_must_fail test-parse-options -h >output 2>output.err && test_must_be_empty output.err && test_i18ncmp expect output ' @@ -156,7 +156,7 @@ test_expect_success 'OPT_MAGNITUDE() 3giga' ' check magnitude: 3221225472 -m 3g ' -cat > expect << EOF +cat >expect < expect << EOF +cat >expect < expect << EOF +cat >expect < output 2> output.err && + >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > expect << EOF +cat >expect < output 2> output.err && + test-parse-options --int 2 --boolean --no-bo >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' test_expect_success 'unambiguously abbreviated option with "="' ' - test-parse-options --int=2 > output 2> output.err && + test-parse-options --int=2 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' @@ -256,7 +256,7 @@ test_expect_success 'ambiguously abbreviated option' ' test_expect_code 129 test-parse-options --strin 123 ' -cat > expect << EOF +cat >expect < output 2> output.err && + test-parse-options --st 123 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > typo.err << EOF -error: did you mean \`--boolean\` (with two dashes ?) +cat >typo.err <<\EOF +error: did you mean `--boolean` (with two dashes ?) EOF test_expect_success 'detect possible typos' ' - test_must_fail test-parse-options -boolean > output 2> output.err && + test_must_fail test-parse-options -boolean >output 2>output.err && test_must_be_empty output && test_cmp typo.err output.err ' -cat > typo.err << EOF -error: did you mean \`--ambiguous\` (with two dashes ?) +cat >typo.err <<\EOF +error: did you mean `--ambiguous` (with two dashes ?) EOF test_expect_success 'detect possible typos' ' - test_must_fail test-parse-options -ambiguous > output 2> output.err && + test_must_fail test-parse-options -ambiguous >output 2>output.err && test_must_be_empty output && test_cmp typo.err output.err ' -cat > expect <expect < output 2> output.err && + test-parse-options --quux >output 2>output.err && test_must_be_empty output.err && - test_cmp expect output + test_cmp expect output ' -cat > expect <expect < output 2> output.err && + foo -q >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > expect <expect < output 2> output.err && + test-parse-options --length=four -b -4 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > expect <expect < output 2> output.err && + test_must_fail test-parse-options --no-length >output 2>output.err && test_i18ncmp expect output && test_i18ncmp expect.err output.err ' -cat > expect <expect < output 2> output.err && + test-parse-options --set23 -bbbbb --no-or4 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' test_expect_success 'OPT_NEGBIT() and OPT_SET_INT() work' ' - test-parse-options --set23 -bbbbb --neg-or4 > output 2> output.err && + test-parse-options --set23 -bbbbb --neg-or4 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > expect <expect < output 2> output.err && + test-parse-options -bb --or4 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' test_expect_success 'OPT_NEGBIT() works' ' - test-parse-options -bb --no-neg-or4 > output 2> output.err && + test-parse-options -bb --no-neg-or4 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' ' - test-parse-options + + + + + + > output 2> output.err && + test-parse-options + + + + + + >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > expect <expect < output 2> output.err && + test-parse-options -12345 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' @@ -460,7 +460,7 @@ test_expect_success 'negation of OPT_NONEG flags is not ambiguous' ' test_cmp expect output ' -cat >>expect <<'EOF' +cat >>expect < URL: https://gitlist.dev/e/01020153d952be81-1859e6e4-81e4-473f-a2fa-7e27305b4e3e-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153d952bd99-d3812bd6-d189-4780-ab48-f015696e9cf0-000000@eu-west-1.amazonses.com> ``` Add commit.verbose configuration variable as a convenience for those who always prefer --verbose. Helped-by: Junio C Hamano Helped-by: Eric Sunshine Signed-off-by: Pranit Bauva --- The previous version of the patch are: - [v11] $gmane/288820 - [v10] $gmane/288820 - [v9] $gmane/288820 - [v8] $gmane/288820 - [v7] $gmane/288820 - [v6] $gmane/288728 - [v5] $gmane/288728 - [v4] $gmane/288652 - [v3] $gmane/288634 - [v2] $gmane/288569 - [v1] $gmane/287540 Note: One might think some tests are extra but I think that it will be better to include them as they "complete the continuity" thus generalising the series which will make the patch even more clearer. --- Documentation/config.txt | 4 + Documentation/git-commit.txt | 3 +- builtin/commit.c | 14 +++- t/t7507-commit-verbose.sh | 175 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 194 insertions(+), 2 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 2cd6bdd..1d0ec2e 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1110,6 +1110,10 @@ commit.template:: "`~/`" is expanded to the value of `$HOME` and "`~user/`" to the specified user's home directory. +commit.verbose:: + A boolean or int to specify the level of verbose with `git commit`. + See linkgit:git-commit[1]. + credential.helper:: Specify an external helper to be called when a username or password credential is needed; the helper may consult external diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt index 9ec6b3c..d474226 100644 --- a/Documentation/git-commit.txt +++ b/Documentation/git-commit.txt @@ -290,7 +290,8 @@ configuration variable documented in linkgit:git-config[1]. what changes the commit has. Note that this diff output doesn't have its lines prefixed with '#'. This diff will not be a part - of the commit message. + of the commit message. See the `commit.verbose` configuration + variable in linkgit:git-config[1]. + If specified twice, show in addition the unified diff between what would be committed and the worktree files, i.e. the unstaged diff --git a/builtin/commit.c b/builtin/commit.c index b3bd2d4..96e6190 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -113,7 +113,9 @@ static char *edit_message, *use_message; static char *fixup_message, *squash_message; static int all, also, interactive, patch_interactive, only, amend, signoff; static int edit_flag = -1; /* unspecified */ -static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship; +static int config_verbose = -1; /* unspecified */ +static int verbose = -1; /* unspecified */ +static int quiet, no_verify, allow_empty, dry_run, renew_authorship; static int no_post_rewrite, allow_empty_message; static char *untracked_files_arg, *force_date, *ignore_submodule_arg; static char *sign_commit; @@ -1354,6 +1356,8 @@ int cmd_status(int argc, const char **argv, const char *prefix) builtin_status_usage, 0); finalize_colopts(&s.colopts, -1); finalize_deferred_config(&s); + if (verbose == -1) + verbose = 0; handle_untracked_files_arg(&s); if (show_ignored_in_status) @@ -1505,6 +1509,11 @@ static int git_commit_config(const char *k, const char *v, void *cb) sign_commit = git_config_bool(k, v) ? "" : NULL; return 0; } + if (!strcmp(k, "commit.verbose")) { + int is_bool; + config_verbose = git_config_bool_or_int(k, v, &is_bool); + return 0; + } status = git_gpg_config(k, v, NULL); if (status) @@ -1654,6 +1663,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix) argc = parse_and_validate_options(argc, argv, builtin_commit_options, builtin_commit_usage, prefix, current_head, &s); + if (verbose == -1) + verbose = (config_verbose < 0) ? 0 : config_verbose; + if (dry_run) return dry_run_commit(argc, argv, prefix, current_head, &s); index_file = prepare_index(argc, argv, prefix, current_head, 0); diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh index 0f28a86..7c79484 100755 --- a/t/t7507-commit-verbose.sh +++ b/t/t7507-commit-verbose.sh @@ -98,4 +98,179 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' ' test_i18ngrep "Aborting commit due to empty commit message." err ' +test_expect_success 'set up -v -v' ' + echo dirty >file && + echo dirty >file2 && + git add file2 +' +test_expect_success 'commit.verbose true and --verbose omitted' ' + git -c commit.verbose=true commit -F message && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose true and --verbose' ' + git -c commit.verbose=true commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose true and -v -v' ' + git -c commit.verbose=true commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose true and --no-verbose' ' + git -c commit.verbose=true commit --amend --no-verbose && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose false and --verbose' ' + git -c commit.verbose=false commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose false and -v -v' ' + git -c commit.verbose=false commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose false and --verbose omitted' ' + git -c commit.verbose=false commit --amend && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose false and --no-verbose' ' + git -c commit.verbose=false commit --amend --no-verbose && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=-2 and --verbose omitted' ' + git -c commit.verbose=-2 commit --amend && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=-1 and --verbose omitted' ' + git -c commit.verbose=-1 commit --amend && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=0 and --verbose omitted' ' + git -c commit.verbose=0 commit --amend && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=1 and --verbose omitted' ' + git -c commit.verbose=1 commit --amend && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose=2 and --verbose omitted' ' + git -c commit.verbose=2 commit --amend && + test_line_count = 2 out +' + +test_expect_success 'commit-verbose=3 and --verbose omitted' ' + git -c commit.verbose=3 commit --amend && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose=-2 and --verbose' ' + git -c commit.verbose=-2 commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose=-1 and --verbose' ' + git -c commit.verbose=-1 commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose=0 and --verbose' ' + git -c commit.verbose=0 commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose=1 and --verbose' ' + git -c commit.verbose=1 commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose=2 and --verbose' ' + git -c commit.verbose=2 commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose=3 and --verbose' ' + git -c commit.verbose=3 commit --amend --verbose && + test_line_count = 1 out +' + +test_expect_success 'commit.verbose=-2 and -v -v' ' + git -c commit.verbose=-2 commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose=-1 and -v -v' ' + git -c commit.verbose=-1 commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose=0 and -v -v' ' + git -c commit.verbose=0 commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose=1 and -v -v' ' + git -c commit.verbose=1 commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose=2 and -v -v' ' + git -c commit.verbose=2 commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose=3 and -v -v' ' + git -c commit.verbose=3 commit --amend -v -v && + test_line_count = 2 out +' + +test_expect_success 'commit.verbose=-2 and --no-verbose' ' + git -c commit.verbose=-2 commit --amend --no-verbose && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=-1 and --no-verbose' ' + git -c commit.verbose=-1 commit --amend --no-verbose && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=0 and --no-verbose' ' + git -c commit.verbose=0 commit --amend --no-verbose && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=1 and --no-verbose' ' + git -c commit.verbose=1 commit --amend --no-verbose && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=2 and --no-verbose' ' + git -c commit.verbose=2 commit --amend --no-verbose && + test_line_count = 0 out +' + +test_expect_success 'commit.verbose=3 and --no-verbose' ' + git -c commit.verbose=3 commit --amend --no-verbose && + test_line_count = 0 out +' + +test_expect_success 'status ignores commit.verbose=true' ' + git -c commit.verbose=true status >actual && + ! grep "^diff --git" actual +' + +test_expect_success 'status does not verbose without --verbose' ' + git status >actual && + ! grep "^diff --git" actual +' + test_done -- https://github.com/git/git/pull/218 ``` ## Eric Sunshine, 2016-04-03 21:00 Subject: Re: [PATCH v12 1/5] t0040-test-parse-options.sh: fix style issues Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQ%2B89MZSyHJhCrZsGMc7mM_uorE5j9GXk7kCTT1OTXV6Q%40mail.gmail.com In-Reply-To: <01020153d952bd99-d3812bd6-d189-4780-ab48-f015696e9cf0-000000@eu-west-1.amazonses.com> ``` On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote: > Signed-off-by: Pranit Bauva > --- > diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh > @@ -7,7 +7,7 @@ test_description='our own option parser' > > . ./test-lib.sh > > -cat > expect << EOF > +cat >expect < usage: test-parse-options > --yes get a boolean It would be better to use <<\EOF for this one to make it clear that no interpolation is desired in the heredoc. > @@ -156,7 +156,7 @@ test_expect_success 'OPT_MAGNITUDE() 3giga' ' > check magnitude: 3221225472 -m 3g > ' > > -cat > expect << EOF > +cat >expect < boolean: 2 > integer: 1729 > magnitude: 16384 > @@ -310,12 +310,12 @@ arg 00: --quux > EOF > > test_expect_success 'keep some options as arguments' ' > - test-parse-options --quux > output 2> output.err && > + test-parse-options --quux >output 2>output.err && > test_must_be_empty output.err && > - test_cmp expect output > + test_cmp expect output Okay, this is a whitespace change (spaces -> tab). > ' > @@ -460,7 +460,7 @@ test_expect_success 'negation of OPT_NONEG flags is not ambiguous' ' > -cat >>expect <<'EOF' > +cat >>expect < list: foo > list: bar > list: baz ``` ## Eric Sunshine, 2016-04-03 21:30 Subject: Re: [PATCH v12 2/5] test-parse-options: print quiet as integer Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQpdr7A7GxKUN3tDs2rQ7z_S5b1WVsk01%3DBwkCwB2mp0A%40mail.gmail.com In-Reply-To: <01020153d952be64-9ca893f1-5a7b-4a2c-a0bd-98abf93a17ac-000000@eu-west-1.amazonses.com> ``` On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote: > Current implementation of parse-options.c treats OPT__QUIET() as integer > and not boolean and thus it is more appropriate to print it as integer > to avoid confusion. I can buy this line of reasoning, however, it would be even easier to sell the change if you cited an existing client (a git command) which actually respects multiple quiet levels. Are there any? More importantly, though, this change implies that you should also add tests to ensure that the quiet level is indeed incremented with each --quiet, just as "-vv" and "--verbose --verbose" are already tested. You might be able to include such new tests directly in this patch as long as the commit message is clear about it, or add them in a separate patch. By the way, I don't see any tests to ensure that --no-verbose and --no-quiet reset those respective values to 0. A separate patch which adds such tests would be nice (unless such tests already exist and I merely missed them). > Signed-off-by: Pranit Bauva > --- > t/t0040-parse-options.sh | 26 +++++++++++++------------- > test-parse-options.c | 2 +- > 2 files changed, 14 insertions(+), 14 deletions(-) > > diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh > index c6f205b..302c315 100755 > --- a/t/t0040-parse-options.sh > +++ b/t/t0040-parse-options.sh > @@ -64,7 +64,7 @@ timestamp: 0 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > EOF > @@ -164,7 +164,7 @@ timestamp: 0 > string: 123 > abbrev: 7 > verbose: 2 > -quiet: no > +quiet: 0 > dry run: yes > file: prefix/my.file > EOF > @@ -184,7 +184,7 @@ timestamp: 0 > string: 321 > abbrev: 10 > verbose: 2 > -quiet: no > +quiet: 0 > dry run: no > file: prefix/fi.le > EOF > @@ -212,7 +212,7 @@ timestamp: 0 > string: 123 > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > arg 00: a1 > @@ -235,7 +235,7 @@ timestamp: 0 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > EOF > @@ -264,7 +264,7 @@ timestamp: 0 > string: 123 > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > EOF > @@ -303,7 +303,7 @@ timestamp: 0 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > arg 00: --quux > @@ -323,7 +323,7 @@ timestamp: 1 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: yes > +quiet: 1 > dry run: no > file: (not set) > arg 00: foo > @@ -345,7 +345,7 @@ timestamp: 0 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > EOF > @@ -374,7 +374,7 @@ timestamp: 0 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > EOF > @@ -399,7 +399,7 @@ timestamp: 0 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > EOF > @@ -430,7 +430,7 @@ timestamp: 0 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > EOF > @@ -449,7 +449,7 @@ timestamp: 0 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > EOF > diff --git a/test-parse-options.c b/test-parse-options.c > index 2c8c8f1..86afa98 100644 > --- a/test-parse-options.c > +++ b/test-parse-options.c > @@ -90,7 +90,7 @@ int main(int argc, char **argv) > printf("string: %s\n", string ? string : "(not set)"); > printf("abbrev: %d\n", abbrev); > printf("verbose: %d\n", verbose); > - printf("quiet: %s\n", quiet ? "yes" : "no"); > + printf("quiet: %d\n", quiet); > printf("dry run: %s\n", dry_run ? "yes" : "no"); > printf("file: %s\n", file ? file : "(not set)"); ``` ## Eric Sunshine, 2016-04-03 23:10 Subject: Re: [PATCH v12 3/5] parse-options.c: make OPTION_COUNTUP respect "unspecified" values Message-ID: URL: https://gitlist.dev/e/CAPig%2BcS23i9XFTntLTHRbNf21DxhLAcw%2B9ent_ZtWj%3D1aO7JwQ%40mail.gmail.com In-Reply-To: <01020153d952be68-ee055619-664d-4502-92e9-6aa524e2354b-000000@eu-west-1.amazonses.com> ``` On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote: > The reason to make it respect "unspecified" values is to give the > ability to differentiate whether `--option` or `--no-option` was > specified at all. "unspecified" values should be in the form of negative > values. If initial value is set to negative and `--option` specified > then it will reflect the number of occurrences (counting done from 0), > if `--no-option` is specified then it will reflect 0 and if nothing at > all is given then it will retain its negative value. Thanks, this rewrite does a better job of explaining the aim of the change and how a client can take advantage of it. However, with my "first-time reader" hat on, I still have some trouble digesting it as a coherent whole. I wonder if a rewrite like this would help? OPT_COUNTUP() merely increments the counter upon --option, and resets it to 0 upon --no-option, which means that there is no "unspecified" value with which a client can initialize the counter to determine whether or not --[no-]option was seen at all. Make OPT_COUNTUP() treat any negative number as an "unspecified" value to address this shortcoming. In particular, if a client initializes the counter to -1, then if it is still -1 after parse_options(), then neither --option nor --no-option was seen; if it is 0, then --no-option was seen last, and if it is 1 or greater, than --option was seen last. > This change will not affect existing users of COUNTUP, because they all > use the initial value of 0 (or more). "This change does not affect behavior of existing clients of..." > Note that builtin/clean.c initializes the variable used with > OPT__FORCE (which uses OPT_COUNTUP()) to a negative value, but it is set > to either 0 or 1 by reading the configuration before the code calls > parse_options(), i.e. as far as parse_options() is concerned, the > initial value of the variable is not negative. > > To test this behavior "verbose" is set to "unspecified" while quiet is > set to 0 which will test the new behavior with all sets of values. I think you need to mention here that you're talking about test-parse-options.c (and indirectly t0040) since it's otherwise too easy for the reader to think that this paragraph is a continuation of the discussion about OPT_COUNTUP()'s new behavior and how it won't impact existing tests, rather than a new topic of its own (testing the behavior). Maybe say something like this: To test the new behavior, augment the initial "verbose" setting of test-parse-options.c to be unspecified... and then go on to say that, because "quiet" is still initialized to 0, you have complete coverage. An alternative would be to split off the new testing into its own patch, which would make this patch (which is the real meat of the change) less noisy. I actually expected you to add new functionality to test-parse-options.c specifically to test OPT_COUNTUP() directly rather than indirectly through --verbose and --quiet, however, I think I can be sold on the approach you took for a couple reasons. First, you have a genuine use-case for an "unspecified" --verbose value, so changing test-parse-options.c's --verbose to start at -1 tests what you ultimately care about. Second, since you retained 0-initialization of --quiet, that case of OPT_COUNTUP() is still being tested. What I find a bit disturbing about this approach is that you are getting "full coverage" only because of current *implementation*, not because you are explicitly testing for *expected* behavior. That is, you get that coverage only while both OPT__VERBOSE() and OPT__QUIET() are built atop OPT_COUNTUP(); if OPT__QUIET() is ever rewritten so it no longer uses OPT_COUNTUP(), then the tests silently lose full coverage. However, I may be over-worrying about the situation... > Signed-off-by: Pranit Bauva > --- > diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt > @@ -144,8 +144,12 @@ There are some macros to easily define options: > `OPT_COUNTUP(short, long, &int_var, description)`:: > Introduce a count-up option. > - `int_var` is incremented on each use of `--option`, and > - reset to zero with `--no-option`. > + Each use of `--option` increments `int_var`, starting from zero > + (even if initially negative), and `--no-option` resets it to > + zero. To determine if `--option` or `--no-option` was set at s/was set/was encountered/ > + all, set `int_var` to a negative value, and if it is still s/set `int_var`/initialize `int_var`/ > + negative after parse_options(), then neither `--option` nor > + `--no-option` was seen. > diff --git a/parse-options.c b/parse-options.c > @@ -110,6 +110,8 @@ static int get_value(struct parse_opt_ctx_t *p, > case OPTION_COUNTUP: > + if (*(int *)opt->value < 0) > + *(int *)opt->value = 0; > *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; This is nicer. > return 0; > diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh > @@ -454,6 +454,25 @@ dry run: no > +test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' ' > + test-parse-options --no-verbose >output 2>output.err && > + test_must_be_empty output.err && > + test_cmp expect output > +' If you take the advice of my 2/5 review and add new tests (in a new patch) which test --no-verbose and --no-quiet, then I think this new test here can just go away since the case it cares about will already be covered. ``` ## Eric Sunshine, 2016-04-04 00:02 Subject: Re: [PATCH v12 4/5] t7507-commit-verbose: improve test coverage by testing number of diffs Message-ID: URL: https://gitlist.dev/e/CAPig%2BcRBwOjxA2cB%3DZR__o7LsHxk9Rbzfv7ngRxv8h6%3DkoQSRw%40mail.gmail.com In-Reply-To: <01020153d952be6c-04f90bd2-291e-4eca-99fd-e897d8601132-000000@eu-west-1.amazonses.com> ``` On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote: > Make the fake "editor" store output of grep in a file so that we can > see how many diffs were contained in the message and use them in > individual tests where ever it is required. Also use write_script() > to create the fake "editor". > > A subsequent commit will introduce scenarios where it is important to be > able to exactly determine how many diffs were present. These two sentences are backward. The "subsequent commit" bit is justification for why you are making the "editor" store the output, thus it belongs with the first paragraph. The bit about write_script() is just a minor aside which can go in its own paragraph. I think it's also important to explain that you're changing the behavior of write_script() so that it always succeeds, regardless of whether grep found diff headers or not, and to give the reason for making this change ("so that you don't have to use 'test_must_fail' for cases when no diff headers are expected and can instead easily use 'test_line_count = 0'"). The patch itself looks fine and is easy enough to read. > Helped-by: Eric Sunshine > Signed-off-by: Pranit Bauva > --- > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh > index 2ddf28c..0f28a86 100755 > --- a/t/t7507-commit-verbose.sh > +++ b/t/t7507-commit-verbose.sh > @@ -3,11 +3,10 @@ > test_description='verbose commit template' > . ./test-lib.sh > > -cat >check-for-diff < -#!$SHELL_PATH > -exec grep '^diff --git' "\$1" > +write_script "check-for-diff" <<\EOF && > +grep '^diff --git' "$1" >out > +exit 0 > EOF > -chmod +x check-for-diff > test_set_editor "$PWD/check-for-diff" > > cat >message <<'EOF' > @@ -23,7 +22,8 @@ test_expect_success 'setup' ' > ' > > test_expect_success 'initial commit shows verbose diff' ' > - git commit --amend -v > + git commit --amend -v && > + test_line_count = 1 out > ' > > test_expect_success 'second commit' ' > @@ -39,13 +39,15 @@ check_message() { > > test_expect_success 'verbose diff is stripped out' ' > git commit --amend -v && > - check_message message > + check_message message && > + test_line_count = 1 out > ' > > test_expect_success 'verbose diff is stripped out (mnemonicprefix)' ' > git config diff.mnemonicprefix true && > git commit --amend -v && > - check_message message > + check_message message && > + test_line_count = 1 out > ' ``` ## Eric Sunshine, 2016-04-04 00:58 Subject: Re: [PATCH v12 5/5] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTYHFyDXJ2ZECtje2OG-ObT04gnQw%2B0%2BLbMMVO51bx2fg%40mail.gmail.com In-Reply-To: <01020153d952be81-1859e6e4-81e4-473f-a2fa-7e27305b4e3e-000000@eu-west-1.amazonses.com> ``` On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote: > Add commit.verbose configuration variable as a convenience for those > who always prefer --verbose. > > Signed-off-by: Pranit Bauva > --- > diff --git a/Documentation/config.txt b/Documentation/config.txt > @@ -1110,6 +1110,10 @@ commit.template:: > +commit.verbose:: > + A boolean or int to specify the level of verbose with `git commit`. > + See linkgit:git-commit[1]. s/level of verbose with/verbosity level of/ > diff --git a/builtin/commit.c b/builtin/commit.c > @@ -1505,6 +1509,11 @@ static int git_commit_config(const char *k, const char *v, void *cb) > + if (!strcmp(k, "commit.verbose")) { > + int is_bool; > + config_verbose = git_config_bool_or_int(k, v, &is_bool); > + return 0; > + } > @@ -1654,6 +1663,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > argc = parse_and_validate_options(argc, argv, builtin_commit_options, > builtin_commit_usage, > prefix, current_head, &s); > + if (verbose == -1) > + verbose = (config_verbose < 0) ? 0 : config_verbose; Okay, so this version compares 'config_verbose' with "< 0" rather than "== -1" so it won't misbehave when someone sets config.verbose=-2 as pointed out in [1]. Good. > diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh > @@ -98,4 +98,179 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' ' > +test_expect_success 'set up -v -v' ' Style: "setup -v -v", not "set up -v -v" > + echo dirty >file && > + echo dirty >file2 && > + git add file2 > +' Why this extra complexity when all you need for the "-v -v" case is: test_expect_success 'setup -v -v' ' echo dirty >file ' as suggested by [1]? Try to keep unnecessary gunk out of the tests (and code, in general) to avoid misleading readers into wondering if something unusual is going on. Which leads to... > +test_expect_success 'commit.verbose true and --verbose omitted' ' > + git -c commit.verbose=true commit -F message && > + test_line_count = 1 out > +' Why is this new test different (by using "-F message") from all other new tests which simply use "--amend"? The answer is that it is performing a "setup" action which should have been done in the "setup" test just above. But, as noted, this extra setup is unnecessary, thus also misleading and confusing for readers. Better would be to use the minimal "setup" shown above (from [1]), and restore this test to use "--amend" like all other tests. More below... > +test_expect_success 'commit.verbose true and --verbose' ' > + git -c commit.verbose=true commit --amend --verbose && > + test_line_count = 1 out > +' > + > +test_expect_success 'commit.verbose true and -v -v' ' > + git -c commit.verbose=true commit --amend -v -v && > + test_line_count = 2 out > +' > [...large number of almost identical tests omitted...] > + > +test_expect_success 'commit.verbose=-2 and --no-verbose' ' > + git -c commit.verbose=-2 commit --amend --no-verbose && > + test_line_count = 0 out > +' > + > +test_expect_success 'commit.verbose=-1 and --no-verbose' ' > + git -c commit.verbose=-1 commit --amend --no-verbose && > + test_line_count = 0 out > +' > + > +test_expect_success 'commit.verbose=0 and --no-verbose' ' > + git -c commit.verbose=0 commit --amend --no-verbose && > + test_line_count = 0 out > +' > + > +test_expect_success 'commit.verbose=1 and --no-verbose' ' > + git -c commit.verbose=1 commit --amend --no-verbose && > + test_line_count = 0 out > +' The fact that the 32 new tests are nearly identical suggests strongly that the testing should instead either be table-driven or be done via for-loops to systematically cover all cases. Not only would either of these approaches be easier to digest, but they would make it easy to tell at a glance if any cases were missing. See [2] for an example of how the tests can be table-driven, and see the bottom of [3] for an example of using for-loops to test systematically (though you'd need to use nested for-loops rather than just the one in the example). I'm leaning toward systematic testing via nested for-loops as the more suitable of the two approach for this particular application. [1]: http://article.gmane.org/gmane.comp.version-control.git/290000 [2]: http://article.gmane.org/gmane.comp.version-control.git/290344 [3]: http://article.gmane.org/gmane.comp.version-control.git/289860 ``` ## Eric Sunshine, 2016-04-04 01:05 Subject: Re: [PATCH v12 4/5] t7507-commit-verbose: improve test coverage by testing number of diffs Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQGeqvmTz1SdpJG137PgR%3DbC0d_Nu_q_ofhZM1BCYoxjQ%40mail.gmail.com In-Reply-To: ``` On Sun, Apr 3, 2016 at 8:02 PM, Eric Sunshine wrote: > On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote: >> Make the fake "editor" store output of grep in a file so that we can >> see how many diffs were contained in the message and use them in >> individual tests where ever it is required. Also use write_script() >> to create the fake "editor". >> >> A subsequent commit will introduce scenarios where it is important to be >> able to exactly determine how many diffs were present. > > These two sentences are backward. The "subsequent commit" bit is > justification for why you are making the "editor" store the output, > thus it belongs with the first paragraph. The bit about write_script() > is just a minor aside which can go in its own paragraph. > > I think it's also important to explain that you're changing the > behavior of write_script() so that it always succeeds, regardless of s/write_script()/the fake "editor"/ > whether grep found diff headers or not, and to give the reason for > making this change ("so that you don't have to use 'test_must_fail' > for cases when no diff headers are expected and can instead easily use > 'test_line_count = 0'"). ``` ## Pranit Bauva, 2016-04-04 12:45 Subject: Re: [PATCH v12 1/5] t0040-test-parse-options.sh: fix style issues Message-ID: URL: https://gitlist.dev/e/CAFZEwPNaxFyFmCkvPjRZQ0grOm1oWB1%2B5QhkpOMyEKW7j_99qQ%40mail.gmail.com In-Reply-To: ``` On Mon, Apr 4, 2016 at 2:30 AM, Eric Sunshine wrote: > On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote: >> Signed-off-by: Pranit Bauva >> --- >> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh >> @@ -7,7 +7,7 @@ test_description='our own option parser' >> >> . ./test-lib.sh >> >> -cat > expect << EOF >> +cat >expect <> usage: test-parse-options >> --yes get a boolean > > It would be better to use <<\EOF for this one to make it clear that no > interpolation is desired in the heredoc. > >> @@ -156,7 +156,7 @@ test_expect_success 'OPT_MAGNITUDE() 3giga' ' >> check magnitude: 3221225472 -m 3g >> ' >> >> -cat > expect << EOF >> +cat >expect < > Ditto: <<\EOF > > Same applies to all similar heredocs in subsequent tests where > interpolation is not desired. > >> boolean: 2 >> integer: 1729 >> magnitude: 16384 >> @@ -310,12 +310,12 @@ arg 00: --quux >> EOF >> >> test_expect_success 'keep some options as arguments' ' >> - test-parse-options --quux > output 2> output.err && >> + test-parse-options --quux >output 2>output.err && >> test_must_be_empty output.err && >> - test_cmp expect output >> + test_cmp expect output > > Okay, this is a whitespace change (spaces -> tab). > >> ' >> @@ -460,7 +460,7 @@ test_expect_success 'negation of OPT_NONEG flags is not ambiguous' ' >> -cat >>expect <<'EOF' >> +cat >>expect < > This is not a desirable change. This heredoc does not require > interpolation, so you don't want to turn it into a form which does > interpolate. For style consistency, therefore, you should change 'EOF' > to \EOF. > >> list: foo >> list: bar >> list: baz Okay I will do the change. I was previously unaware about the use of '\' before EOF. I googled it now. But I am still confused about its use in this scenario. Upto what I understood, it is used where you want to expand a variable, substitute a command, arithmethic expansion. The use of '\' in the tests I have changed in v12 wrt 11 is understood by me as you want to remove the use of escape sequences which is justified. But this seems a bit vague. Is it some convention in git? ``` ## Eric Sunshine, 2016-04-04 17:30 Subject: Re: [PATCH v12 1/5] t0040-test-parse-options.sh: fix style issues Message-ID: URL: https://gitlist.dev/e/CAPig%2BcQw%3DaG0NOgEsmdTGc7v3iBA-qG9_C4EdSm%3D6PcE6Qn-uQ%40mail.gmail.com In-Reply-To: ``` On Mon, Apr 4, 2016 at 8:45 AM, Pranit Bauva wrote: > Okay I will do the change. I was previously unaware about the use of > '\' before EOF. I googled it now. But I am still confused about its > use in this scenario. Upto what I understood, it is used where you > want to expand a variable, substitute a command, arithmethic > expansion. The use of '\' in the tests I have changed in v12 wrt 11 is > understood by me as you want to remove the use of escape sequences > which is justified. But this seems a bit vague. Is it some convention > in git? Both 'EOF' and \EOF suppress interpolation and other transformations in the heredoc content which would otherwise occur with plain EOF. The 'EOF' form is well documented; \EOF not so much, but is used heavily in git test scripts. So: x=flormp echo < URL: https://gitlist.dev/e/CAPig%2BcQdAbweFF7NWJhNjALXN5cdpE-gtvZ_LB0tVmLJ2XGLiA%40mail.gmail.com In-Reply-To: ``` On Sun, Apr 3, 2016 at 8:58 PM, Eric Sunshine wrote: > The fact that the 32 new tests are nearly identical suggests strongly > that the testing should instead either be table-driven or be done via > for-loops to systematically cover all cases. Not only would either of > these approaches be easier to digest, but they would make it easy to > tell at a glance if any cases were missing. See [2] for an example of > how the tests can be table-driven, and see the bottom of [3] for an > example of using for-loops to test systematically (though you'd need > to use nested for-loops rather than just the one in the example). > > I'm leaning toward systematic testing via nested for-loops as the more > suitable of the two approach for this particular application. By the way, while this would be a nice change, it doesn't necessarily have to be part of this series, and could be done as a follow-up by you or someone else. (The other changes suggested in the same review, however, ought to be fixed in this series; in particular, simplifying the "setup" test and making the first test after "setup" consistent with the remaining tests.) ``` ## Pranit Bauva, 2016-04-05 05:08 Subject: Re: [PATCH v12 1/5] t0040-test-parse-options.sh: fix style issues Message-ID: URL: https://gitlist.dev/e/CAFZEwPPPf3OUb8rL-juL3dMXZamwsTO2e%2BpLg3y8grdJf6W6YA%40mail.gmail.com In-Reply-To: ``` On Mon, Apr 4, 2016 at 11:00 PM, Eric Sunshine wrote: > On Mon, Apr 4, 2016 at 8:45 AM, Pranit Bauva wrote: >> Okay I will do the change. I was previously unaware about the use of >> '\' before EOF. I googled it now. But I am still confused about its >> use in this scenario. Upto what I understood, it is used where you >> want to expand a variable, substitute a command, arithmethic >> expansion. The use of '\' in the tests I have changed in v12 wrt 11 is >> understood by me as you want to remove the use of escape sequences >> which is justified. But this seems a bit vague. Is it some convention >> in git? > > Both 'EOF' and \EOF suppress interpolation and other transformations > in the heredoc content which would otherwise occur with plain EOF. The > 'EOF' form is well documented; \EOF not so much, but is used heavily > in git test scripts. So: > > x=flormp > echo < Hello, $x > EOF > > prints "Hello, flormp", whereas: > > echo <<\EOF > Hello, $x > EOF > > prints "Hello, $x". > > While test scripts sometimes use \EOF to explicitly suppress variable > expansion, it's also quite common to use it even when there is nothing > which could be expanded in the heredoc content, in which case it > signals to the reader that the author doesn't expect the content to > undergo expansion or interpolation. It's also a bit of future-proofing > in case some later change to the heredoc content inserts something > which might otherwise be expanded. Thanks for taking out your time to explain this clearly. I will do the changes in the tests as suggested by your review. :) ``` ## Pranit Bauva, 2016-04-05 15:39 Subject: Re: [PATCH v12 2/5] test-parse-options: print quiet as integer Message-ID: URL: https://gitlist.dev/e/CAFZEwPORAer3%3DWmUqqhztv4kAOHdv%2ByB12tRyUYh_YTSmtcQsQ%40mail.gmail.com In-Reply-To: ``` [+cc:Duy Nguyen, Jonathan Nieder] On Mon, Apr 4, 2016 at 3:00 AM, Eric Sunshine wrote: > On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote: >> Current implementation of parse-options.c treats OPT__QUIET() as integer >> and not boolean and thus it is more appropriate to print it as integer >> to avoid confusion. > > I can buy this line of reasoning, however, it would be even easier to > sell the change if you cited an existing client (a git command) which > actually respects multiple quiet levels. Are there any? I investigated into this. But I was unable to find any git commit which actually respects mulitple quiet levels. I first did a 'git grep OPT__QUIET' to find the commands which use this. Then I went through the documentation which covers it. None of them have any such mention of multiple quiet levels. But still I dug further and and went through all the individual source files. I followed the corresponding C source code for the header file included and also searched there for any trace of quiet. But I still didn't find any such use of multiple quiet levels. I have found that the commit id 212c0a6f (Duy Ngyuyen; 7 Dec, 2013; parse-options: remove OPT__BOOLEAN). Maybe he has something to say as to why this was introduced and OPT__QUIET which previously used OPT__BOOLEAN, now uses OPT_COUNTUP rather than OPT_BOOL. This commit In fact git-repack command has quiet but this is not mentioned in the documentation. If someone could include this it in the documentation. I would do it but I am not quite familiar with git-repack and haven't really used it anytime. > More importantly, though, this change implies that you should also add > tests to ensure that the quiet level is indeed incremented with each > --quiet, just as "-vv" and "--verbose --verbose" are already tested. > You might be able to include such new tests directly in this patch as > long as the commit message is clear about it, or add them in a > separate patch. I will include the tests for multiple level of quiet in the patch. > By the way, I don't see any tests to ensure that --no-verbose and > --no-quiet reset those respective values to 0. A separate patch which > adds such tests would be nice (unless such tests already exist and I > merely missed them). There are no tests to ensure that --no-verbose and --no-quiet reset those respective values to 0 just before this patch. But I have covered the --no-verbose tests in the upcoming patch 3/5. I will include the tests for --no-quiet in this patch. ``` ## Pranit Bauva, 2016-04-05 15:51 Subject: Re: [PATCH v12 3/5] parse-options.c: make OPTION_COUNTUP respect "unspecified" values Message-ID: URL: https://gitlist.dev/e/CAFZEwPPvwiw4E78vYA_jpLAfASS-M0fxPopnbFSLd4yomzVciA%40mail.gmail.com In-Reply-To: ``` On Mon, Apr 4, 2016 at 4:40 AM, Eric Sunshine wrote: > On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote: >> The reason to make it respect "unspecified" values is to give the >> ability to differentiate whether `--option` or `--no-option` was >> specified at all. "unspecified" values should be in the form of negative >> values. If initial value is set to negative and `--option` specified >> then it will reflect the number of occurrences (counting done from 0), >> if `--no-option` is specified then it will reflect 0 and if nothing at >> all is given then it will retain its negative value. > > Thanks, this rewrite does a better job of explaining the aim of the > change and how a client can take advantage of it. However, with my > "first-time reader" hat on, I still have some trouble digesting it as > a coherent whole. I wonder if a rewrite like this would help? > > OPT_COUNTUP() merely increments the counter upon --option, and > resets it to 0 upon --no-option, which means that there is no > "unspecified" value with which a client can initialize the > counter to determine whether or not --[no-]option was seen at > all. > > Make OPT_COUNTUP() treat any negative number as an "unspecified" > value to address this shortcoming. In particular, if a client > initializes the counter to -1, then if it is still -1 after > parse_options(), then neither --option nor --no-option was seen; > if it is 0, then --no-option was seen last, and if it is 1 or > greater, than --option was seen last. > >> This change will not affect existing users of COUNTUP, because they all >> use the initial value of 0 (or more). > > "This change does not affect behavior of existing clients of..." > Sure I could do the change. >> Note that builtin/clean.c initializes the variable used with >> OPT__FORCE (which uses OPT_COUNTUP()) to a negative value, but it is set >> to either 0 or 1 by reading the configuration before the code calls >> parse_options(), i.e. as far as parse_options() is concerned, the >> initial value of the variable is not negative. >> >> To test this behavior "verbose" is set to "unspecified" while quiet is >> set to 0 which will test the new behavior with all sets of values. > > I think you need to mention here that you're talking about test-parse-options.c > (and indirectly t0040) since it's otherwise too easy for the reader to > think that this paragraph is a continuation of the discussion about > OPT_COUNTUP()'s new behavior and how it won't impact existing tests, > rather than a new topic of its own (testing the behavior). Maybe say > something like this: > > To test the new behavior, augment the initial "verbose" setting > of test-parse-options.c to be unspecified... > > and then go on to say that, because "quiet" is still initialized to 0, > you have complete coverage. An alternative would be to split off the > new testing into its own patch, which would make this patch (which is > the real meat of the change) less noisy. I do like including test-parse-options.c in commit message. My main motive behind including the test with this patch was to show the "first-time" reader how to use the changes introduced in this patch. This would also set a complete picture of this commit. And I kind of believe it is much effort for the reader to find the commit whose parent will be this (if it exists at all, as the reader won't know about it) which will give a kind of demonstration to utilizing this change. > > I actually expected you to add new functionality to > test-parse-options.c specifically to test OPT_COUNTUP() directly > rather than indirectly through --verbose and --quiet, however, I think > I can be sold on the approach you took for a couple reasons. First, > you have a genuine use-case for an "unspecified" --verbose value, so > changing test-parse-options.c's --verbose to start at -1 tests what > you ultimately care about. Second, since you retained 0-initialization > of --quiet, that case of OPT_COUNTUP() is still being tested. > > What I find a bit disturbing about this approach is that you are > getting "full coverage" only because of current *implementation*, not > because you are explicitly testing for *expected* behavior. That is, > you get that coverage only while both OPT__VERBOSE() and OPT__QUIET() > are built atop OPT_COUNTUP(); if OPT__QUIET() is ever rewritten so it > no longer uses OPT_COUNTUP(), then the tests silently lose full > coverage. However, I may be over-worrying about the situation... The main reason as I mentioned above was to give a demonstration of how to utilize the change introduced. > >> Signed-off-by: Pranit Bauva >> --- >> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt >> @@ -144,8 +144,12 @@ There are some macros to easily define options: >> `OPT_COUNTUP(short, long, &int_var, description)`:: >> Introduce a count-up option. >> - `int_var` is incremented on each use of `--option`, and >> - reset to zero with `--no-option`. >> + Each use of `--option` increments `int_var`, starting from zero >> + (even if initially negative), and `--no-option` resets it to >> + zero. To determine if `--option` or `--no-option` was set at > > s/was set/was encountered/ > >> + all, set `int_var` to a negative value, and if it is still > > s/set `int_var`/initialize `int_var`/ > >> + negative after parse_options(), then neither `--option` nor >> + `--no-option` was seen. >> diff --git a/parse-options.c b/parse-options.c >> @@ -110,6 +110,8 @@ static int get_value(struct parse_opt_ctx_t *p, >> case OPTION_COUNTUP: >> + if (*(int *)opt->value < 0) >> + *(int *)opt->value = 0; >> *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; > > This is nicer. > >> return 0; >> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh >> @@ -454,6 +454,25 @@ dry run: no >> +test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' ' >> + test-parse-options --no-verbose >output 2>output.err && >> + test_must_be_empty output.err && >> + test_cmp expect output >> +' > > If you take the advice of my 2/5 review and add new tests (in a new > patch) which test --no-verbose and --no-quiet, then I think this new > test here can just go away since the case it cares about will already > be covered. Adding a new patch between 2/5 and 3/5 would be a better choice. ``` ## Pranit Bauva, 2016-04-05 15:54 Subject: Re: [PATCH v12 4/5] t7507-commit-verbose: improve test coverage by testing number of diffs Message-ID: URL: https://gitlist.dev/e/CAFZEwPO_Y6-jEGXDspExhmaWO36QWhaK813TcEUWSqB_byZngQ%40mail.gmail.com In-Reply-To: ``` On Mon, Apr 4, 2016 at 5:32 AM, Eric Sunshine wrote: > On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote: >> Make the fake "editor" store output of grep in a file so that we can >> see how many diffs were contained in the message and use them in >> individual tests where ever it is required. Also use write_script() >> to create the fake "editor". >> >> A subsequent commit will introduce scenarios where it is important to be >> able to exactly determine how many diffs were present. > > These two sentences are backward. The "subsequent commit" bit is > justification for why you are making the "editor" store the output, > thus it belongs with the first paragraph. The bit about write_script() > is just a minor aside which can go in its own paragraph. > > I think it's also important to explain that you're changing the > behavior of write_script() so that it always succeeds, regardless of > whether grep found diff headers or not, and to give the reason for > making this change ("so that you don't have to use 'test_must_fail' > for cases when no diff headers are expected and can instead easily use > 'test_line_count = 0'"). > > The patch itself looks fine and is easy enough to read. I will include some more explanation as you suggest. > >> Helped-by: Eric Sunshine >> Signed-off-by: Pranit Bauva >> --- >> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh >> index 2ddf28c..0f28a86 100755 >> --- a/t/t7507-commit-verbose.sh >> +++ b/t/t7507-commit-verbose.sh >> @@ -3,11 +3,10 @@ >> test_description='verbose commit template' >> . ./test-lib.sh >> >> -cat >check-for-diff <> -#!$SHELL_PATH >> -exec grep '^diff --git' "\$1" >> +write_script "check-for-diff" <<\EOF && >> +grep '^diff --git' "$1" >out >> +exit 0 >> EOF >> -chmod +x check-for-diff >> test_set_editor "$PWD/check-for-diff" >> >> cat >message <<'EOF' >> @@ -23,7 +22,8 @@ test_expect_success 'setup' ' >> ' >> >> test_expect_success 'initial commit shows verbose diff' ' >> - git commit --amend -v >> + git commit --amend -v && >> + test_line_count = 1 out >> ' >> >> test_expect_success 'second commit' ' >> @@ -39,13 +39,15 @@ check_message() { >> >> test_expect_success 'verbose diff is stripped out' ' >> git commit --amend -v && >> - check_message message >> + check_message message && >> + test_line_count = 1 out >> ' >> >> test_expect_success 'verbose diff is stripped out (mnemonicprefix)' ' >> git config diff.mnemonicprefix true && >> git commit --amend -v && >> - check_message message >> + check_message message && >> + test_line_count = 1 out >> ' ``` ## Pranit Bauva, 2016-04-05 15:58 Subject: Re: [PATCH v12 5/5] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAFZEwPNQE82_A39sO7z--3sR74GwF-HEgHTePmN_DbFgFmgKgQ%40mail.gmail.com In-Reply-To: ``` On Tue, Apr 5, 2016 at 4:59 AM, Eric Sunshine wrote: > On Sun, Apr 3, 2016 at 8:58 PM, Eric Sunshine wrote: >> The fact that the 32 new tests are nearly identical suggests strongly >> that the testing should instead either be table-driven or be done via >> for-loops to systematically cover all cases. Not only would either of >> these approaches be easier to digest, but they would make it easy to >> tell at a glance if any cases were missing. See [2] for an example of >> how the tests can be table-driven, and see the bottom of [3] for an >> example of using for-loops to test systematically (though you'd need >> to use nested for-loops rather than just the one in the example). >> >> I'm leaning toward systematic testing via nested for-loops as the more >> suitable of the two approach for this particular application. > I hadn't thought of this before. I also believe using for-loops will make it more clear, crisp and will avoid the effort of going through the whole patch to find out if some test is missing. > By the way, while this would be a nice change, it doesn't necessarily > have to be part of this series, and could be done as a follow-up by > you or someone else. > > (The other changes suggested in the same review, however, ought to be > fixed in this series; in particular, simplifying the "setup" test and > making the first test after "setup" consistent with the remaining > tests.) I will include it this series only as it will be a bit easier for me to keep a track of the previous reviews. ``` ## Eric Sunshine, 2016-04-06 05:56 Subject: Re: [PATCH v12 2/5] test-parse-options: print quiet as integer Message-ID: URL: https://gitlist.dev/e/CAPig%2BcTynD42tDe9bQDvKEb7ACFpWwwq85AT8byb8u79G0iVHA%40mail.gmail.com In-Reply-To: ``` On Tue, Apr 5, 2016 at 11:39 AM, Pranit Bauva wrote: > On Mon, Apr 4, 2016 at 3:00 AM, Eric Sunshine wrote: >> On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote: >>> Current implementation of parse-options.c treats OPT__QUIET() as integer >>> and not boolean and thus it is more appropriate to print it as integer >>> to avoid confusion. >> >> I can buy this line of reasoning, however, it would be even easier to >> sell the change if you cited an existing client (a git command) which >> actually respects multiple quiet levels. Are there any? > > I investigated into this. But I was unable to find any git commit > which actually respects mulitple quiet levels. I first did a 'git grep > OPT__QUIET' to find the commands which use this. Then I went through > the documentation which covers it. None of them have any such mention > of multiple quiet levels. But still I dug further and and went through > all the individual source files. I followed the corresponding C source > code for the header file included and also searched there for any > trace of quiet. But I still didn't find any such use of multiple quiet > levels. I have found that the commit id 212c0a6f (Duy Ngyuyen; 7 Dec, > 2013; parse-options: remove OPT__BOOLEAN). Maybe he has something to > say as to why this was introduced and OPT__QUIET which previously used > OPT__BOOLEAN, now uses OPT_COUNTUP rather than OPT_BOOL. This commit > In fact git-repack command has quiet but this is not mentioned in the > documentation. If someone could include this it in the documentation. > I would do it but I am not quite familiar with git-repack and haven't > really used it anytime. I didn't find any existing Git command recognizing multiple quiet levels either, however, I think I can still buy the change considering that there is existing precedent in other Unix commands. ``` ## Duy Nguyen, 2016-04-08 11:33 Subject: Re: [PATCH v12 2/5] test-parse-options: print quiet as integer Message-ID: URL: https://gitlist.dev/e/CACsJy8Bru3ZW_g2HA3yNt_yc6fV9DfWLY1BHWzVWWxxrtLp-9A%40mail.gmail.com In-Reply-To: ``` On Tue, Apr 5, 2016 at 10:39 PM, Pranit Bauva wrote: > [+cc:Duy Nguyen, Jonathan Nieder] > > On Mon, Apr 4, 2016 at 3:00 AM, Eric Sunshine wrote: >> On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote: >>> Current implementation of parse-options.c treats OPT__QUIET() as integer >>> and not boolean and thus it is more appropriate to print it as integer >>> to avoid confusion. >> >> I can buy this line of reasoning, however, it would be even easier to >> sell the change if you cited an existing client (a git command) which >> actually respects multiple quiet levels. Are there any? > > I investigated into this. But I was unable to find any git commit > which actually respects mulitple quiet levels. I first did a 'git grep > OPT__QUIET' to find the commands which use this. Then I went through > the documentation which covers it. None of them have any such mention > of multiple quiet levels. But still I dug further and and went through > all the individual source files. I followed the corresponding C source > code for the header file included and also searched there for any > trace of quiet. But I still didn't find any such use of multiple quiet > levels. I have found that the commit id 212c0a6f (Duy Ngyuyen; 7 Dec, > 2013; parse-options: remove OPT__BOOLEAN). Maybe he has something to > say as to why this was introduced and OPT__QUIET which previously used > OPT__BOOLEAN, now uses OPT_COUNTUP rather than OPT_BOOL. I don't have much to say because my commit is a harmless conversion :) OPT_BOOLEAN _is_ OPT_COUNTUP with a misleading name, see b04ba2b (parse-options: deprecate OPT_BOOLEAN - 2011-09-27). If you dig further back, both OPT_VERBOSE and OPT_QUIET are introduced at the same time in 0ce865b (Add shortcuts for very often used options. - 2007-10-14) and they both are defined with OPT_BOOLEAN. I guess it was just an oversight that OPT_QUIET is defined as OPT_BOOLEAN instead of OPT_BOOL. -- Duy ``` ## Junio C Hamano, 2016-04-08 16:52 Subject: Re: [PATCH v12 2/5] test-parse-options: print quiet as integer Message-ID: URL: https://gitlist.dev/e/xmqq4mbcxctd.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Eric Sunshine writes: > On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva wrote: >> Current implementation of parse-options.c treats OPT__QUIET() as integer >> and not boolean and thus it is more appropriate to print it as integer >> to avoid confusion. > > I can buy this line of reasoning, however, it would be even easier to > sell the change if you cited an existing client (a git command) which > actually respects multiple quiet levels. Are there any? This is the only one I found in Documentation/ -q:: --quiet:: Make 'git svn' less verbose. Specify a second time to make it even less verbose. It probably is not a good idea to use OPT__QUIET() and try to do multiple classes of quietness in the longer term anyway. New work should be done to improve OPT_VERBOSITY() that already can do "even more verbose" and "even more quiet" with a single variable; currently there is no way to ask "has any verbosity ever been given from the command line?", and we may want to support that. ``` ## Pranit Bauva, 2016-04-09 12:23 Subject: [PATCH v13 1/6] t0040-test-parse-options.sh: fix style issues Message-ID: <01020153faf9fc14-e6ad18fa-c892-4601-bab7-e2cc4cd9e100-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020153faf9fc14-e6ad18fa-c892-4601-bab7-e2cc4cd9e100-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153d952bd99-d3812bd6-d189-4780-ab48-f015696e9cf0-000000@eu-west-1.amazonses.com> ``` Signed-off-by: Pranit Bauva --- Changes wrt previous version (v12): - Use '\' when interpolation isn't required --- t/t0040-parse-options.sh | 76 ++++++++++++++++++++++++------------------------ 1 file changed, 38 insertions(+), 38 deletions(-) diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index 9be6411..477fcff 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -7,7 +7,7 @@ test_description='our own option parser' . ./test-lib.sh -cat > expect << EOF +cat >expect <<\EOF usage: test-parse-options --yes get a boolean @@ -49,14 +49,14 @@ Standard options EOF test_expect_success 'test help' ' - test_must_fail test-parse-options -h > output 2> output.err && + test_must_fail test-parse-options -h >output 2>output.err && test_must_be_empty output.err && test_i18ncmp expect output ' mv expect expect.err -cat >expect.template <expect.template <<\EOF boolean: 0 integer: 0 magnitude: 0 @@ -156,7 +156,7 @@ test_expect_success 'OPT_MAGNITUDE() 3giga' ' check magnitude: 3221225472 -m 3g ' -cat > expect << EOF +cat >expect <<\EOF boolean: 2 integer: 1729 magnitude: 16384 @@ -176,7 +176,7 @@ test_expect_success 'short options' ' test_must_be_empty output.err ' -cat > expect << EOF +cat >expect <<\EOF boolean: 2 integer: 1729 magnitude: 16384 @@ -204,7 +204,7 @@ test_expect_success 'missing required value' ' test_expect_code 129 test-parse-options --file ' -cat > expect << EOF +cat >expect <<\EOF boolean: 1 integer: 13 magnitude: 0 @@ -222,12 +222,12 @@ EOF test_expect_success 'intermingled arguments' ' test-parse-options a1 --string 123 b1 --boolean -j 13 -- --boolean \ - > output 2> output.err && + >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > expect << EOF +cat >expect <<\EOF boolean: 0 integer: 2 magnitude: 0 @@ -241,13 +241,13 @@ file: (not set) EOF test_expect_success 'unambiguously abbreviated option' ' - test-parse-options --int 2 --boolean --no-bo > output 2> output.err && + test-parse-options --int 2 --boolean --no-bo >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' test_expect_success 'unambiguously abbreviated option with "="' ' - test-parse-options --int=2 > output 2> output.err && + test-parse-options --int=2 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' @@ -256,7 +256,7 @@ test_expect_success 'ambiguously abbreviated option' ' test_expect_code 129 test-parse-options --strin 123 ' -cat > expect << EOF +cat >expect <<\EOF boolean: 0 integer: 0 magnitude: 0 @@ -270,32 +270,32 @@ file: (not set) EOF test_expect_success 'non ambiguous option (after two options it abbreviates)' ' - test-parse-options --st 123 > output 2> output.err && + test-parse-options --st 123 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > typo.err << EOF -error: did you mean \`--boolean\` (with two dashes ?) +cat >typo.err <<\EOF +error: did you mean `--boolean` (with two dashes ?) EOF test_expect_success 'detect possible typos' ' - test_must_fail test-parse-options -boolean > output 2> output.err && + test_must_fail test-parse-options -boolean >output 2>output.err && test_must_be_empty output && test_cmp typo.err output.err ' -cat > typo.err << EOF -error: did you mean \`--ambiguous\` (with two dashes ?) +cat >typo.err <<\EOF +error: did you mean `--ambiguous` (with two dashes ?) EOF test_expect_success 'detect possible typos' ' - test_must_fail test-parse-options -ambiguous > output 2> output.err && + test_must_fail test-parse-options -ambiguous >output 2>output.err && test_must_be_empty output && test_cmp typo.err output.err ' -cat > expect <expect <<\EOF boolean: 0 integer: 0 magnitude: 0 @@ -310,12 +310,12 @@ arg 00: --quux EOF test_expect_success 'keep some options as arguments' ' - test-parse-options --quux > output 2> output.err && + test-parse-options --quux >output 2>output.err && test_must_be_empty output.err && - test_cmp expect output + test_cmp expect output ' -cat > expect <expect <<\EOF boolean: 0 integer: 0 magnitude: 0 @@ -331,12 +331,12 @@ EOF test_expect_success 'OPT_DATE() works' ' test-parse-options -t "1970-01-01 00:00:01 +0000" \ - foo -q > output 2> output.err && + foo -q >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > expect <expect <<\EOF Callback: "four", 0 boolean: 5 integer: 4 @@ -351,22 +351,22 @@ file: (not set) EOF test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' ' - test-parse-options --length=four -b -4 > output 2> output.err && + test-parse-options --length=four -b -4 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > expect <expect <<\EOF Callback: "not set", 1 EOF test_expect_success 'OPT_CALLBACK() and callback errors work' ' - test_must_fail test-parse-options --no-length > output 2> output.err && + test_must_fail test-parse-options --no-length >output 2>output.err && test_i18ncmp expect output && test_i18ncmp expect.err output.err ' -cat > expect <expect <<\EOF boolean: 1 integer: 23 magnitude: 0 @@ -380,18 +380,18 @@ file: (not set) EOF test_expect_success 'OPT_BIT() and OPT_SET_INT() work' ' - test-parse-options --set23 -bbbbb --no-or4 > output 2> output.err && + test-parse-options --set23 -bbbbb --no-or4 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' test_expect_success 'OPT_NEGBIT() and OPT_SET_INT() work' ' - test-parse-options --set23 -bbbbb --neg-or4 > output 2> output.err && + test-parse-options --set23 -bbbbb --neg-or4 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > expect <expect <<\EOF boolean: 6 integer: 0 magnitude: 0 @@ -405,24 +405,24 @@ file: (not set) EOF test_expect_success 'OPT_BIT() works' ' - test-parse-options -bb --or4 > output 2> output.err && + test-parse-options -bb --or4 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' test_expect_success 'OPT_NEGBIT() works' ' - test-parse-options -bb --no-neg-or4 > output 2> output.err && + test-parse-options -bb --no-neg-or4 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' test_expect_success 'OPT_COUNTUP() with PARSE_OPT_NODASH works' ' - test-parse-options + + + + + + > output 2> output.err && + test-parse-options + + + + + + >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat > expect <expect <<\EOF boolean: 0 integer: 12345 magnitude: 0 @@ -436,12 +436,12 @@ file: (not set) EOF test_expect_success 'OPT_NUMBER_CALLBACK() works' ' - test-parse-options -12345 > output 2> output.err && + test-parse-options -12345 >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ' -cat >expect <expect <<\EOF boolean: 0 integer: 0 magnitude: 0 @@ -460,7 +460,7 @@ test_expect_success 'negation of OPT_NONEG flags is not ambiguous' ' test_cmp expect output ' -cat >>expect <<'EOF' +cat >>expect <<\EOF list: foo list: bar list: baz -- https://github.com/git/git/pull/218 ``` ## Pranit Bauva, 2016-04-09 12:23 Subject: [PATCH v13 6/6] commit: add a commit.verbose config variable Message-ID: <01020153faf9fcd0-879c8263-6143-4ad2-a3b3-deb78c93246a-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020153faf9fcd0-879c8263-6143-4ad2-a3b3-deb78c93246a-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153faf9fc14-e6ad18fa-c892-4601-bab7-e2cc4cd9e100-000000@eu-west-1.amazonses.com> ``` Add commit.verbose configuration variable as a convenience for those who always prefer --verbose. Helped-by: Junio C Hamano Helped-by: Eric Sunshine Signed-off-by: Pranit Bauva --- The previous version of the patch are: - [v12] $gmane/288820 - [v11] $gmane/288820 - [v10] $gmane/288820 - [v9] $gmane/288820 - [v8] $gmane/288820 - [v7] $gmane/288820 - [v6] $gmane/288728 - [v5] $gmane/288728 - [v4] $gmane/288652 - [v3] $gmane/288634 - [v2] $gmane/288569 - [v1] $gmane/287540 Note: One might think some tests are extra but I think that it will be better to include them as they "complete the continuity" thus generalising the series which will make the patch even more clearer. Changes wrt v12: - Change the setup test as suggested by Eric. - Use for loops for systematic testing. I have avoided nested-for loops or using a function to test because I believe it will just add to the complexity rather than simplifying it. Just by using for loops the tests look quite systematic and easy to digest. --- Documentation/config.txt | 4 ++++ Documentation/git-commit.txt | 3 ++- builtin/commit.c | 14 ++++++++++- t/t7507-commit-verbose.sh | 56 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 75 insertions(+), 2 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 2cd6bdd..1d0ec2e 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1110,6 +1110,10 @@ commit.template:: "`~/`" is expanded to the value of `$HOME` and "`~user/`" to the specified user's home directory. +commit.verbose:: + A boolean or int to specify the level of verbose with `git commit`. + See linkgit:git-commit[1]. + credential.helper:: Specify an external helper to be called when a username or password credential is needed; the helper may consult external diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt index 9ec6b3c..d474226 100644 --- a/Documentation/git-commit.txt +++ b/Documentation/git-commit.txt @@ -290,7 +290,8 @@ configuration variable documented in linkgit:git-config[1]. what changes the commit has. Note that this diff output doesn't have its lines prefixed with '#'. This diff will not be a part - of the commit message. + of the commit message. See the `commit.verbose` configuration + variable in linkgit:git-config[1]. + If specified twice, show in addition the unified diff between what would be committed and the worktree files, i.e. the unstaged diff --git a/builtin/commit.c b/builtin/commit.c index b3bd2d4..96e6190 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -113,7 +113,9 @@ static char *edit_message, *use_message; static char *fixup_message, *squash_message; static int all, also, interactive, patch_interactive, only, amend, signoff; static int edit_flag = -1; /* unspecified */ -static int quiet, verbose, no_verify, allow_empty, dry_run, renew_authorship; +static int config_verbose = -1; /* unspecified */ +static int verbose = -1; /* unspecified */ +static int quiet, no_verify, allow_empty, dry_run, renew_authorship; static int no_post_rewrite, allow_empty_message; static char *untracked_files_arg, *force_date, *ignore_submodule_arg; static char *sign_commit; @@ -1354,6 +1356,8 @@ int cmd_status(int argc, const char **argv, const char *prefix) builtin_status_usage, 0); finalize_colopts(&s.colopts, -1); finalize_deferred_config(&s); + if (verbose == -1) + verbose = 0; handle_untracked_files_arg(&s); if (show_ignored_in_status) @@ -1505,6 +1509,11 @@ static int git_commit_config(const char *k, const char *v, void *cb) sign_commit = git_config_bool(k, v) ? "" : NULL; return 0; } + if (!strcmp(k, "commit.verbose")) { + int is_bool; + config_verbose = git_config_bool_or_int(k, v, &is_bool); + return 0; + } status = git_gpg_config(k, v, NULL); if (status) @@ -1654,6 +1663,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix) argc = parse_and_validate_options(argc, argv, builtin_commit_options, builtin_commit_usage, prefix, current_head, &s); + if (verbose == -1) + verbose = (config_verbose < 0) ? 0 : config_verbose; + if (dry_run) return dry_run_commit(argc, argv, prefix, current_head, &s); index_file = prepare_index(argc, argv, prefix, current_head, 0); diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh index 0f28a86..00e0c3d 100755 --- a/t/t7507-commit-verbose.sh +++ b/t/t7507-commit-verbose.sh @@ -98,4 +98,60 @@ test_expect_success 'verbose diff is stripped out with set core.commentChar' ' test_i18ngrep "Aborting commit due to empty commit message." err ' +test_expect_success 'setup -v -v' ' + echo dirty >file +' + +for i in true 1 +do + test_expect_success "commit.verbose=$i and --verbose omitted" " + git -c commit.verbose=$i commit --amend && + test_line_count = 1 out + " +done + +for i in false -2 -1 0 +do + test_expect_success "commit.verbose=$i and --verbose omitted" " + git -c commit.verbose=$i commit --amend && + test_line_count = 0 out + " +done + +for i in 2 3 +do + test_expect_success "commit.verbose=$i and --verbose omitted" " + git -c commit.verbose=$i commit --amend && + test_line_count = 2 out + " +done + +for i in true false -2 -1 0 1 2 3 +do + test_expect_success "commit.verbose=$i and --verbose" " + git -c commit.verbose=$i commit --amend --verbose && + test_line_count = 1 out + " + + test_expect_success "commit.verbose=$i and --no-verbose" " + git -c commit.verbose=$i commit --amend --no-verbose && + test_line_count = 0 out + " + + test_expect_success "commit.verbose=$i and -v -v" " + git -c commit.verbose=$i commit --amend -v -v && + test_line_count = 2 out + " +done + +test_expect_success 'status ignores commit.verbose=true' ' + git -c commit.verbose=true status >actual && + ! grep "^diff --git" actual +' + +test_expect_success 'status does not verbose without --verbose' ' + git status >actual && + ! grep "^diff --git" actual +' + test_done -- https://github.com/git/git/pull/218 ``` ## Pranit Bauva, 2016-04-09 12:23 Subject: [PATCH v13 5/6] t7507-commit-verbose: improve test coverage by testing number of diffs Message-ID: <01020153faf9fcc9-b883c097-517a-449f-914e-f3f378d3e6f8-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020153faf9fcc9-b883c097-517a-449f-914e-f3f378d3e6f8-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153faf9fc14-e6ad18fa-c892-4601-bab7-e2cc4cd9e100-000000@eu-west-1.amazonses.com> ``` Make the fake "editor" store output of grep in a file so that we can see how many diffs were contained in the message and use them in individual tests where ever it is required. A subsequent commit will introduce scenarios where it is important to be able to exactly determine how many diffs were present. Also use write_script() to create the fake "editor". The fake "editor" is always made to succeed regardless of whether grep found diff headers or not so that we don't have to use 'test_must_fail' for which 'test_line_count = 0' is an easy substitute. Helped-by: Eric Sunshine Signed-off-by: Pranit Bauva --- Previous version of this patch: - [v12] : $gmane/288820 - [v11] : $gmane/288820 - [v10]: $gmane/288820 Changes this version wrt previous one: Change the commit message as suggested by Eric --- t/t7507-commit-verbose.sh | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh index 2ddf28c..0f28a86 100755 --- a/t/t7507-commit-verbose.sh +++ b/t/t7507-commit-verbose.sh @@ -3,11 +3,10 @@ test_description='verbose commit template' . ./test-lib.sh -cat >check-for-diff <out +exit 0 EOF -chmod +x check-for-diff test_set_editor "$PWD/check-for-diff" cat >message <<'EOF' @@ -23,7 +22,8 @@ test_expect_success 'setup' ' ' test_expect_success 'initial commit shows verbose diff' ' - git commit --amend -v + git commit --amend -v && + test_line_count = 1 out ' test_expect_success 'second commit' ' @@ -39,13 +39,15 @@ check_message() { test_expect_success 'verbose diff is stripped out' ' git commit --amend -v && - check_message message + check_message message && + test_line_count = 1 out ' test_expect_success 'verbose diff is stripped out (mnemonicprefix)' ' git config diff.mnemonicprefix true && git commit --amend -v && - check_message message + check_message message && + test_line_count = 1 out ' cat >diff <<'EOF' -- https://github.com/git/git/pull/218 ``` ## Pranit Bauva, 2016-04-09 12:23 Subject: [PATCH v13 2/6] test-parse-options: print quiet as integer Message-ID: <01020153faf9fcbb-adadd774-db3a-43ce-b908-d08ac1312b31-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020153faf9fcbb-adadd774-db3a-43ce-b908-d08ac1312b31-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153faf9fc14-e6ad18fa-c892-4601-bab7-e2cc4cd9e100-000000@eu-west-1.amazonses.com> ``` Current implementation of parse-options.c treats OPT__QUIET() as integer and not boolean and thus it is more appropriate to print it as integer to avoid confusion. Signed-off-by: Pranit Bauva --- t/t0040-parse-options.sh | 26 +++++++++++++------------- test-parse-options.c | 2 +- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index 477fcff..450da45 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -64,7 +64,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF @@ -164,7 +164,7 @@ timestamp: 0 string: 123 abbrev: 7 verbose: 2 -quiet: no +quiet: 0 dry run: yes file: prefix/my.file EOF @@ -184,7 +184,7 @@ timestamp: 0 string: 321 abbrev: 10 verbose: 2 -quiet: no +quiet: 0 dry run: no file: prefix/fi.le EOF @@ -212,7 +212,7 @@ timestamp: 0 string: 123 abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) arg 00: a1 @@ -235,7 +235,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF @@ -264,7 +264,7 @@ timestamp: 0 string: 123 abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF @@ -303,7 +303,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) arg 00: --quux @@ -323,7 +323,7 @@ timestamp: 1 string: (not set) abbrev: 7 verbose: 0 -quiet: yes +quiet: 1 dry run: no file: (not set) arg 00: foo @@ -345,7 +345,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF @@ -374,7 +374,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF @@ -399,7 +399,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF @@ -430,7 +430,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF @@ -449,7 +449,7 @@ timestamp: 0 string: (not set) abbrev: 7 verbose: 0 -quiet: no +quiet: 0 dry run: no file: (not set) EOF diff --git a/test-parse-options.c b/test-parse-options.c index 2c8c8f1..86afa98 100644 --- a/test-parse-options.c +++ b/test-parse-options.c @@ -90,7 +90,7 @@ int main(int argc, char **argv) printf("string: %s\n", string ? string : "(not set)"); printf("abbrev: %d\n", abbrev); printf("verbose: %d\n", verbose); - printf("quiet: %s\n", quiet ? "yes" : "no"); + printf("quiet: %d\n", quiet); printf("dry run: %s\n", dry_run ? "yes" : "no"); printf("file: %s\n", file ? file : "(not set)"); -- https://github.com/git/git/pull/218 ``` ## Pranit Bauva, 2016-04-09 12:23 Subject: [PATCH v13 3/6] t0040-parse-options: improve test coverage Message-ID: <01020153faf9fcbf-e99e4a5b-b892-4e4a-8de5-84ee4315f01d-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020153faf9fcbf-e99e4a5b-b892-4e4a-8de5-84ee4315f01d-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153faf9fc14-e6ad18fa-c892-4601-bab7-e2cc4cd9e100-000000@eu-west-1.amazonses.com> ``` Include tests to check for multiple levels of quiet and to check if the '--no-quiet' option sets it to 0. Signed-off-by: Pranit Bauva --- t/t0040-parse-options.sh | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index 450da45..c913d1c 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -476,4 +476,41 @@ test_expect_success '--no-list resets list' ' test_cmp expect output ' +cat >expect <<\EOF +boolean: 0 +integer: 0 +magnitude: 0 +timestamp: 0 +string: (not set) +abbrev: 7 +verbose: 0 +quiet: 3 +dry run: no +file: (not set) +EOF + +test_expect_success 'multiple quiet levels' ' + test-parse-options -q -q -q >output 2>output.err && + test_must_be_empty output.err && + test_cmp expect output +' + +cat >expect <<\EOF +boolean: 0 +integer: 0 +magnitude: 0 +timestamp: 0 +string: (not set) +abbrev: 7 +verbose: 0 +quiet: 0 +dry run: no +file: (not set) +EOF + +test_expect_success '--no-quiet sets quiet to 0' ' + test-parse-options --no-quiet >output 2>output.err && + test_must_be_empty output.err && + test_cmp expect output +' test_done -- https://github.com/git/git/pull/218 ``` ## Pranit Bauva, 2016-04-09 12:23 Subject: [PATCH v13 4/6] parse-options.c: make OPTION_COUNTUP respect "unspecified" values Message-ID: <01020153faf9fcca-f9fc4f38-7432-4f5c-831d-0274eb5ec992-000000@eu-west-1.amazonses.com> URL: https://gitlist.dev/e/01020153faf9fcca-f9fc4f38-7432-4f5c-831d-0274eb5ec992-000000%40eu-west-1.amazonses.com In-Reply-To: <01020153faf9fc14-e6ad18fa-c892-4601-bab7-e2cc4cd9e100-000000@eu-west-1.amazonses.com> ``` OPT_COUNTUP() merely increments the counter upon --option, and resets it to 0 upon --no-option, which means that there is no "unspecified" value with which a client can initialize the counter to determine whether or not --[no]-option was seen at all. Make OPT_COUNTUP() treat any negative number as an "unspecified" value to address this shortcoming. In particular, if a client initializes the counter to -1, then if it is still -1 after parse_options(), then neither --option nor --no-option was seen; If it is 0, then --no-option was seen last, and if it is 1 or greater, than --option was seen last. This change does not affect the behavior of existing clients because they all use the initial value of 0 (or more). Note that builtin/clean.c initializes the variable used with OPT__FORCE (which uses OPT_COUNTUP()) to a negative value, but it is set to either 0 or 1 by reading the configuration before the code calls parse_options(), i.e. as far as parse_options() is concerned, the initial value of the variable is not negative. To test this behavior, in test-parse-options.c, "verbose" is set to "unspecified" while quiet is set to 0 which will test the new behavior with all sets of values. Helped-by: Jeff King Helped-by: Eric Sunshine Helped-by: Junio C Hamano Signed-off-by: Pranit Bauva --- The discussion about this patch: [1] : http://thread.gmane.org/gmane.comp.version-control.git/289027 Previous version of the patch: [v11] : http://thread.gmane.org/gmane.comp.version-control.git/288820 [v10] : http://thread.gmane.org/gmane.comp.version-control.git/288820 [v9] : http://thread.gmane.org/gmane.comp.version-control.git/288820 [v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061 Changes wrt previous version (v12): - Use bits of commit message provided by Eric Sunshine. Please Note: The diff might seem improper especially the part where I have introduced some continuous lines but this is a logical error by git diff (nothing could be done about it) and thus the changes will be clearly visible with the original file itself. --- Documentation/technical/api-parse-options.txt | 8 ++++-- parse-options.c | 2 ++ t/t0040-parse-options.sh | 39 ++++++++++++++++++++------- test-parse-options.c | 3 ++- 4 files changed, 39 insertions(+), 13 deletions(-) diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt index 5f0757d..8908bf7 100644 --- a/Documentation/technical/api-parse-options.txt +++ b/Documentation/technical/api-parse-options.txt @@ -144,8 +144,12 @@ There are some macros to easily define options: `OPT_COUNTUP(short, long, &int_var, description)`:: Introduce a count-up option. - `int_var` is incremented on each use of `--option`, and - reset to zero with `--no-option`. + Each use of `--option` increments `int_var`, starting from zero + (even if initially negative), and `--no-option` resets it to + zero. To determine if `--option` or `--no-option` was set at + all, set `int_var` to a negative value, and if it is still + negative after parse_options(), then neither `--option` nor + `--no-option` was seen. `OPT_BIT(short, long, &int_var, description, mask)`:: Introduce a boolean option. diff --git a/parse-options.c b/parse-options.c index 47a9192..312a85d 100644 --- a/parse-options.c +++ b/parse-options.c @@ -110,6 +110,8 @@ static int get_value(struct parse_opt_ctx_t *p, return 0; case OPTION_COUNTUP: + if (*(int *)opt->value < 0) + *(int *)opt->value = 0; *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; return 0; diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh index c913d1c..efcd844 100755 --- a/t/t0040-parse-options.sh +++ b/t/t0040-parse-options.sh @@ -63,7 +63,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -211,7 +211,7 @@ magnitude: 0 timestamp: 0 string: 123 abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -234,7 +234,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -263,7 +263,7 @@ magnitude: 0 timestamp: 0 string: 123 abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -302,7 +302,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -322,7 +322,7 @@ magnitude: 0 timestamp: 1 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 1 dry run: no file: (not set) @@ -344,7 +344,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -373,7 +373,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -398,7 +398,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -429,7 +429,7 @@ magnitude: 0 timestamp: 0 string: (not set) abbrev: 7 -verbose: 0 +verbose: -1 quiet: 0 dry run: no file: (not set) @@ -454,6 +454,25 @@ dry run: no file: (not set) EOF +test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' ' + test-parse-options --no-verbose >output 2>output.err && + test_must_be_empty output.err && + test_cmp expect output +' + +cat >expect <output 2>output.err && test_must_be_empty output.err && diff --git a/test-parse-options.c b/test-parse-options.c index 86afa98..f02c275 100644 --- a/test-parse-options.c +++ b/test-parse-options.c @@ -7,7 +7,8 @@ static int integer = 0; static unsigned long magnitude = 0; static unsigned long timestamp; static int abbrev = 7; -static int verbose = 0, dry_run = 0, quiet = 0; +static int verbose = -1; /* unspecified */ +static int dry_run = 0, quiet = 0; static char *string = NULL; static char *file = NULL; static int ambiguous; -- https://github.com/git/git/pull/218 ``` ## Junio C Hamano, 2016-04-12 21:24 Subject: Re: [PATCH v13 6/6] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/xmqqa8kyplkp.fsf%40gitster.mtv.corp.google.com In-Reply-To: <01020153faf9fcd0-879c8263-6143-4ad2-a3b3-deb78c93246a-000000@eu-west-1.amazonses.com> ``` Hmph, isn't this already in 'next', hence we cannot accept a replacement patch? ``` ## Pranit Bauva, 2016-04-12 21:28 Subject: Re: [PATCH v13 6/6] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAFZEwPO5uYxh%3D9FetsNqsDxfGw6HcBWYZg76BcFL4J_2fKAXEw%40mail.gmail.com In-Reply-To: ``` On Wed, Apr 13, 2016 at 2:54 AM, Junio C Hamano wrote: > Hmph, isn't this already in 'next', hence we cannot accept a > replacement patch? Yes, this is already in 'next'. This was going to be merged in the third cycle after 2.8 but on my request, you delayed it. So this is an update on the patch. Is it that once a patch is in 'next', it cannot be replaced with a new updated one? There are 5 other patches along with this which are a requirement for this patch. ``` ## Junio C Hamano, 2016-04-12 21:33 Subject: Re: [PATCH v13 2/6] test-parse-options: print quiet as integer Message-ID: URL: https://gitlist.dev/e/xmqq60vmpl5c.fsf%40gitster.mtv.corp.google.com In-Reply-To: <01020153faf9fcbb-adadd774-db3a-43ce-b908-d08ac1312b31-000000@eu-west-1.amazonses.com> ``` Pranit Bauva writes: > Current implementation of parse-options.c treats OPT__QUIET() as integer > and not boolean and thus it is more appropriate to print it as integer > to avoid confusion. There is no "confusion" factor involved, as we do not use native "boolean" type in our C code. IIUC, the reason why we want to do this is because we may want to see how it would affect the value of the underlying variable to give multiple --quiet options from the command line, which is a policy issue (i.e. we want to allow commands to react to multiple quiet options differently), not an implementation one (i.e. "current implementation happens to use integer"). We would want to see how multiple --quiet options affect the value of the underlying variable (we may want "--quiet --quiet" to still be 1, or we may want to see the value incremented to 2). Show the value as integer to allow us to inspect it. perhaps? > > Signed-off-by: Pranit Bauva > --- > t/t0040-parse-options.sh | 26 +++++++++++++------------- > test-parse-options.c | 2 +- > 2 files changed, 14 insertions(+), 14 deletions(-) > > diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh > index 477fcff..450da45 100755 > --- a/t/t0040-parse-options.sh > +++ b/t/t0040-parse-options.sh > @@ -64,7 +64,7 @@ timestamp: 0 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > EOF > @@ -164,7 +164,7 @@ timestamp: 0 > string: 123 > abbrev: 7 > verbose: 2 > -quiet: no > +quiet: 0 > dry run: yes > file: prefix/my.file > EOF > @@ -184,7 +184,7 @@ timestamp: 0 > string: 321 > abbrev: 10 > verbose: 2 > -quiet: no > +quiet: 0 > dry run: no > file: prefix/fi.le > EOF > @@ -212,7 +212,7 @@ timestamp: 0 > string: 123 > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > arg 00: a1 > @@ -235,7 +235,7 @@ timestamp: 0 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > EOF > @@ -264,7 +264,7 @@ timestamp: 0 > string: 123 > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > EOF > @@ -303,7 +303,7 @@ timestamp: 0 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > arg 00: --quux > @@ -323,7 +323,7 @@ timestamp: 1 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: yes > +quiet: 1 > dry run: no > file: (not set) > arg 00: foo > @@ -345,7 +345,7 @@ timestamp: 0 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > EOF > @@ -374,7 +374,7 @@ timestamp: 0 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > EOF > @@ -399,7 +399,7 @@ timestamp: 0 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > EOF > @@ -430,7 +430,7 @@ timestamp: 0 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > EOF > @@ -449,7 +449,7 @@ timestamp: 0 > string: (not set) > abbrev: 7 > verbose: 0 > -quiet: no > +quiet: 0 > dry run: no > file: (not set) > EOF > diff --git a/test-parse-options.c b/test-parse-options.c > index 2c8c8f1..86afa98 100644 > --- a/test-parse-options.c > +++ b/test-parse-options.c > @@ -90,7 +90,7 @@ int main(int argc, char **argv) > printf("string: %s\n", string ? string : "(not set)"); > printf("abbrev: %d\n", abbrev); > printf("verbose: %d\n", verbose); > - printf("quiet: %s\n", quiet ? "yes" : "no"); > + printf("quiet: %d\n", quiet); > printf("dry run: %s\n", dry_run ? "yes" : "no"); > printf("file: %s\n", file ? file : "(not set)"); > > > -- > https://github.com/git/git/pull/218 ``` ## Junio C Hamano, 2016-04-12 21:39 Subject: Re: [PATCH v13 6/6] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/xmqqy48io6bh.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Junio C Hamano writes: > Hmph, isn't this already in 'next', hence we cannot accept a > replacement patch? As a one-time measure, I'll revert the previous one 50f0d20d (commit: add a commit.verbose config variable, 2016-03-14) out of 'next' and queue this one instead on 'pu'. ``` ## Pranit Bauva, 2016-04-12 22:16 Subject: Re: [PATCH v13 2/6] test-parse-options: print quiet as integer Message-ID: URL: https://gitlist.dev/e/CAFZEwPPSY%2Bk9qc8hZ685o9_U3UktbgK-e00zrG2%2B7GehG-47dQ%40mail.gmail.com In-Reply-To: ``` On Wed, Apr 13, 2016 at 3:03 AM, Junio C Hamano wrote: > Pranit Bauva writes: > >> Current implementation of parse-options.c treats OPT__QUIET() as integer >> and not boolean and thus it is more appropriate to print it as integer >> to avoid confusion. > > There is no "confusion" factor involved, as we do not use native > "boolean" type in our C code. IIUC, the reason why we want to do > this is because we may want to see how it would affect the value of > the underlying variable to give multiple --quiet options from the > command line, which is a policy issue (i.e. we want to allow > commands to react to multiple quiet options differently), not an > implementation one (i.e. "current implementation happens to use > integer"). > > We would want to see how multiple --quiet options affect the > value of the underlying variable (we may want "--quiet --quiet" > to still be 1, or we may want to see the value incremented > to 2). Show the value as integer to allow us to inspect it. > > perhaps? This commit message does look a lot better. I will re-roll this. Should I just send an update of this patch or the whole series? ``` ## Junio C Hamano, 2016-04-12 22:18 Subject: Re: [PATCH v13 6/6] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/xmqqtwj6o4ip.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Junio C Hamano writes: > Junio C Hamano writes: > >> Hmph, isn't this already in 'next', hence we cannot accept a >> replacement patch? > > As a one-time measure, I'll revert the previous one > > 50f0d20d (commit: add a commit.verbose config variable, 2016-03-14) > > out of 'next' and queue this one instead on 'pu'. I queued these 6 patches on 'master' and merged to 'pu', but it seems the series breaks t0040, so in the meantime, I ejected the whole thing. ``` ## Pranit Bauva, 2016-04-12 22:25 Subject: Re: [PATCH v13 6/6] commit: add a commit.verbose config variable Message-ID: URL: https://gitlist.dev/e/CAFZEwPMvBBjp2%3Dydq1oRcdc-d7oT68mYWC%3DPFqKe4fv7brGkvw%40mail.gmail.com In-Reply-To: ``` On Wed, Apr 13, 2016 at 3:48 AM, Junio C Hamano wrote: > Junio C Hamano writes: > >> Junio C Hamano writes: >> >>> Hmph, isn't this already in 'next', hence we cannot accept a >>> replacement patch? >> >> As a one-time measure, I'll revert the previous one >> >> 50f0d20d (commit: add a commit.verbose config variable, 2016-03-14) >> >> out of 'next' and queue this one instead on 'pu'. > > I queued these 6 patches on 'master' and merged to 'pu', but it > seems the series breaks t0040, so in the meantime, I ejected the > whole thing. I forgot to update the newly introduced tests in this series for the new behaviour. I will send an re-roll ``` ## Junio C Hamano, 2016-04-12 23:11 Subject: Re: [PATCH v13 2/6] test-parse-options: print quiet as integer Message-ID: URL: https://gitlist.dev/e/xmqqd1puo21a.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Pranit Bauva writes: > On Wed, Apr 13, 2016 at 3:03 AM, Junio C Hamano wrote: >> Pranit Bauva writes: >> >>> Current implementation of parse-options.c treats OPT__QUIET() as integer >>> and not boolean and thus it is more appropriate to print it as integer >>> to avoid confusion. >> >> There is no "confusion" factor involved, as we do not use native >> "boolean" type in our C code. IIUC, the reason why we want to do >> this is because we may want to see how it would affect the value of >> the underlying variable to give multiple --quiet options from the >> command line, which is a policy issue (i.e. we want to allow >> commands to react to multiple quiet options differently), not an >> implementation one (i.e. "current implementation happens to use >> integer"). >> >> We would want to see how multiple --quiet options affect the >> value of the underlying variable (we may want "--quiet --quiet" >> to still be 1, or we may want to see the value incremented >> to 2). Show the value as integer to allow us to inspect it. >> >> perhaps? > > This commit message does look a lot better. I will re-roll this. You need to pick which one you want inside the (parentheses), though. After reading 3/6 I am guessing that that you wanted the latter. > Should I just send an update of this patch or the whole series? The latter, for a series this small. Thanks. ```