threads / patch / 48655

patcht3200-branch.sh: use "--set-upstream-to" in test

Subject: [PATCH] t3200-branch.sh: use "--set-upstream-to" in test

## tl;dr

8 messages between Jun 5, 2018 and Jun 17, 2018. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Robert P. J. Day· Jun 5, 2018, 11:14 UTC · lore

Change deprecated "--set-upstream" branch option to preferred "--set-upstream-to".

Signed-off-by: Robert P. J. Day <rpjday@crashcourse.ca>
---
  i'm assuming this should use "--set-upstream-to" as do all the
others.
Show changes to t/t3200-branch.sh +2 −2
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 69ea8202f4..ef887a0b32 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -885,8 +885,8 @@ 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_must_fail git branch --set-upstream origin/master
+test_expect_success '--set-upstream-to fails' '
+    test_must_fail git branch --set-upstream-to origin/master
 '

 test_expect_success '--set-upstream-to notices an error to set branch as own upstream' '
-- 
========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                  http://crashcourse.ca/dokuwiki

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================
SZEDER Gábor· Jun 5, 2018, 11:24 UTC · re: Robert P. J. Day · lore

Re: [PATCH] t3200-branch.sh: use "--set-upstream-to" in test

Show 9 quoted lines
> Change deprecated "--set-upstream" branch option to
> preferred "--set-upstream-to".
> 
> Signed-off-by: Robert P. J. Day <rpjday@crashcourse.ca>
> 
> ---
> 
>   i'm assuming this should use "--set-upstream-to" as do all the
> others.

I don't think so, see 52668846ea (builtin/branch: stop supporting the "--set-upstream" option, 2017-08-17).

Though arguably the test name could be more descriptive and tell why it should fail.

Show 26 quoted lines
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index 69ea8202f4..ef887a0b32 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -885,8 +885,8 @@ 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_must_fail git branch --set-upstream origin/master
> +test_expect_success '--set-upstream-to fails' '
> +    test_must_fail git branch --set-upstream-to origin/master
>  '
> 
>  test_expect_success '--set-upstream-to notices an error to set branch as own upstream' '
> 
> -- 
> 
> ========================================================================
> Robert P. J. Day                                 Ottawa, Ontario, CANADA
>                   http://crashcourse.ca/dokuwiki
> 
> Twitter:                                       http://twitter.com/rpjday
> LinkedIn:                               http://ca.linkedin.com/in/rpjday
> ========================================================================
> 
Robert P. J. Day· Jun 5, 2018, 11:34 UTC · re: SZEDER Gábor · lore

Re: [PATCH] t3200-branch.sh: use "--set-upstream-to" in test

On Tue, 5 Jun 2018, SZEDER Gábor wrote:
Show 12 quoted lines
> > Change deprecated "--set-upstream" branch option to
> > preferred "--set-upstream-to".
> >
> > Signed-off-by: Robert P. J. Day <rpjday@crashcourse.ca>
> >
> > ---
> >
> >   i'm assuming this should use "--set-upstream-to" as do all the
> > others.
>
> I don't think so, see 52668846ea (builtin/branch: stop supporting
> the "--set-upstream" option, 2017-08-17).
  yes, you're right, i didn't look at the context enough. my bad.
rday
-- 
========================================================================
Robert P. J. Day                                 Ottawa, Ontario, CANADA
                  http://crashcourse.ca/dokuwiki

Twitter:                                       http://twitter.com/rpjday
LinkedIn:                               http://ca.linkedin.com/in/rpjday
========================================================================
Kaartic Sivaraam· Jun 6, 2018, 15:50 UTC · re: SZEDER Gábor · lore

Re: [PATCH] t3200-branch.sh: use "--set-upstream-to" in test

On Tuesday 05 June 2018 04:54 PM, SZEDER Gábor wrote:
> 
> Though arguably the test name could be more descriptive and tell why
> it should fail.
> 

That's arguable, indeed. I was about to send a patch that gives a better description for the test. I didn't do it as I started wondering, Is it even worth testing whether a removed option fails? Is this done for other options that have been removed in the past? Should we just remove the test completely?

-- 
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!
Kaartic Sivaraam· Jun 14, 2018, 14:06 UTC · re: SZEDER Gábor · lore

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

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 <kaartic.sivaraam@gmail.com>
---
 t/t3200-branch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t3200-branch.sh +1 −1
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
Junio C Hamano· Jun 14, 2018, 17:43 UTC · re: Kaartic Sivaraam · lore

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

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
Show 16 quoted lines
> 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 <kaartic.sivaraam@gmail.com>
> ---
>  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.

Show 10 quoted lines
> 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
>  '
Kaartic Sivaraam· Jun 17, 2018, 08:53 UTC · re: Junio C Hamano · lore

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

On Thursday 14 June 2018 11:13 PM, Junio C Hamano wrote:
Show 6 quoted lines
> 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.
Show 14 quoted lines
>> 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!
Kaartic Sivaraam· Jun 17, 2018, 11:56 UTC · re: Junio C Hamano · lore

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

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 instead of silently acting as an alias for the '--set-upstream-to' option due to option parsing features.

To avoid confusion, clarify that the option is disabled intentionally in the corresponding test description.

The test is expected to be around as long as we intentionally fail on seeing the '--set-upstream' option which in turn we expect to do for a period of time after which we can be sure that existing users of '--set-upstream' are aware that the option is no longer supported.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
Changes since v1:
	- update the test description as suggested by Junio
	- updated the commit message to be clear about period upto
	  which we should be testing this.
 t/t3200-branch.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t3200-branch.sh +1 −1
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 6c0b7ea4a..e98dbfc1a 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 'disabled option --set-upstream fails' '
     test_must_fail git branch --set-upstream origin/master
 '
 
-- 
2.18.0.rc1.242.g61856ae69

← back to recent threads