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

[Junio C Hamano <gitster@pobox.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.

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

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

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

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

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

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

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