Re: [PATCH v12 2/5] test-parse-options: print quiet as integer
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Apr 3, 2016, 21:30 UTC
- Message-ID
- <CAPig+cQpdr7A7GxKUN3tDs2rQ7z_S5b1WVsk01=BwkCwB2mp0A@mail.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 <pranit.bauva@gmail.com> 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).
Show 139 quoted lines
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> ---
> 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)");