Re: [PATCH] t3200: clarify description of --set-upstream test

2018-06-17 Thread Kaartic Sivaraam
On Thursday 14 June 2018 11:13 PM, Junio C Hamano wrote:
> It is technically correct to call --set-upstream "unsupported", but
> the reason why we want to see it fail is not because it is
> unsupported, but because we actively interfere with the usual
> "unique prefix" logic parse-options API gives its users and make it
> not to trigger the longer-and-unique --set-upstream-to logic.
> 

That sounds right.


>> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
>> index 6c0b7ea4a..d14de82ba 100755
>> --- a/t/t3200-branch.sh
>> +++ b/t/t3200-branch.sh
>> @@ -884,7 +884,7 @@ test_expect_success 'test --unset-upstream on a 
>> particular branch' '
>>  test_must_fail git config branch.my14.merge
>>  '
>>  
>> -test_expect_success '--set-upstream fails' '
>> +test_expect_success 'unsupported option --set-upstream fails' '
> 
> In other words, I am wondering if s/unsupported/disabled/ makes it
> even more clear what is going on here.
>

I guess it would :-) Thanks for the better wording. I actually thought
of asking for a better wording for the test message while sending the
patch but somehow forgot to mention it. It seems I've got better
wordings, regardless.

I'll send a v2 with the correction. I'm still open to an alternative
test description in case that make things even more clearer :-)


Thanks,
Sivaraam

QUOTE:

“The three principal virtues of a programmer are Laziness, Impatience,
and Hubris.”

- Camel book

Sivaraam?

You possibly might have noticed that my signature recently changed from
'Kaartic' to 'Sivaraam' both of which are parts of my name. I find the
new signature to be better for several reasons one of which is that the
former signature has a lot of ambiguities in the place I live as it is a
common name (NOTE: it's not a common spelling, just a common name). So,
I switched signatures before it's too late.

That said, I won't mind you calling me 'Kaartic' if you like it [of
course ;-)]. You can always call me using either of the names.


KIND NOTE TO THE NATIVE ENGLISH SPEAKER:

As I'm not a native English speaker myself, there might be mistaeks in
my usage of English. I apologise for any mistakes that I make.

It would be "helpful" if you take the time to point out the mistakes.

It would be "super helpful" if you could provide suggestions about how
to correct those mistakes.

Thanks in advance!



signature.asc
Description: OpenPGP digital signature


Re: [PATCH] t3200: clarify description of --set-upstream test

2018-06-14 Thread Junio C Hamano
Kaartic Sivaraam  writes:

> Support for the --set-upstream option was removed in 52668846ea
> (builtin/branch: stop supporting the "--set-upstream" option,
> 2017-08-17). The change did not completely remove the command
> due to an issue noted in the commit's log message.
>
> So, a test was added to ensure that a command which uses the
> '--set-upstream' option fails and doesn't silently act as an alias
> for the '--set-upstream-to' option due to option parsing features.
>
> To avoid confusion, clarify that the option is an unsupported one
> in the corresponding test description.
>
> Signed-off-by: Kaartic Sivaraam 
> ---
>  t/t3200-branch.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

The above description is much clearer than what the test title after
the patch gives its readers.

It is technically correct to call --set-upstream "unsupported", but
the reason why we want to see it fail is not because it is
unsupported, but because we actively interfere with the usual
"unique prefix" logic parse-options API gives its users and make it
not to trigger the longer-and-unique --set-upstream-to logic.

> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 6c0b7ea4a..d14de82ba 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -884,7 +884,7 @@ test_expect_success 'test --unset-upstream on a 
> particular branch' '
>   test_must_fail git config branch.my14.merge
>  '
>  
> -test_expect_success '--set-upstream fails' '
> +test_expect_success 'unsupported option --set-upstream fails' '

In other words, I am wondering if s/unsupported/disabled/ makes it
even more clear what is going on here.

>  test_must_fail git branch --set-upstream origin/master
>  '



[PATCH] t3200: clarify description of --set-upstream test

2018-06-14 Thread Kaartic Sivaraam
Support for the --set-upstream option was removed in 52668846ea
(builtin/branch: stop supporting the "--set-upstream" option,
2017-08-17). The change did not completely remove the command
due to an issue noted in the commit's log message.

So, a test was added to ensure that a command which uses the
'--set-upstream' option fails and doesn't silently act as an alias
for the '--set-upstream-to' option due to option parsing features.

To avoid confusion, clarify that the option is an unsupported one
in the corresponding test description.

Signed-off-by: Kaartic Sivaraam 
---
 t/t3200-branch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 6c0b7ea4a..d14de82ba 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -884,7 +884,7 @@ test_expect_success 'test --unset-upstream on a particular 
branch' '
test_must_fail git config branch.my14.merge
 '
 
-test_expect_success '--set-upstream fails' '
+test_expect_success 'unsupported option --set-upstream fails' '
 test_must_fail git branch --set-upstream origin/master
 '
 
-- 
2.18.0.rc1.242.g61856ae69