On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva <pranit.bauva@xxxxxxxxx> 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 <pranit.bauva@xxxxxxxxx>
> ---
> 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)");
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html