[PATCH v15 1/7] t0040-test-parse-options.sh: fix style issues

classic Classic list List threaded Threaded
53 messages Options
123
Reply | Threaded
Open this post in threaded view
|

[PATCH v15 1/7] t0040-test-parse-options.sh: fix style issues

pranitbauva1997
Signed-off-by: Pranit Bauva <[hidden email]>

---
Changes wrt previous version (v12):
 - Use '\' when interpolation isn't required

Signed-off-by: Pranit Bauva <[hidden email]>
---
 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 <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 <<EOF
+cat >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 <<EOF
+cat >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 <<EOF
+cat >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 <<EOF
+cat >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 <<EOF
+cat >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 <<EOF
+cat >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 <<EOF
+cat >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 <<EOF
+cat >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 <<EOF
+cat >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
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v15 2/7] test-parse-options: print quiet as integer

pranitbauva1997
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.

Signed-off-by: Pranit Bauva <[hidden email]>
---
 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)");
 
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v15 3/7] t0040-parse-options: improve test coverage

pranitbauva1997
In reply to this post by pranitbauva1997
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 <[hidden email]>

---
Link to v14:
 - $gmane/288880

Changes wrt v14:
 - Change the test to use '-q -q -q --no-quiet' instead of just '--no-quiet'
 - Move the test for '--no-verbose' from OPT_COUNTUP patch to this one.

Signed-off-by: Pranit Bauva <[hidden email]>
---
 t/t0040-parse-options.sh | 57 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 450da45..57fc2a1 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -476,4 +476,61 @@ 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 -q -q -q --no-quiet >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-verbose sets verbose to 0' '
+ test-parse-options --no-verbose >output 2> output.err &&
+ test_must_be_empty output.err &&
+ test_cmp expect output
+'
+
 test_done
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v15 4/7] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

pranitbauva1997
In reply to this post by pranitbauva1997
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 <[hidden email]>
Helped-by: Eric Sunshine <[hidden email]>
Helped-by: Junio C Hamano <[hidden email]>
Signed-off-by: Pranit Bauva <[hidden email]>

---
The discussion about this patch:
[1] : http://thread.gmane.org/gmane.comp.version-control.git/289027

Previous version of the patch:
[v14] : http://thread.gmane.org/gmane.comp.version-control.git/288820

Changes wrt previous version (v14):
 - Remove a test and move it to a preparatory patch.

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.

Signed-off-by: Pranit Bauva <[hidden email]>
---
 Documentation/technical/api-parse-options.txt |  8 ++++++--
 parse-options.c                               |  2 ++
 t/t0040-parse-options.sh                      | 26 +++++++++++++-------------
 test-parse-options.c                          |  3 ++-
 4 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 695bd4b..27bd701 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 encountered at
