Re: [GSoC][PATCH v5] Make options that expect object ids less chatty if id is invalid

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 19, 2018, 21:54 UTC
Message-ID: <xmqqd0zzu3x6.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180319185436.14309-1-ungureanupaulsebastian@gmail.com>

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

> Subject: Re: [GSoC][PATCH v5] Make options that expect object ids less chatty if id is invalid

For the past few iterations, this is no longer about "options that expect object IDs that get an invalid one" anymore, right? The fix has become a lot more generic and extended in scope.

> Usually, the usage should be shown only if the user does not know what
> options are available. If the user specifies an invalid value, the user
> is already aware of the available options. In this case, there is no
> point in displaying the usage anymore.

This presents the more general fix implemented in these later rounds rather well.

	parse-options: do not show usage upon invalid option value

may be a title that match the more recent state of this topioc better.

Show 14 quoted lines

> diff --git a/t/t0041-usage.sh b/t/t0041-usage.sh
> new file mode 100755
> index 000000000..2fc08ae70
> --- /dev/null
> +++ b/t/t0041-usage.sh
> @@ -0,0 +1,89 @@
> +#!/bin/sh
> +
> +test_description='Test commands behavior when given invalid argument value'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup ' '
> +	git init . &&

Do you need this "git init ."?  I somehow doubt it.

Show 19 quoted lines

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

So... at this point there are v1.0 and v1.1 that point at the same commit and there is no other comit nor tag. There is no tag that does not contain v1.1 so the output is empty.

Which is correct, but is a rather boring thing to try ;-).

> +test_expect_success 'tag --no-contains <inexistent_tag>' '
> +	test_must_fail git tag --no-contains "notag" 2>actual &&
> +	test_i18ngrep "error" actual
> +'

Good.  Don't we need to check that this does not have "usage" in it?

Show 14 quoted lines

> +test_expect_success 'tag usage error' '
> +	test_must_fail git tag --noopt 2>actual &&
> +	test_i18ngrep "usage" actual
> +'
> +
> +test_expect_success 'branch --contains <existent_commit>' '
> +	git branch --contains "master" >actual &&
> +	test_i18ngrep "master" actual
> +'
> +
> +test_expect_success 'branch --contains <inexistent_commit>' '
> +	test_must_fail git branch --no-contains "nocommit" 2>actual &&
> +	test_i18ngrep "error" actual
> +'

Likewise.

Show 9 quoted lines

> +test_expect_success 'branch --no-contains <existent_commit>' '
> +	git branch --no-contains "master" >actual &&
> +	test_line_count = 0 actual
> +'
> +
> +test_expect_success 'branch --no-contains <inexistent_commit>' '
> +	test_must_fail git branch --no-contains "nocommit" 2>actual &&
> +	test_i18ngrep "error" actual
> +'

Likewise.

Show 14 quoted lines

> +test_expect_success 'branch usage error' '
> +	test_must_fail git branch --noopt 2>actual &&
> +	test_i18ngrep "usage" actual
> +'
> +
> +test_expect_success 'for-each-ref --contains <existent_object>' '
> +	git for-each-ref --contains "master" >actual &&
> +	test_line_count = 3 actual
> +'
> +
> +test_expect_success 'for-each-ref --contains <inexistent_object>' '
> +	test_must_fail git for-each-ref --no-contains "noobject" 2>actual &&
> +	test_i18ngrep "error" actual
> +'

Likewise.

Show 9 quoted lines

> +test_expect_success 'for-each-ref --no-contains <existent_object>' '
> +	git for-each-ref --no-contains "master" >actual &&
> +	test_line_count = 0 actual
> +'
> +
> +test_expect_success 'for-each-ref --no-contains <inexistent_object>' '
> +	test_must_fail git for-each-ref --no-contains "noobject" 2>actual &&
> +	test_i18ngrep "error" actual
> +'

Likewise.

Show 22 quoted lines

> +test_expect_success 'for-each-ref usage error' '
> +	test_must_fail git for-each-ref --noopt 2>actual &&
> +	test_i18ngrep "usage" actual
> +'
> +
> +test_done
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index ef2887bd8..cac8b2bd8 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -919,10 +919,8 @@ test_expect_success 'rebase --exec works without -i ' '
>  test_expect_success 'rebase -i --exec without <CMD>' '
>  	git reset --hard execute &&
>  	set_fake_editor &&
> -	test_must_fail git rebase -i --exec 2>tmp &&
> -	sed -e "1d" tmp >actual &&
> -	test_must_fail git rebase -h >expected &&
> -	test_cmp expected actual &&
> +	test_must_fail git rebase -i --exec 2>actual &&
> +	test_i18ngrep "requires a value" actual &&
>  	git checkout master
>  '