Re: [GSoC][PATCH v6] parse-options: do not show usage upon invalid option value

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 21, 2018, 18:22 UTC
Message-ID: <xmqqk1u5p9um.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180320175005.18759-1-ungureanupaulsebastian@gmail.com>

Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> writes:

Show 18 quoted lines

> diff --git a/t/t0041-usage.sh b/t/t0041-usage.sh
> new file mode 100755
> index 000000000..ac96bc3b9
> --- /dev/null
> +++ b/t/t0041-usage.sh
> @@ -0,0 +1,107 @@
> +#!/bin/sh
> +
> +test_description='Test commands behavior when given invalid argument value'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup ' '
> +	test_commit "v1.0"
> +'
> +
> +test_expect_success 'tag --contains <existent_tag>' '
> +	git tag --contains "v1.0" 1>actual 2>actual.err &&

It is not wrong per-se, but >redirection without a number redirects fd#1, so "1>actual" is an unusual way to spell ">actual".

Show 9 quoted lines

> +	grep "v1.0" actual &&
> +	test_line_count = 0 actual.err
> +'
> +
> +test_expect_success 'tag --contains <inexistent_tag>' '
> +	test_must_fail git tag --contains "notag" 1>actual 2>actual.err &&
> +	test_line_count = 0 actual &&
> +	test_i18ngrep "error" actual.err &&
> +	test_must_fail test_i18ngrep "usage" actual.err

test_must_fail mustn't be used like that for two reasons

 - It is to be used for "git" stuff, because it knows failing with
   segfault and other failure modes are _wrong_ and should not be
   happy even if the command "fail"ed.  We however do not expect
   commands that are not git (e.g. test_i18ngrep) to require special
   casing of the failure modes.

 - test_i18ngrep pretends to always "have found match" when running
   under GETTEXT_POISON build, so it will pretend that usage exists
   in actual.error.  test_must_fail will then say "oops, the string
   'usage' shouldn't appear in the output but it did", which is not
   what you want.

Perhaps

	test_i18ngrep ! "usage" actual.err

is what you want to say here instead.

In any case, the tests got a lot cleaner compared to the previous round, and I feel that this patch is "getting there" ;-)

Thanks for working on it.