+ all, initialize `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 57fc2a1..9638ca0 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)
@@ -448,7 +448,7 @@ magnitude: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 0
 dry run: no
 file: (not set)
@@ -483,7 +483,7 @@ magnitude: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 3
 dry run: no
 file: (not set)
@@ -502,7 +502,7 @@ magnitude: 0
 timestamp: 0
 string: (not set)
 abbrev: 7
-verbose: 0
+verbose: -1
 quiet: 0
 dry run: no
 file: (not set)
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;
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v15 5/7] t7507-commit-verbose: improve test coverage by testing number of diffs

pranitbauva1997
In reply to this post by pranitbauva1997
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.

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 and also helps in
maintaining the consistency.

Also use write_script() to create the fake "editor".

Helped-by: Eric Sunshine <[hidden email]>
Signed-off-by: Pranit Bauva <[hidden email]>

---
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

Signed-off-by: Pranit Bauva <[hidden email]>
---
 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 <<EOF
-#!$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
 '
 
 cat >diff <<'EOF'
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v15 6/7] commit: add a commit.verbose config variable

pranitbauva1997
In reply to this post by pranitbauva1997
Add commit.verbose configuration variable as a convenience for those
who always prefer --verbose.

Helped-by: Junio C Hamano <[hidden email]>
Helped-by: Eric Sunshine <[hidden email]>
Signed-off-by: Pranit Bauva <[hidden email]>

---
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 v14:
 - Add the status related tests in a different patch after this patch.

Signed-off-by: Pranit Bauva <[hidden email]>
---
 Documentation/config.txt     |  4 ++++
 Documentation/git-commit.txt |  3 ++-
 builtin/commit.c             | 14 +++++++++++++-
 t/t7507-commit-verbose.sh    | 46 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 42d2b50..8bf6040 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 391126e..114ffc9 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;
@@ -1364,6 +1366,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)
@@ -1515,6 +1519,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)
@@ -1664,6 +1673,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..2bb6d8d 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -98,4 +98,50 @@ 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_done
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v15 7/7] t/t7507: tests for broken behavior of status

pranitbauva1997
In reply to this post by pranitbauva1997
Variable named 'verbose' in builtin/commit.c is consumed by git-status
and git-commit so if a new verbose related behavior is introduced in
git-commit, then it should not affect the behavior of git-status.

One previous commit (title: commit: add a commit.verbose config
variable) introduced a new config variable named commit.verbose,
so care should be taken that it would not affect the behavior of
status.

Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
respect "unspecified" values") changes the initial value of verbose
from 0 to -1. This can cause git-status to display a verbose output even
when it isn't supposed to.

Signed-off-by: Pranit Bauva <[hidden email]>

---
This is a split off from the previous patch 6/6 as suggested by Eric
Sunshine.

Signed-off-by: Pranit Bauva <[hidden email]>
---
 t/t7507-commit-verbose.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index 2bb6d8d..00e0c3d 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -144,4 +144,14 @@ do
  "
 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
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

Junio C Hamano
Pranit Bauva <[hidden email]> writes:

> Variable named 'verbose' in builtin/commit.c is consumed by git-status
> and git-commit so if a new verbose related behavior is introduced in
> git-commit, then it should not affect the behavior of git-status.
>
> One previous commit (title: commit: add a commit.verbose config
> variable) introduced a new config variable named commit.verbose,
> so care should be taken that it would not affect the behavior of
> status.
>
> Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
> respect "unspecified" values") changes the initial value of verbose
> from 0 to -1. This can cause git-status to display a verbose output even
> when it isn't supposed to.
>
> Signed-off-by: Pranit Bauva <[hidden email]>
>
> ---
> This is a split off from the previous patch 6/6 as suggested by Eric
> Sunshine.

If these are documenting what your previous patches broke, then
there test body should describe what should happen, and then if it
is broken, use test_expect_failure, no?

Your first test does "run status with commit.verbose is set, and
make sure the "diff --git" does not appear", which is correct, so if
it does not work, test_expect_failure would be the right thing to
use.

These, especially the latter, look rather unpleasant regressions to
me, and the main commit.verbose change would need to be held back
before they are fixed.

> ---
>  t/t7507-commit-verbose.sh | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> index 2bb6d8d..00e0c3d 100755
> --- a/t/t7507-commit-verbose.sh
> +++ b/t/t7507-commit-verbose.sh
> @@ -144,4 +144,14 @@ do
>   "
>  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

pranitbauva1997
On Tue, May 3, 2016 at 4:37 AM, Junio C Hamano <[hidden email]> wrote:

> Pranit Bauva <[hidden email]> writes:
>
>> Variable named 'verbose' in builtin/commit.c is consumed by git-status
>> and git-commit so if a new verbose related behavior is introduced in
>> git-commit, then it should not affect the behavior of git-status.
>>
>> One previous commit (title: commit: add a commit.verbose config
>> variable) introduced a new config variable named commit.verbose,
>> so care should be taken that it would not affect the behavior of
>> status.
>>
>> Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
>> respect "unspecified" values") changes the initial value of verbose
>> from 0 to -1. This can cause git-status to display a verbose output even
>> when it isn't supposed to.
>>
>> Signed-off-by: Pranit Bauva <[hidden email]>
>>
>> ---
>> This is a split off from the previous patch 6/6 as suggested by Eric
>> Sunshine.
>
> If these are documenting what your previous patches broke, then
> there test body should describe what should happen, and then if it
> is broken, use test_expect_failure, no?
>
> Your first test does "run status with commit.verbose is set, and
> make sure the "diff --git" does not appear", which is correct, so if
> it does not work, test_expect_failure would be the right thing to
> use.
>
> These, especially the latter, look rather unpleasant regressions to
> me, and the main commit.verbose change would need to be held back
> before they are fixed.

I agree that using test_expect_failure would be a better way of going
with this thing. Thanks. Will send an updated patch for this.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

Eric Sunshine
On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva <[hidden email]> wrote:

> On Tue, May 3, 2016 at 4:37 AM, Junio C Hamano <[hidden email]> wrote:
>> Pranit Bauva <[hidden email]> writes:
>>> Variable named 'verbose' in builtin/commit.c is consumed by git-status
>>> and git-commit so if a new verbose related behavior is introduced in
>>> git-commit, then it should not affect the behavior of git-status.
>>>
>>> One previous commit (title: commit: add a commit.verbose config
>>> variable) introduced a new config variable named commit.verbose,
>>> so care should be taken that it would not affect the behavior of
>>> status.
>>>
>>> Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
>>> respect "unspecified" values") changes the initial value of verbose
>>> from 0 to -1. This can cause git-status to display a verbose output even
>>> when it isn't supposed to.
>>>
>>> Signed-off-by: Pranit Bauva <[hidden email]>
>>
>> If these are documenting what your previous patches broke, then
>> there test body should describe what should happen, and then if it
>> is broken, use test_expect_failure, no?
>>
>> Your first test does "run status with commit.verbose is set, and
>> make sure the "diff --git" does not appear", which is correct, so if
>> it does not work, test_expect_failure would be the right thing to
>> use.
>>
>> These, especially the latter, look rather unpleasant regressions to
>> me, and the main commit.verbose change would need to be held back
>> before they are fixed.
>
> I agree that using test_expect_failure would be a better way of going
> with this thing. Thanks. Will send an updated patch for this.

Please don't. test_expect_failure() is not warranted.

Step back a moment and recall why these tests were added. Earlier
rounds of this series were buggy and caused regressions in git-status.
As a consequence, reviewers suggested[1,2] that you improve test
coverage to ensure that such breakage is caught early.

The problems which caused the regressions were addressed in later
versions of the series, thus using test_expect_success() is indeed
correct, whereas test_expect_failure(), which illustrates broken
behavior, would be the wrong choice.

The point of these new tests is to prevent regressions caused by
*subsequent* changes, which is why it was suggested that these tests
be added early (as a "preparatory patch"[3]), not at the very end of
the series as done here in v15.

This patch's commit message is perhaps a bit too detailed about what
could have gone wrong in earlier patches in this series; indeed, it
misled Junio into thinking that patches in this series did break
behavior, when in fact, it was instead previous rounds of this series
which were buggy. If you instead make this a preparatory patch[3],
then you can sell it more simply by explaining that git-commit and
git-status share implementation (without necessarily going into detail
about exactly what is shared), and that you're improving test coverage
to ensure that changes specific to git-commit don't accidentally
impact git-status, as well.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/288634/focus=288648
[2]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289730
[3]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=291468
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

pranitbauva1997
On Tue, May 3, 2016 at 10:42 AM, Eric Sunshine <[hidden email]> wrote:

> On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva <[hidden email]> wrote:
>> On Tue, May 3, 2016 at 4:37 AM, Junio C Hamano <[hidden email]> wrote:
>>> Pranit Bauva <[hidden email]> writes:
>>>> Variable named 'verbose' in builtin/commit.c is consumed by git-status
>>>> and git-commit so if a new verbose related behavior is introduced in
>>>> git-commit, then it should not affect the behavior of git-status.
>>>>
>>>> One previous commit (title: commit: add a commit.verbose config
>>>> variable) introduced a new config variable named commit.verbose,
>>>> so care should be taken that it would not affect the behavior of
>>>> status.
>>>>
>>>> Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
>>>> respect "unspecified" values") changes the initial value of verbose
>>>> from 0 to -1. This can cause git-status to display a verbose output even
>>>> when it isn't supposed to.
>>>>
>>>> Signed-off-by: Pranit Bauva <[hidden email]>
>>>
>>> If these are documenting what your previous patches broke, then
>>> there test body should describe what should happen, and then if it
>>> is broken, use test_expect_failure, no?
>>>
>>> Your first test does "run status with commit.verbose is set, and
>>> make sure the "diff --git" does not appear", which is correct, so if
>>> it does not work, test_expect_failure would be the right thing to
>>> use.
>>>
>>> These, especially the latter, look rather unpleasant regressions to
>>> me, and the main commit.verbose change would need to be held back
>>> before they are fixed.
>>
>> I agree that using test_expect_failure would be a better way of going
>> with this thing. Thanks. Will send an updated patch for this.
>
> Please don't. test_expect_failure() is not warranted.

I got confused between test_must_fail and test_expect_failure. I
thought Junio mentioned to use test_must_fail and remove the " ! "
sign.

> Step back a moment and recall why these tests were added. Earlier
> rounds of this series were buggy and caused regressions in git-status.
> As a consequence, reviewers suggested[1,2] that you improve test
> coverage to ensure that such breakage is caught early.
>
> The problems which caused the regressions were addressed in later
> versions of the series, thus using test_expect_success() is indeed
> correct, whereas test_expect_failure(), which illustrates broken
> behavior, would be the wrong choice.
>
> The point of these new tests is to prevent regressions caused by
> *subsequent* changes, which is why it was suggested that these tests
> be added early (as a "preparatory patch"[3]), not at the very end of
> the series as done here in v15.
>
> This patch's commit message is perhaps a bit too detailed about what
> could have gone wrong in earlier patches in this series; indeed, it
> misled Junio into thinking that patches in this series did break
> behavior, when in fact, it was instead previous rounds of this series
> which were buggy. If you instead make this a preparatory patch[3],
> then you can sell it more simply by explaining that git-commit and
> git-status share implementation (without necessarily going into detail
> about exactly what is shared), and that you're improving test coverage
> to ensure that changes specific to git-commit don't accidentally
> impact git-status, as well.

Sure! I just wanted the commit message to be detailed as per the
guidelines given by SubmittingPatches. I will swap the patch 6/7 and
patch 7/7 changing the commit message. Also I will make the commit
message less detailed.

>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/288634/focus=288648
> [2]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=289730
> [3]: http://thread.gmane.org/gmane.comp.version-control.git/288820/focus=291468
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

Eric Sunshine
On Tue, May 3, 2016 at 2:42 AM, Pranit Bauva <[hidden email]> wrote:

> On Tue, May 3, 2016 at 10:42 AM, Eric Sunshine <[hidden email]> wrote:
>> On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva <[hidden email]> wrote:
>>> I agree that using test_expect_failure would be a better way of going
>>> with this thing. Thanks. Will send an updated patch for this.
>>
>> Please don't. test_expect_failure() is not warranted.
>
> I got confused between test_must_fail and test_expect_failure. I
> thought Junio mentioned to use test_must_fail and remove the " ! "
> sign.
>
>> Step back a moment and recall why these tests were added. Earlier
>> rounds of this series were buggy and caused regressions in git-status.
>> As a consequence, reviewers suggested[1,2] that you improve test
>> coverage to ensure that such breakage is caught early.
>>
>> The problems which caused the regressions were addressed in later
>> versions of the series, thus using test_expect_success() is indeed
>> correct, whereas test_expect_failure(), which illustrates broken
>> behavior, would be the wrong choice.
>>
>> The point of these new tests is to prevent regressions caused by
>> *subsequent* changes, which is why it was suggested that these tests
>> be added early (as a "preparatory patch"[3]), not at the very end of
>> the series as done here in v15.
>>
>> This patch's commit message is perhaps a bit too detailed about what
>> could have gone wrong in earlier patches in this series; indeed, it
>> misled Junio into thinking that patches in this series did break
>> behavior, when in fact, it was instead previous rounds of this series
>> which were buggy. If you instead make this a preparatory patch[3],
>> then you can sell it more simply by explaining that git-commit and
>> git-status share implementation (without necessarily going into detail
>> about exactly what is shared), and that you're improving test coverage
>> to ensure that changes specific to git-commit don't accidentally
>> impact git-status, as well.
>
> Sure! I just wanted the commit message to be detailed as per the
> guidelines given by SubmittingPatches. I will swap the patch 6/7 and
> patch 7/7 changing the commit message. Also I will make the commit
> message less detailed.

This patch should be inserted before 4/7 since it needs to protect
against breakage which might occur when 4/7 changes the behavior of
OPTION_COUNTUP.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

pranitbauva1997
On Tue, May 3, 2016 at 12:19 PM, Eric Sunshine <[hidden email]> wrote:

> On Tue, May 3, 2016 at 2:42 AM, Pranit Bauva <[hidden email]> wrote:
>> On Tue, May 3, 2016 at 10:42 AM, Eric Sunshine <[hidden email]> wrote:
>>> On Mon, May 2, 2016 at 11:39 PM, Pranit Bauva <[hidden email]> wrote:
>>>> I agree that using test_expect_failure would be a better way of going
>>>> with this thing. Thanks. Will send an updated patch for this.
>>>
>>> Please don't. test_expect_failure() is not warranted.
>>
>> I got confused between test_must_fail and test_expect_failure. I
>> thought Junio mentioned to use test_must_fail and remove the " ! "
>> sign.
>>
>>> Step back a moment and recall why these tests were added. Earlier
>>> rounds of this series were buggy and caused regressions in git-status.
>>> As a consequence, reviewers suggested[1,2] that you improve test
>>> coverage to ensure that such breakage is caught early.
>>>
>>> The problems which caused the regressions were addressed in later
>>> versions of the series, thus using test_expect_success() is indeed
>>> correct, whereas test_expect_failure(), which illustrates broken
>>> behavior, would be the wrong choice.
>>>
>>> The point of these new tests is to prevent regressions caused by
>>> *subsequent* changes, which is why it was suggested that these tests
>>> be added early (as a "preparatory patch"[3]), not at the very end of
>>> the series as done here in v15.
>>>
>>> This patch's commit message is perhaps a bit too detailed about what
>>> could have gone wrong in earlier patches in this series; indeed, it
>>> misled Junio into thinking that patches in this series did break
>>> behavior, when in fact, it was instead previous rounds of this series
>>> which were buggy. If you instead make this a preparatory patch[3],
>>> then you can sell it more simply by explaining that git-commit and
>>> git-status share implementation (without necessarily going into detail
>>> about exactly what is shared), and that you're improving test coverage
>>> to ensure that changes specific to git-commit don't accidentally
>>> impact git-status, as well.
>>
>> Sure! I just wanted the commit message to be detailed as per the
>> guidelines given by SubmittingPatches. I will swap the patch 6/7 and
>> patch 7/7 changing the commit message. Also I will make the commit
>> message less detailed.
>
> This patch should be inserted before 4/7 since it needs to protect
> against breakage which might occur when 4/7 changes the behavior of
> OPTION_COUNTUP.

I forgot to mention about this earlier. When I was rebasing, this stroked me.
I guess making any changes in ordering the commits will make one of
the test as absurd. One of the test uses a configuration variable
'commit.verbose' will won't be effective before the patch 6/7. So I
guess I will have to only change the commit message to reflect as
"improving test coverage".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

Junio C Hamano
In reply to this post by Eric Sunshine
Eric Sunshine <[hidden email]> writes:

>>>> One previous commit (title: commit: add a commit.verbose config
>>>> variable) introduced a new config variable named commit.verbose,
>>>> so care should be taken that it would not affect the behavior of
>>>> status.
>>>>
>>>> Another previous commit (title: "parse-options.c: make OPTION_COUNTUP
>>>> respect "unspecified" values") changes the initial value of verbose
>>>> from 0 to -1. This can cause git-status to display a verbose output even
>>>> when it isn't supposed to.
>>>> ...
>
> This patch's commit message is perhaps a bit too detailed about what
> could have gone wrong in earlier patches in this series; indeed, it
> misled Junio into thinking that patches in this series did break
> behavior, when in fact, it was instead previous rounds of this series
> which were buggy.

Indeed.  Please forget everything I said about expect-failure, if
the top two paragraphs are describing breakages that this series
does *NOT* introduce.  I was misled by them--and others will, too.
These two paragraphs do not belong to the log message.

Thanks for clarifying.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

Eric Sunshine
In reply to this post by pranitbauva1997
On Tue, May 3, 2016 at 5:18 AM, Pranit Bauva <[hidden email]> wrote:

> On Tue, May 3, 2016 at 12:19 PM, Eric Sunshine <[hidden email]> wrote:
>>>> Step back a moment and recall why these tests were added. Earlier
>>>> rounds of this series were buggy and caused regressions in git-status.
>>>> As a consequence, reviewers suggested[1,2] that you improve test
>>>> coverage to ensure that such breakage is caught early.
>>>>
>>>> The point of these new tests is to prevent regressions caused by
>>>> *subsequent* changes, which is why it was suggested that these tests
>>>> be added early (as a "preparatory patch"[3]), not at the very end of
>>>> the series as done here in v15.
>>>
>>> Sure! I just wanted the commit message to be detailed as per the
>>> guidelines given by SubmittingPatches. I will swap the patch 6/7 and
>>> patch 7/7 changing the commit message. Also I will make the commit
>>> message less detailed.
>>
>> This patch should be inserted before 4/7 since it needs to protect
>> against breakage which might occur when 4/7 changes the behavior of
>> OPTION_COUNTUP.
>
> I forgot to mention about this earlier. When I was rebasing, this stroked me.
> I guess making any changes in ordering the commits will make one of
> the test as absurd. One of the test uses a configuration variable
> 'commit.verbose' will won't be effective before the patch 6/7. So I
> guess I will have to only change the commit message to reflect as
> "improving test coverage".

I also had intended to talk about this but forgot. What would be quite
logical is to introduce only the "git-status without --verbose" test
in this new "improve coverage" patch before 4/7. The other test, which
ensures that git-status doesn't regress with commit.verbose, would
then very naturally be included in the patch which adds the
commit.verbose functionality (currently patch 6/7).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v15 7/7] t/t7507: tests for broken behavior of status

pranitbauva1997
On Tue, May 3, 2016 at 9:47 PM, Eric Sunshine <[hidden email]> wrote:

> On Tue, May 3, 2016 at 5:18 AM, Pranit Bauva <[hidden email]> wrote:
>> On Tue, May 3, 2016 at 12:19 PM, Eric Sunshine <[hidden email]> wrote:
>>>>> Step back a moment and recall why these tests were added. Earlier
>>>>> rounds of this series were buggy and caused regressions in git-status.
>>>>> As a consequence, reviewers suggested[1,2] that you improve test
>>>>> coverage to ensure that such breakage is caught early.
>>>>>
>>>>> The point of these new tests is to prevent regressions caused by
>>>>> *subsequent* changes, which is why it was suggested that these tests
>>>>> be added early (as a "preparatory patch"[3]), not at the very end of
>>>>> the series as done here in v15.
>>>>
>>>> Sure! I just wanted the commit message to be detailed as per the
>>>> guidelines given by SubmittingPatches. I will swap the patch 6/7 and
>>>> patch 7/7 changing the commit message. Also I will make the commit
>>>> message less detailed.
>>>
>>> This patch should be inserted before 4/7 since it needs to protect
>>> against breakage which might occur when 4/7 changes the behavior of
>>> OPTION_COUNTUP.
>>
>> I forgot to mention about this earlier. When I was rebasing, this stroked me.
>> I guess making any changes in ordering the commits will make one of
>> the test as absurd. One of the test uses a configuration variable
>> 'commit.verbose' will won't be effective before the patch 6/7. So I
>> guess I will have to only change the commit message to reflect as
>> "improving test coverage".
>
> I also had intended to talk about this but forgot. What would be quite
> logical is to introduce only the "git-status without --verbose" test
> in this new "improve coverage" patch before 4/7. The other test, which
> ensures that git-status doesn't regress with commit.verbose, would
> then very naturally be included in the patch which adds the
> commit.verbose functionality (currently patch 6/7).

Sure. Will do. Thanks!
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v15 3/7] t0040-parse-options: improve test coverage

Eric Sunshine
In reply to this post by pranitbauva1997
On Sat, Apr 30, 2016 at 4:03 PM, Pranit Bauva <[hidden email]> wrote:
> Include tests to check for multiple levels of quiet and to check if the
> '--no-quiet' option sets it to 0.

As this patch is also adding a test of --[no-]verbose, the commit
message should mention it.

More below...

> Signed-off-by: Pranit Bauva <[hidden email]>
> ---
> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> @@ -476,4 +476,61 @@ test_expect_success '--no-list resets list' '
> +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
> +'
> +
> +test_expect_success '--no-quiet sets quiet to 0' '
> +       test-parse-options -q -q -q --no-quiet >output 2>output.err &&
> +       test_must_be_empty output.err &&
> +       test_cmp expect output
> +'

It wouldn't hurt to have two tests for --no-quiet: one which tests
--no-quiet alone to ensure that 'quiet' *remains* at 0, and one which
tests --no-quiet in combination with some --quiet's to ensure that
'quiet' is *reset* to 0. These tests would give you good coverage for
changes by subsequent patches, such as the OPTION_COUNTUP patch which
flips the initial value to -1.

> +
> +test_expect_success '--no-verbose sets verbose to 0' '
> +       test-parse-options --no-verbose >output 2> output.err &&
> +       test_must_be_empty output.err &&
> +       test_cmp expect output
> +'

One would expect to see 'verbose' get the same treatment of having a
test invoke --verbose multiple times. (Yes, I realize that the "long
options" test does just this, but testing multiple --verbose's is not
its primary purpose, so having a test which does test multiple
--verbose's as its primary purpose can be beneficial and is less
likely to be broken by someone in the future.)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v15 3/7] t0040-parse-options: improve test coverage

pranitbauva1997
On Wed, May 4, 2016 at 2:06 PM, Eric Sunshine <[hidden email]> wrote:
> On Sat, Apr 30, 2016 at 4:03 PM, Pranit Bauva <[hidden email]> wrote:
>> Include tests to check for multiple levels of quiet and to check if the
>> '--no-quiet' option sets it to 0.
>
> As this patch is also adding a test of --[no-]verbose, the commit
> message should mention it.

Will include this in commit message.

>
> More below...
>
>> Signed-off-by: Pranit Bauva <[hidden email]>
>> ---
>> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
>> @@ -476,4 +476,61 @@ test_expect_success '--no-list resets list' '
>> +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
>> +'
>> +
>> +test_expect_success '--no-quiet sets quiet to 0' '
>> +       test-parse-options -q -q -q --no-quiet >output 2>output.err &&
>> +       test_must_be_empty output.err &&
>> +       test_cmp expect output
>> +'
>
> It wouldn't hurt to have two tests for --no-quiet: one which tests
> --no-quiet alone to ensure that 'quiet' *remains* at 0, and one which
> tests --no-quiet in combination with some --quiet's to ensure that
> 'quiet' is *reset* to 0. These tests would give you good coverage for
> changes by subsequent patches, such as the OPTION_COUNTUP patch which
> flips the initial value to -1.

Will add them

>> +
>> +test_expect_success '--no-verbose sets verbose to 0' '
>> +       test-parse-options --no-verbose >output 2> output.err &&
>> +       test_must_be_empty output.err &&
>> +       test_cmp expect output
>> +'
>
> One would expect to see 'verbose' get the same treatment of having a
> test invoke --verbose multiple times. (Yes, I realize that the "long
> options" test does just this, but testing multiple --verbose's is not
> its primary purpose, so having a test which does test multiple
> --verbose's as its primary purpose can be beneficial and is less
> likely to be broken by someone in the future.)

Sure. Having another test dedicated wouldn't hurt.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v16 0/7] config commit verbose

pranitbauva1997
In reply to this post by pranitbauva1997
This series of patches add a configuration variable for verbose in
git-commit.

Link to v15:
http://thread.gmane.org/gmane.comp.version-control.git/293127

Changes wrt v15:
 * Remove the previous patch 7/7 and split the tests. Include one in
   initial patch 6/7. The other one is introduced in a separate commit
   after 4/7.
 * Include tests in patch 3/6 for --no-quiet without -q, multiple verbose,
   --no-verbose with -v as suggested by Eric Sunshine

Pranit Bauva (7):
  t0040-test-parse-options.sh: fix style issues
  test-parse-options: print quiet as integer
  t0040-parse-options: improve test coverage
  t/t7507: improve test coverage
  parse-options.c: make OPTION_COUNTUP respect "unspecified" values
  t7507-commit-verbose: improve test coverage by testing number of diffs
  commit: add a commit.verbose config variable

 Documentation/config.txt                      |   4 +
 Documentation/git-commit.txt                  |   3 +-
 Documentation/technical/api-parse-options.txt |   8 +-
 builtin/commit.c                              |  14 +-
 parse-options.c                               |   2 +
 t/t0040-parse-options.sh                      | 238 +++++++++++++++++++-------
 t/t7507-commit-verbose.sh                     |  72 +++++++-
 test-parse-options.c                          |   5 +-
 8 files changed, 271 insertions(+), 75 deletions(-)

--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

[PATCH v16 1/7] t0040-test-parse-options.sh: fix style issues

pranitbauva1997
Signed-off-by: Pranit Bauva <[hidden email]>

---

 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 <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 <<EOF
+cat >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 <<EOF
+cat >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 <<EOF
+cat >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 <<EOF
+cat >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 <<EOF
+cat >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 <<EOF
+cat >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 <<EOF
+cat >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 <<EOF
+cat >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 <<EOF
+cat >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
--
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
123